Display files from referenced crossref in entry table (Toro520 version) #10577

Toro520 · 2023-10-25T01:39:07Z

Resolves #7731
Improves code quality of - and is based on - PR #9717

Refactor getResolvedFieldOrAlias for clarity and modern style

Use Stream API for better readability and concise code.
Simplify the logic to fetch the first resolved field or alias.
No change in the method's functionality or output.

Mandatory checks

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Refactor getResolvedFieldOrAlias for clarity and modern style - Use Stream API for better readability and concise code. - Simplify the logic to fetch the first resolved field or alias. - No change in the method's functionality or output.

calixtus · 2023-10-25T11:02:15Z

settings.gradle

@@ -11,4 +11,4 @@ plugins {
    id("org.gradle.toolchains.foojay-resolver-convention") version "0.7.0"
 }

-rootProject.name = "JabRef"
+rootProject.name = "jabRef"


Please do not change something here, this has severe consequences for our installer infrastructure.
As the letter R is still in uppercase, I suggest this change happend by mistake.
As a general advice: Always review your own changes before committing them (use git gui or any other git tool) and only commit changes that are directly linked to your issue and are really intended.

@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

@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.
But now I have fixed it. I hope everything goes well.

@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.

calixtus · 2023-10-25T11:06:11Z

src/main/java/org/jabref/model/entry/BibEntry.java

+        } catch (
+                IllegalArgumentException e) {


Please undo

@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.

Huh, I suppose I do not manipulate this 😢 . Perhaps I used the boss key ctrl + Alt + L, to organized the entire code format.

@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.

calixtus · 2023-10-25T11:20:43Z

src/main/java/org/jabref/model/entry/BibEntry.java

    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();
    }


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.

Yes, it really depends on the context

@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)

I would not expect this. I agree, current implementation is to return as soon as a hit occurs.
Speed could be a real issue here, if we are working with a large bibfile.

Yeah. I was just curious. For training the usage of performance.

I fully agree that the change has to be reverted.

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. 😅

@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)

@k3KAW8Pnf7mkmdSMPHz27 Just for information: crossref is looked at by a student. Hope, it is manageable ^^

@koppor Thanks so much. 👍

in the stream version, ALL elements are mapped and then filtered, then mapped and then the first is taken

Intermediate operations should be lazily evaluated

Laziness-seeking. Many stream operations, such as filtering, mapping, or duplicate removal, can be implemented lazily, exposing opportunities for optimization. For example, "find the first String with three consecutive vowels" need not examine all the input strings. Stream operations are divided into intermediate (Stream-producing) operations and terminal (value- or side-effect-producing) operations. Intermediate operations are always lazy.

I would use a flatMap rather than two filtering operations, but I don't think the performance improvement is significant.

I bet that the current for loop is faster - or does the compiler optimize the stream that much so that it is equal?

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.

@k3KAW8Pnf7mkmdSMPHz27 Just for information: crossref is looked at by a student. Hope, it is manageable ^^

@koppor thank you and that is awesome! This really needs to get done. @Toro520 I as well appreciate you taking a look at it!

koppor

Just some small comments.

koppor · 2023-10-25T12:51:22Z

src/main/java/org/jabref/model/entry/BibEntry.java

    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();
    }


@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)

koppor · 2023-10-25T12:51:43Z

src/main/java/org/jabref/model/entry/BibEntry.java

+    /**
+     * 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.
+     */


Thank you for the JavaDoc 🤩

koppor · 2023-10-25T12:52:21Z

src/main/java/org/jabref/model/entry/BibEntry.java

