Store journal abbreviation lists in specified directory #12544

adhamahmad · 2025-02-23T02:52:37Z

UI changes:

Mandatory checks

I own the copyright of the code submitted and I licence it under the MIT license
Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
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.

…tom.csv exists

github-actions

Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues.
In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.

koppor · 2025-02-26T11:03:55Z

Thank you for working on this.

TODO (Not implemented in this PR):

What does this mean?

Will there be follow-up PRs? Do you intent to work on this in the context of commits to this PR?

adhamahmad · 2025-02-26T14:44:40Z

@koppor

I intend to include them as additional commits within this PR.

koppor · 2025-02-26T15:15:56Z

I intend to include them as additional commits within this PR.

Nice! 👍

koppor · 2025-02-27T11:19:57Z

* Implementation of .csv to .mv store conversion

Code hint: This is already implemented at org.jabref.cli.JournalListMvGenerator. One just needs to move that class to org.jabref.logic.journals and adapt it.

koppor · 2025-02-27T11:22:24Z

* Implement automatic detection of journal lists in the specified directory:

This could be easy.

Have a HashMap (stored inMVStore) from filename to timestamp
Files.walktree -> for each file: look up. if not found: import and put timestamp of file. If found: compare timestamp. If not equal: clear current hashmap for that. import and update time stamp

…version - Automatically detect journal abbreviation lists in the specified directory - Convert .csv files to .mv store format if the CSV timestamp is newer - Maintain a HashMap in MVStore to track file timestamps and update accordingly - Ensure JabRef always has a custom.csv available for user-defined abbreviations - Improve journal abbreviation loading and preferences handling

src/main/java/org/jabref/logic/journals/JournalAbbreviationPreferences.java

koppor · 2025-03-16T22:07:32Z

@adhamahmad I think, the TODO list in the PR description should be updated if I read your last commit comment right?

adhamahmad · 2025-03-17T00:51:19Z

@adhamahmad I think, the TODO list in the PR description should be updated if I read your last commit comment right?

You're right—the .csv to .mv conversion is now implemented. I updated the TODO list, the only missing part is the unit test.

koppor · 2025-03-17T08:09:21Z

This PR might have conflicts with #12606 - They should be combined at some point.

Depening on which PR gets in first, the other one needs to be adapted.

koppor · 2025-03-17T08:16:03Z

src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTabViewModel.java

+                if (filePath.endsWith(".mv")) {
+                    openMvFile(path);
+                } else {
+                    openFile(path);
+                }


I think, the logic is wrong.

Think of .mv as cache.

If .csv is older than .mv: Use .mv

Else: Create .mv from csv

I reviewed the logic and found that the else branch (handling CSV files) is not needed.

By the time updateJournalFiles() is executed, the conversion process in JournalAbbreviationMvGenerator.convertAllCsvToMv() has already performed a timestamp check on all CSV files.

This ensures that any CSV file in the new directory has been converted to an MV file.

…nalFiles()

trag-bot · 2025-03-17T19:26:50Z

src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTabViewModel.java

+            if (name.endsWith(".mv")) {
+                openMvFile(filePath);
+            } else { // .csv file
+                openFile(filePath);
+            }


Magic string ".mv" should be defined as a constant. String literals used for file extension checking should be extracted to named constants for better maintainability.

trag-bot · 2025-03-17T20:56:59Z

src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java

@@ -58,13 +59,27 @@ public void readAbbreviations() throws IOException {
        }
    }

