Enhancement: Move linked file to folder offers all JabRef Directories as options#15055
Enhancement: Move linked file to folder offers all JabRef Directories as options#15055ganesh-vk wants to merge 45 commits into
Conversation
…ady in a Jabref Directory File is moved to the next available file directory that is configured. Available directorie's are searched in the order User -> Library -> BIB git commit -m
ganesh-vk
left a comment
ganesh-vk
left a comment
There was a problem hiding this comment.
Let me know what I can do here
| assertTrue(action.isExecutable(), "DOWNLOAD_FILE should be executable for online link"); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
This test uses the earlier implementation (which found the first configured directory in User > Library > Bib). Test fails with the new implementation so I deleted it. The UI now does not get greyed out since the check is no longer present. However the error dialog box provides specific reasons for failure instead of something generic.
github-actions
Bot
added
status: changes-required
|
I've looked into this a little more. The text for each action is set once at build time and is not changed dynamically. Since a user can select multiple files at once and then attempt to move them all at once and since it's possible that the files can be in different directories, I think the generic message is the solution that we should use. If two files are linked to the same entry (one present in say the User specific directory and the other in the Library specific directory) and a user selects both and opens the menu, we cannot mention a singular directory to which both files shall be moved to since they are different. |
Review Summary by QodoImplement file rotation across configured directories
WalkthroughsDescription• Implement file rotation across configured directories - Files now move to next available directory (User → Library → BIB) - Skips null/unconfigured directories in rotation cycle - Preserves directory structure when moving between configured directories • Update UI text to clarify "next configured directory" behavior • Add comprehensive test coverage for directory rotation logic • Refactor LinkedFileHandler to support arbitrary target directories Diagramflowchart LR
A["File in User Dir"] -->|moveToNextPossibleDirectory| B["Determine Current Dir Index"]
B -->|Find Next Valid Dir| C["Skip Null Directories"]
C -->|Rotate Cycle| D["Move to Next Dir"]
D -->|Preserve Structure| E["File in Library/BIB Dir"]
File Changes1. jabgui/src/main/java/org/jabref/gui/actions/StandardActions.java
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
…o move-linked-files-directory
github-actions
Bot
added
the
status: changes-required
github-actions
Bot
removed
the
status: changes-required
| MOVE_FILE_TO_FOLDER_AND_RENAME, | ||
| OPEN_FILE, | ||
| OPEN_FOLDER, | ||
| RENAME_FILE_TO_NAME, |
There was a problem hiding this comment.
Keep the binding with !linkedFile.isGeneratedPathSameAsOriginal(), for this case.
There was a problem hiding this comment.
I'm not sure if you're talking about MOVE_FILE_TO_FOLDER_AND_RENAME but if you are, the method uses the new implementation underneath (moveToNextPossibleDirectory) so it faces the same problem.
There was a problem hiding this comment.
A GitHub comment is directly below the line
I commented to RENAME_FILE_TO_NAME
| /// The returned list has a fixed size and preserves positional meaning. | ||
| /// | ||
| /// The directories are returned in the following order: | ||
| /// <ol> |
There was a problem hiding this comment.
What is this? Do you know Markdown?
Ah, you copied existing code without linking the original code - and not fixing the Markdown of the original code.
There was a problem hiding this comment.
After the update, you need to update getFileDirectories to use the other method and add the directories if they are available.
Note that the record will have three entries only.
- userFileDirectory
- librarySpecificFileDirectory
- fallbackDirectory
There was a problem hiding this comment.
I did not know how ordered lists are implemented in markdown but the unordered points rendered in a readable manner so I did not bother looking into it. Fixed it now :)
| } | ||
|
|
||
| /// Look up all configured file directories of this database. | ||
| /// The returned list has a fixed size and preserves positional meaning. |
There was a problem hiding this comment.
Wrong data structure!
Use a @NullMarked record with Optional<Path> to be consistent with the other code.
There was a problem hiding this comment.
Switched to a record
github-actions
Bot
added
the
status: changes-required
…and use new implementation in getFileDirectories. Fix markdown comment.
|
I've changed it to explicitly mention all JabRef directories as possible options. They're disabled if not configured or if the file is already present in the same directory. It does not mirror file directory structure anymore either. Single File: Multiple Files: |
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
…o move-linked-files-directory
|
PR title needs to be updated - "next possible directory" was not asked for. |
ganesh-vk
changed the title
| SimpleCommand command = new SimpleCommand() { | ||
| { | ||
| setExecutable(isMenuItemExecutable); | ||
| } |
There was a problem hiding this comment.
Double braces initialization is an antipattern.
https://blog.jooq.org/dont-be-clever-the-double-curly-braces-anti-pattern/
There was a problem hiding this comment.
Thanks for sharing, will read, changed
| SimpleCommand command = new SimpleCommand() { | ||
| { | ||
| setExecutable(menuItemExecutable); | ||
| } |
github-actions
Bot
added
status: changes-required
| Menu moveSubmenu = contextMenu.getItems().stream() | ||
| .filter(Menu.class::isInstance) | ||
| .map(Menu.class::cast) | ||
| .filter(menu -> menu.getText().contains("Move file")) | ||
| .findFirst() | ||
| .orElseThrow(); | ||
|
|
||
| assertEquals(4, moveSubmenu.getItems().size()); |
There was a problem hiding this comment.
Isnt that always 4?
Please make sure we are testing real jabref logic, not the framework
There was a problem hiding this comment.
The other tests here were also testing to ensure that the all the Menu's were built correctly, so I kept it. I've removed it now
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
| selectedFiles | ||
| )); | ||
| menu.getItems().add(createItem( | ||
| Localization.lang("Next to library"), |
There was a problem hiding this comment.
| Localization.lang("Next to library"), | |
| Localization.lang("Next to library file"), |
| Optional<Path> librarySpecificFileDirectory = metaData.getLibrarySpecificFileDirectory().map(this::getFileDirectoryPath); | ||
| librarySpecificFileDirectory.ifPresent(fileDirs::add); | ||
|
|
||
| // fileDirs.isEmpty() is true after these two if there are no directories set in the BIB file itself: |
|
|
||
| /// Holds the configured file directories in a fixed order. | ||
| @NullMarked | ||
| public record FileDirectories(Optional<Path> userDirectory, |
There was a problem hiding this comment.
Optionally should ideally not be a parameter , it is a a return type
There was a problem hiding this comment.
Changed it to use @Nullable Path with getters that return Optionals
5 participants
Closes #12287
Steps to test
Move file(s) to the next configured directoryto move the file to the next configured directory.Opened as a draft since the following needs discussion:
Move file(s) to the next configured directory. I think this is still an improvement compared to the current status quo but the issue wanted it to mention the directory its going to be moved to. I couldn't figure out how to implement this cleanly so some hints would be appreciated or the current implementation can be accepted.AI usage disclosure: Used Deepwiki to better understand the issue and discussed with ChatGPT/Gemini when necessary. All usage was in accordance with the AI usage policy.
Checklist
CHANGELOG.mdin a way that is understandable for the average user (if change is visible to the user)