Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement journal abbreviation storage in separate directory (Closes #10557) #12036

Closed
wants to merge 8 commits into from
Closed

Implement journal abbreviation storage in separate directory (Closes #10557) #12036

wants to merge 8 commits into from

Conversation

Daniel-Ruan
Copy link

"Closes #10557".

Description

This pull request addresses issue #10557 by implementing two features to store journal abbreviation lists in a separate directory. The main goal is to improve the organization and management of journal abbreviations in JabRef.

Changes made:

  1. Implemented a new default directory for journal abbreviation lists:

    • On Windows: %APPDATA%\..\local\org.jabref\jabref\journal-abbreviations
    • Similar paths for other operating systems

  2. Added a preference option for users to change the journal abbreviation directory.

  3. Implemented automatic creation of custom.csv in the journal abbreviation directory if it doesn't exist.

  4. Updated the relevant logic classes:

    • Added new properties and methods to handle the journal abbreviation directory

  5. Updated related components:

    • Modified the preferences dialog to include the new journal abbreviation directory setting
    • Updated the journal abbreviation management UI to reflect the new directory structure

  6. Implemented automatic detection of journal lists in the specified directory:

    • Added logic to scan for .csv files in the directory
    • Prepared groundwork for future implementation of .csv to .mv store conversion (not implemented in this PR)
    • Update the journal abbreviation management UI to reflect the new directory structure

TODO (Not implemented in this PR):

Implement automatic detection of journal lists in the specified directory:

  • Add logic to scan for .csv files in the directory

  • Prepare groundwork for future implementation of .csv to .mv store conversion

  • Implemented logic to ensure the directory always returns a valid path

UI Changes

We have attached three screenshots showing the updated UI for journal abbreviations in the preferences dialog:

[Attach your three screenshots here]

  1. Screenshot 1: Overview of the journal abbreviations preference page
1
  1. Screenshot 2: Switch to the default custom CSV
2
  1. Screenshot 3: List of detected journal abbreviation files in the new directory
3

Testing

  • Extensive manual testing has been performed to verify the functionality of the new journal abbreviation directory feature.
  • Unit tests have been added for the JournalAbbreviationPreferences class to ensure proper handling of the new directory setting.
  • During units testing, we encountered bugs related to the newly introduced directory variable:
    • In some test scenarios, attempts to get the directory path resulted in null values.
    • We are still working on a robust solution to handle these null cases consistently.

Request for Review Guidance

We would greatly appreciate any suggestions or guidance from the reviewers on how to effectively handle potential null values when retrieving the journal abbreviation directory path. Specifically:

  • Are there best practices within the JabRef codebase for handling potentially null file paths?
  • Should we implement additional null checks, or is there a preferred way to ensure the directory is always initialized?
  • Are there any specific test scenarios or edge cases we should consider to improve the robustness of our implementation?

Your insights on these matters would be invaluable in helping us refine our approach and ensure the reliability of this new feature.

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

@Daniel-Ruan
Store journal lists in separate directory (JabRef#10557)
3c52c60 
- Implement storage of journal abbreviation lists in a dedicated directory
- Default directory: '%APPDATA%\..\local\org.jabref\jabref\journal-abbreviations' (Windows)
- Add user preference to change the directory
- Create 'custom.csv' if it doesn't exist in the specified directory
- Update journal abbreviation management to use the new directory structure
- Refactor related code for better maintainability and user experience
- Update documentation to reflect new journal abbreviation storage method

This change improves organization of journal abbreviation lists and
provides a more intuitive way for users to manage custom abbreviations.
It also ensures that JabRef always has a .csv file available for users
to add new abbreviations.
@Daniel-Ruan
Update CHANGELOG.md: Improve journal abbreviation management (JabRef#…
3e5b00b 
…10557)

- Add feature for changing abbreviation storage directory
- Fix automatic creation and addition of custom.csv
- Update description of abbreviation list management

Reflects improvements in journal abbreviation handling, including
new directory preferences and custom.csv file management.
@Daniel-Ruan
Refactor: Standardize Path creation and improve import order
0c623c2 
- Replace Paths.get() with Path.of() for better Java 11+ compatibility
- Reorganize import statements to follow project conventions
- Remove unused imports
- Update JournalAbbreviationPreferences to use Path.of()
- Ensure consistent use of Path creation methods across the project

This commit addresses issues raised by MainArchitectureTest and
improves overall code quality and consistency. It also helps in
maintaining a uniform coding standard throughout the project.
@Daniel-Ruan
Refine JournalAbbreviationPreferences for robustness (JabRef#10557)
977f9a4 
- Enhance null handling in constructor and setters
- Implement Optional usage in getJournalAbbreviationDir method
- Add null checks in updateCustomCsvFile method
- Improve error logging in getDefaultAbbreviationDir
- Ensure default directory is always used when input is null

These changes address edge cases and potential null pointer issues,
improving the overall stability of journal abbreviation handling.
The modifications should resolve test failures related to
NullPointerExceptions in JournalAbbreviationsViewModelTabTest.
@Daniel-Ruan
Prevent NullPointerException in JournalAbbreviationPreferences (JabRe…
302029a 
…f#10557)

Enhanced the robustness of the JournalAbbreviationPreferences class by ensuring that the getJournalAbbreviationDir method never returns null. Added null checks and improved the default directory resolution logic to handle cases where system environment variables might not be set. Also included logging to aid in debugging potential issues with directory paths.

- Ensured getJournalAbbreviationDir always returns a non-null path.
- Improved default path generation in getDefaultAbbreviationDir to create missing directories.
- Added logs for better tracking and diagnosing issues related to path resolution.
@Daniel-Ruan
Implement centralized journal abbreviation storage (JabRef#10557)
1ee90d1 
- Add support for a single directory to store all journal abbreviation lists
- Set default directory to %APPDATA%\..\local\org.jabref\jabref\journal-abbreviations
- Implement user option to change directory via preferences dialog
- Add automatic creation of custom.csv if it doesn't exist
- Refactor JournalAbbreviationPreferences for improved robustness
- Update JournalAbbreviationsTabViewModel to support new storage system
- Enhance error handling and logging in journal abbreviation management
- Remove unused import in JournalAbbreviationPreferences.java

This commit addresses the need for a more organized and user-friendly
approach to managing journal abbreviation lists. It simplifies the
process for users and ensures that JabRef always has a .csv file
available for adding new abbreviations.

The changes improve code quality and error handling, while also
fixing a Checkstyle issue related to an unused import.

TODO: Implement automatic .csv to .mv file conversion and update UI
to display detected lists in future enhancements.
@Daniel-Ruan
Improve null safety in journal abbreviation directory handling (JabRe…
5401d04 
…f#10557)

- Refactor getJournalAbbreviationDir() to use Optional
- Add fallback to system temp directory if default is unavailable
- Enhance error logging for directory creation failures

This commit improves the robustness of journal abbreviation directory
handling by utilizing Optional to avoid null pointer exceptions and
implementing a reliable fallback mechanism.
@Daniel-Ruan
Fix NullPointerException in getDefaultAbbreviationDir() method (JabRe…
0498c42 
…f#10557)

- Ensured getDefaultAbbreviationDir() never returns null by providing a robust default directory path. This fix prevents NullPointerException in unit tests where the method's output is directly converted to a string without null checks.
Siedlerchr
Siedlerchr reviewed Oct 20, 2024
View reviewed changes
CHANGELOG.md

### Fixed

- We fixed an issue where certain actions were not disabled when no libraries were open. [#11923](https://github.com/JabRef/jabref/issues/11923)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the changes with the spaces here

Siedlerchr
Siedlerchr requested changes Oct 20, 2024
View reviewed changes
src/main/java/org/jabref/logic/journals/JournalAbbreviationPreferences.java
}

public static Path getDefaultAbbreviationDir() {
String appData = System.getenv("APPDATA");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have better methods for getting the AppDAta dir, see Directories class, please use that approach

@Siedlerchr Siedlerchr added the status: changes required Pull requests that are not yet complete label Oct 20, 2024
Daniel-Ruan
Daniel-Ruan commented Oct 24, 2024
View reviewed changes
Copy link
Author

@Daniel-Ruan Daniel-Ruan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try to review

@koppor
Copy link
Member

koppor commented Oct 24, 2024

try to review

Who? The JabRef team already reviewed. After the fixes, we review again 😅

@koppor
Copy link
Member

koppor commented Dec 9, 2024

Closing this issue due to inactivity 💤

Please ping us if you intend to resume work on this one.

@koppor koppor closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Siedlerchr Siedlerchr Siedlerchr requested changes

Assignees
No one assigned
Labels
status: changes required Pull requests that are not yet complete
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Store journal lists in separate directory
3 participants
@Daniel-Ruan @koppor @Siedlerchr