+    public void readAbbreviationsFromMv() throws IOException {


Method lacks JavaDoc despite being a complex method that handles file loading and data transformation. The method's purpose and behavior should be documented.

trag-bot · 2025-03-17T20:59:25Z

src/main/java/org/jabref/logic/journals/JournalAbbreviationMvGenerator.java

+            fullToAbbreviation.putAll(abbreviationMap);
+            store.commit();
+            LOGGER.info("Saved MV file: {}", mvFile.getFileName());
+        } catch (IOException e) {


The method declares throws IOException but also catches it internally, which creates confusion about exception handling responsibility. Should either handle or propagate.

trag-bot · 2025-03-17T20:59:45Z

src/main/java/org/jabref/logic/journals/JournalAbbreviationPreferences.java

+        if (journalsDir == null) {
+            String defaultDir = Directories.getJournalAbbreviationsDirectory().toString();
+            setJournalAbbreviationDir(defaultDir);
+            return defaultDir;
+        }


The code follows nested logic instead of failing fast. The condition should be inverted to return early, following the fail-fast principle.

src/main/java/org/jabref/logic/util/Directories.java

trag-bot · 2025-03-17T21:01:32Z

src/test/java/org/jabref/logic/journals/JournalAbbreviationMvGeneratorTest.java

+        JournalAbbreviationMvGenerator.convertCsvToMv(csvFile, mvFile);
+
+        // Verify the MV file is created
+        assertTrue(Files.exists(mvFile), "MV file should be created after conversion");


Assertion message should be removed as the method name already conveys the expected behavior. The test method name clearly indicates what is being tested.

trag-bot · 2025-03-17T21:01:37Z

src/test/java/org/jabref/logic/journals/JournalAbbreviationMvGeneratorTest.java

+        // Load abbreviations from the MV file
+        Collection<Abbreviation> abbreviations = JournalAbbreviationMvGenerator.loadAbbreviationsFromMv(mvFile);
+        // Expecting 2 abbreviations as in the CSV file
+        assertEquals(2, abbreviations.size(), "MV file should contain 2 abbreviations");


Assertion message should be removed as the method name already conveys the expected behavior. The test method name clearly indicates what is being tested.

trag-bot · 2025-03-18T02:07:26Z

@trag-bot didn't find any issues in the code! ✅✨

adhamahmad added 2 commits February 23, 2025 04:25

feat: Allow user to set journal abbreviation directory and ensure cus…

b2dc993

…tom.csv exists

removed extra spaces

53ac1f7

github-actions bot reviewed Feb 23, 2025

View reviewed changes

adhamahmad and others added 2 commits February 23, 2025 05:25

removed extra spaces

1eb3eb0

Merge branch 'main' into fix-for-issue-10557

628a648

adhamahmad marked this pull request as ready for review February 23, 2025 03:47

adhamahmad marked this pull request as draft February 23, 2025 03:47

ThiloteE changed the title ~~Fix for issue 10557~~ Store journal abbreviation lists in specified directory Feb 24, 2025

Merge branch 'JabRef:main' into fix-for-issue-10557

c8d0eaf

adhamahmad and others added 6 commits March 16, 2025 04:40

Merge branch 'main' into fix-for-issue-10557

b926f72

Apply OpenRewrite fixes to follow JabRef coding guidelines

b54cb36

Add detailed JavaDoc for initializeDirectory method

7fa15b1

Refactor: Extract magic strings as constants for maintainability

4b57648

Refactor: removed trivial comment

0ce80ba

trag-bot bot reviewed Mar 16, 2025

View reviewed changes

src/main/java/org/jabref/logic/journals/JournalAbbreviationPreferences.java Outdated Show resolved Hide resolved

trag-bot bot reviewed Mar 16, 2025

View reviewed changes

src/main/java/org/jabref/logic/journals/JournalAbbreviationPreferences.java Outdated Show resolved Hide resolved

adhamahmad and others added 2 commits March 16, 2025 19:44

Merge branch 'main' into fix-for-issue-10557

29eaf82

Regenerate missing .mv files even if CSV timestamps are unchanged

ffd98f7

koppor mentioned this pull request Mar 16, 2025

Add automatic message on submodule modifications #12752

Merged

7 tasks

Created Unit tests for CsvToMv conversion

fdf074f

adhamahmad changed the title ~~Store journal abbreviation lists in specified directory~~ Closes #10557. Store journal abbreviation lists in specified directory Mar 17, 2025

adhamahmad changed the title ~~Closes #10557. Store journal abbreviation lists in specified directory~~ Closes #10557 Store journal abbreviation lists in specified directory Mar 17, 2025

adhamahmad changed the title ~~Closes #10557 Store journal abbreviation lists in specified directory~~ Store journal abbreviation lists in specified directory Mar 17, 2025

koppor mentioned this pull request Mar 17, 2025

Refactor JournalAbbreviationRepository class to use on-disk maps #12606

Draft

7 tasks

koppor requested changes Mar 17, 2025

View reviewed changes

Removed .csv branch from JournalAbbreviationsTabViewModel.updtaeJour…

b8ee7ef

…nalFiles()

trag-bot bot reviewed Mar 17, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into fix-for-issue-10557

700f63c

adhamahmad requested a review from koppor March 17, 2025 20:54

adhamahmad marked this pull request as ready for review March 17, 2025 20:55

trag-bot bot reviewed Mar 17, 2025

View reviewed changes

src/main/java/org/jabref/logic/util/Directories.java Show resolved Hide resolved

trag-bot bot reviewed Mar 17, 2025

View reviewed changes

adhamahmad added 2 commits March 17, 2025 23:27

Merge branch 'main' into fix-for-issue-10557

1b96435

Merge branch 'main' into fix-for-issue-10557

07acaff

koppor marked this pull request as draft March 19, 2025 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store journal abbreviation lists in specified directory #12544

Store journal abbreviation lists in specified directory #12544

adhamahmad commented Feb 23, 2025 •

edited

Loading

github-actions bot left a comment

koppor commented Feb 26, 2025

TODO (Not implemented in this PR):

adhamahmad commented Feb 26, 2025

koppor commented Feb 26, 2025

koppor commented Feb 27, 2025

koppor commented Feb 27, 2025

koppor commented Mar 16, 2025

adhamahmad commented Mar 17, 2025

koppor commented Mar 17, 2025

koppor Mar 17, 2025

adhamahmad Mar 17, 2025 •

edited

Loading

trag-bot bot Mar 17, 2025

trag-bot bot Mar 17, 2025

trag-bot bot Mar 17, 2025

trag-bot bot Mar 17, 2025

trag-bot bot Mar 17, 2025

trag-bot bot Mar 17, 2025

trag-bot bot commented Mar 18, 2025

Store journal abbreviation lists in specified directory #12544

Are you sure you want to change the base?

Store journal abbreviation lists in specified directory #12544

Conversation

adhamahmad commented Feb 23, 2025 • edited Loading

UI changes:

Mandatory checks

github-actions bot left a comment

Choose a reason for hiding this comment

koppor commented Feb 26, 2025

TODO (Not implemented in this PR):

adhamahmad commented Feb 26, 2025

koppor commented Feb 26, 2025

koppor commented Feb 27, 2025

koppor commented Feb 27, 2025

koppor commented Mar 16, 2025

adhamahmad commented Mar 17, 2025

koppor commented Mar 17, 2025

koppor Mar 17, 2025

Choose a reason for hiding this comment

adhamahmad Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

trag-bot bot Mar 17, 2025

Choose a reason for hiding this comment

trag-bot bot Mar 17, 2025

Choose a reason for hiding this comment

trag-bot bot Mar 17, 2025

Choose a reason for hiding this comment

trag-bot bot Mar 17, 2025

Choose a reason for hiding this comment

trag-bot bot Mar 17, 2025

Choose a reason for hiding this comment

trag-bot bot Mar 17, 2025

Choose a reason for hiding this comment

trag-bot bot commented Mar 18, 2025

adhamahmad commented Feb 23, 2025 •

edited

Loading

adhamahmad Mar 17, 2025 •

edited

Loading