-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display files from referenced crossref in entry table (Toro520 version) #10577
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,4 +11,4 @@ plugins { | |
id("org.gradle.toolchains.foojay-resolver-convention") version "0.7.0" | ||
} | ||
|
||
rootProject.name = "JabRef" | ||
rootProject.name = "jabRef" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do not change something here, this has severe consequences for our installer infrastructure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Toro520 You can also check with "git blame" when this line was changed and go to the pull request for some background. This is a procedure for all changes in important part of the code! We did that change at #10322, but it did not work. Changing that lead to more changes, which had unforseen consquences on macOS: #10345 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @calixtus Hi, I'm so sorry to cause this trouble, but when I first time to install it, I have this kind of issue, and I find the documentation : Changing only the case of the Gradle root project name causes exception while importing project: "java.lang.IllegalStateException: Module entity with name: untitled20.main should be available" from Pre Condition 3: Code on the local machineto solve it. It said directory and project name should be same. Right now I'm confused about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @koppor Thanks so much. Because I'm not sure is that the right way to do this(pull request), so I just tried the draft one to have a practice. You are so enthusiasm and passionate about it. I must could learn a lot from you guys. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,17 +52,17 @@ | |
/** | ||
* Represents a Bib(La)TeX entry, which can be BibTeX or BibLaTeX. | ||
* <p> | ||
* Example: | ||
* Example: | ||
* | ||
* <pre>{@code | ||
* <pre>{@code | ||
* Some commment | ||
* @misc{key, | ||
* fieldName = {fieldValue}, | ||
* otherFieldName = {otherVieldValue} | ||
* } | ||
* }</pre> | ||
* | ||
* Then, | ||
* <p> | ||
* Then, | ||
* <ul> | ||
* <li>"Some comment" is the comment before the entry,</li> | ||
* <li>"misc" is the entry type</li> | ||
|
@@ -157,14 +157,20 @@ public Optional<FieldChange> setMonth(Month parsedMonth) { | |
return setField(StandardField.MONTH, parsedMonth.getJabRefFormat()); | ||
} | ||
|
||
/** | ||
* Retrieves the first resolved field or its alias from the provided OrFields object using Stream API. | ||
* | ||
* @param fields The OrFields object containing a list of fields to be resolved. | ||
* @param database The BibDatabase used for resolving the field or its alias. | ||
* @return An Optional containing the first resolved field or alias if present, | ||
* otherwise an empty Optional. | ||
*/ | ||
public Optional<String> getResolvedFieldOrAlias(OrFields fields, BibDatabase database) { | ||
for (Field field : fields.getFields()) { | ||
Optional<String> value = getResolvedFieldOrAlias(field, database); | ||
if (value.isPresent()) { | ||
return value; | ||
} | ||
} | ||
return Optional.empty(); | ||
return fields.getFields().stream() | ||
.map(field -> getResolvedFieldOrAlias(field, database)) | ||
.filter(Optional::isPresent) | ||
.map(Optional::get) | ||
.findFirst(); | ||
} | ||
Comment on lines
168
to
174
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a side note: Although streams can under certain circumstances speed up the execution and make it more readable (so this change is ok as far as I can see), it makes it more complicated to debug, since the stacktrace is broken up by the jdk internal stream handling. @Siedlerchr should confirm this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it really depends on the context There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Toro520 Here, speed comparisons would be interesting. - I bet that the current for loop is faster - or does the compiler optimize the stream that much so that it is equal? (in the stream version, ALL elements are mapped and then filtered, then mapped and then the first is taken - not sure if there is an optimizuation for that in the JDK) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would not expect this. I agree, current implementation is to return as soon as a hit occurs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. I was just curious. For training the usage of performance. I fully agree that the change has to be reverted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you guys, I have learnt a lot from this, and this really raise my enthusiasm to look through entire code and make the better decision next time. But right now where should I start to solve the issue #9717? I considered how to be like you guys to have the deep learning and understanding of the code and try to solve it. If at current state, first year study CS at ANU, what kind of assistance I could support. I know ,first of all, do not cause too much trouble should be the priority. 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Toro520 The issue is much about code reading. The real code won't be much. -- Try to understand how the main table displays data. I think, for crossref one has to manipulate that thing so that not only the bibentry at hand, but also the cross-ref'd entry is taken into consideration. Loom at the discussion at #7754 - there was a first attempt in that direction. (I do not know about the code exactly. -- If, let's say, within 5 hours, you don't have any clue (you can also ask here or at that PR), consider changing the issue) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @k3KAW8Pnf7mkmdSMPHz27 Just for information: crossref is looked at by a student. Hope, it is manageable ^^ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @koppor Thanks so much. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Intermediate operations should be lazily evaluated
I would use a flatMap rather than two filtering operations, but I don't think the performance improvement is significant.
I can't think of any case where a stream will be faster than a well-written for-loop. Streams are IMO well optimized, but so are for-loops. I'd vote to leave for-loops in performance-critical locations.
@koppor thank you and that is awesome! This really needs to get done. @Toro520 I as well appreciate you taking a look at it! |
||
|
||
/** | ||
|
@@ -478,7 +484,7 @@ public boolean hasField(Field field) { | |
|
||
/** | ||
* Internal method used to get the content of a field (or its alias) | ||
* | ||
* <p> | ||
* Used by {@link #getFieldOrAlias(Field)} and {@link #getFieldOrAliasLatexFree(Field)} | ||
* | ||
* @param field the field | ||
|
@@ -711,7 +717,7 @@ public String toString() { | |
* Author1, Author2: Title (Year) | ||
*/ | ||
public String getAuthorTitleYear(int maxCharacters) { | ||
String[] s = new String[]{getField(StandardField.AUTHOR).orElse("N/A"), getField(StandardField.TITLE).orElse("N/A"), | ||
String[] s = new String[] {getField(StandardField.AUTHOR).orElse("N/A"), getField(StandardField.TITLE).orElse("N/A"), | ||
getField(StandardField.YEAR).orElse("N/A")}; | ||
|
||
String text = s[0] + ": \"" + s[1] + "\" (" + s[2] + ')'; | ||
|
@@ -907,7 +913,7 @@ public boolean equals(Object o) { | |
|
||
/** | ||
* On purpose, this hashes the "content" of the BibEntry, not the {@link #sharedBibEntryData}. | ||
* | ||
* <p> | ||
* The content is | ||
* | ||
* <ul> | ||
|
@@ -928,7 +934,8 @@ public void registerListener(Object object) { | |
public void unregisterListener(Object object) { | ||
try { | ||
this.eventBus.unregister(object); | ||
} catch (IllegalArgumentException e) { | ||
} catch ( | ||
IllegalArgumentException e) { | ||
Comment on lines
+937
to
+938
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please undo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Toro520 If you have time, you can investigate the IntelliJ code style checks why this happens. It is a thing that annoys us, but we did not have found the patience to investigate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, I suppose I do not manipulate this 😢 . Perhaps I used the boss key ctrl + Alt + L, to organized the entire code format. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Toro520 Yes, you did. And in a "proper" IDE configuration, the code is well-formatted. We have an issue in our rules. Quick fix: Undo that change (you can do line-based stashing in git gui. Very helpful for these kind of issues). The real fix would be to fix our rules... But takes time at least 1 hour. But one learns much about IDE configuration and code style management in projects. |
||
// occurs if the event source has not been registered, should not prevent shutdown | ||
LOGGER.debug("Problem unregistering", e); | ||
} | ||
|
@@ -1106,7 +1113,7 @@ public Observable[] getObservables() { | |
* This method. adds the given path (as file) to the entry and removes the url. | ||
* | ||
* @param linkToDownloadedFile the link to the file, which was downloaded | ||
* @param downloadedFile the path to be added to the entry | ||
* @param downloadedFile the path to be added to the entry | ||
*/ | ||
public void replaceDownloadedFile(String linkToDownloadedFile, LinkedFile downloadedFile) { | ||
List<LinkedFile> linkedFiles = this.getFiles(); | ||
|
@@ -1144,7 +1151,7 @@ public void mergeWith(BibEntry other) { | |
* Merge this entry's fields with another BibEntry. Non-intersecting fields will be automatically merged. In cases of | ||
* intersection, priority is given to THIS entry's field value, UNLESS specified otherwise in the arguments. | ||
* | ||
* @param other another BibEntry from which fields are sourced from | ||
* @param other another BibEntry from which fields are sourced from | ||
* @param otherPrioritizedFields collection of Fields in which 'other' has a priority into final result | ||
*/ | ||
public void mergeWith(BibEntry other, Set<Field> otherPrioritizedFields) { | ||
|
@@ -1174,4 +1181,52 @@ public boolean isEmpty() { | |
} | ||
return StandardField.AUTOMATIC_FIELDS.containsAll(this.getFields()); | ||
} | ||
|
||
public void addCrossref(BibEntry crossrefEntry) { | ||
// Adding cross-references | ||
this.setField(StandardField.CROSSREF, crossrefEntry.getCitationKey().toString()); | ||
// Updating Cells | ||
updateCell(); | ||
} | ||
|
||
public void addFileFromCrossref(LinkedFile file) { | ||
List<LinkedFile> linkedFiles = this.getFiles(); | ||
// If the cross-referenced entry already has documentation from the cross-referenced entry, the documentation is not updated | ||
if (!linkedFiles.stream().anyMatch(f -> f.getLink().equalsIgnoreCase(file.getLink()))) { | ||
linkedFiles.add(file); | ||
// Updated documents | ||
updateFiles(linkedFiles); | ||
} | ||
} | ||
|
||
public void removeCrossref() { | ||
// Removing cross-references | ||
// this.removeCrossrefedEntry(StandardField.ENTRYSET); | ||
// Updating Cells | ||
updateCell(); | ||
} | ||
|
||
public void updateCitationKey(String newCitationKey) { | ||
// Changing the reference key of a cross-reference entry | ||
this.setField(InternalField.KEY_FIELD, newCitationKey); | ||
// Updating Cells | ||
updateCell(); | ||
} | ||
|
||
public void removeCrossrefedEntry(BibEntry crossrefedEntry) { | ||
// Remove cross-referenced entries | ||
crossrefedEntry.removeCrossref(); | ||
// Updating Cells | ||
updateCell(); | ||
} | ||
|
||
private void updateCell() { | ||
// Logic for updating cells | ||
// ... | ||
} | ||
|
||
private void updateFiles(List<LinkedFile> linkedFiles) { | ||
// Logic for updating documents | ||
// ... | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not modify the gradle file!