+        } catch (
+                IllegalArgumentException e) {


@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.

…references to the current entry and update the relevant cells. 2. Added the addFileFromCrossref(LinkedFile file) method to add files from cross-referenced entries and update the file list. 3. Modified the removeCrossref() method to ensure that the relevant cells are updated when removing cross-references. 4. Added the updateCitationKey(String newCitationKey) method to update the citation key of cross-referenced entries and update the relevant cells. 5. Added the removeCrossrefedEntry(BibEntry crossrefedEntry) method to remove cross-referenced entries and update the relevant cells. 6. Added the updateCell() method to update the data in the cells. The specific implementation should be tailored to your code structure and requirements. 7. Added the updateFiles(List<LinkedFile> linkedFiles) method to update the file list. The specific implementation should be tailored to your code structure and requirements.

Siedlerchr · 2023-10-29T08:25:51Z

build.gradle

@@ -147,7 +147,7 @@ dependencies {

    implementation 'com.fasterxml:aalto-xml:1.3.2'

-    implementation group: 'org.mariadb.jdbc', name: 'mariadb-java-client', version: '2.7.9'
+    implementation group: 'org.mariadb.jdbc', name: 'mariadb-java-client', version: '3.1.2'


Please do not modify the gradle file!

koppor · 2024-03-11T23:26:23Z

Follow-up #10599

Enhancement :

fe185ee

Refactor getResolvedFieldOrAlias for clarity and modern style - Use Stream API for better readability and concise code. - Simplify the logic to fetch the first resolved field or alias. - No change in the method's functionality or output.

calixtus reviewed Oct 25, 2023

View reviewed changes

koppor reviewed Oct 25, 2023

View reviewed changes

koppor mentioned this pull request Oct 26, 2023

Switch code style to Google's Java Code Style JabRef/jabref-koppor#661

Open

7 tasks

ThiloteE changed the title ~~Enhancement : The first trying of pull request to enhance code quality in issue #9717~~ Display files from referenced crossref in entry table (Toro520 version) Oct 28, 2023

ThiloteE added ui maintable external files labels Oct 28, 2023

koppor mentioned this pull request Oct 28, 2023

Switch to palantir-java-format JabRef/jabref-koppor#663

Closed

9 tasks

Siedlerchr reviewed Oct 29, 2023

View reviewed changes

koppor mentioned this pull request Nov 17, 2023

Use Prettier Java for automatic code formatting #10646

Closed

6 tasks

calixtus closed this Mar 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display files from referenced crossref in entry table (Toro520 version) #10577

Display files from referenced crossref in entry table (Toro520 version) #10577

Toro520 commented Oct 25, 2023 •

edited by ThiloteE

Loading

calixtus Oct 25, 2023

koppor Oct 25, 2023

Toro520 Oct 25, 2023

Toro520 Oct 25, 2023

calixtus Oct 25, 2023

koppor Oct 25, 2023

Toro520 Oct 25, 2023

koppor Oct 26, 2023

calixtus Oct 25, 2023

Siedlerchr Oct 25, 2023

koppor Oct 25, 2023

calixtus Oct 25, 2023 •

edited

Loading

koppor Oct 25, 2023

Toro520 Oct 25, 2023

koppor Oct 26, 2023

koppor Oct 26, 2023

Toro520 Oct 26, 2023

k3KAW8Pnf7mkmdSMPHz27 Oct 29, 2023 •

edited

Loading

koppor left a comment

koppor Oct 25, 2023

koppor Oct 25, 2023

koppor Oct 25, 2023

Siedlerchr Oct 29, 2023

koppor commented Mar 11, 2024

Display files from referenced crossref in entry table (Toro520 version) #10577

Display files from referenced crossref in entry table (Toro520 version) #10577

Conversation

Toro520 commented Oct 25, 2023 • edited by ThiloteE Loading

Mandatory checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixtus Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k3KAW8Pnf7mkmdSMPHz27 Oct 29, 2023 • edited Loading

Choose a reason for hiding this comment

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Mar 11, 2024

Toro520 commented Oct 25, 2023 •

edited by ThiloteE

Loading

calixtus Oct 25, 2023 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 Oct 29, 2023 •

edited

Loading