-
-
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
Fix for issue 571: File folders: Move between global and user-specific #12059
base: main
Are you sure you want to change the base?
Conversation
@Officialshubham Please revert updates in `csl-styles. See https://devdocs.jabref.org/code-howtos/faq.html#fix. -- I checked the GitHub Diff (tab "Files") and it still shows changes there. |
Co-authored-by: Oliver Kopp <[email protected]>
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.
Some general remark.
GitHub removed my text, here it is: The class Add another method to
It returns a (See #12037 for the PR renaming the variables properly) Then, your method The code of the handleXcase will be more readable. Also it will be one method. You can ask |
Thanks @koppor |
String user = preferences.getFilePreferences().getUserAndHost().split("-")[0]; | ||
|
||
// Create a set of root-like directories and include the user dynamically | ||
Set<String> rootLikeDirs = new HashSet<>(Set.of( |
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.
We are recreating the same set each time we check if the directory is root. This doesn't sound efficient.
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.
Yes it is not correct. I am moving it as static and final. It will be loaded once now.
BooleanProperty isMoveFileDisabled = new SimpleBooleanProperty(true); | ||
moveFileItem.disableProperty().bind(isMoveFileDisabled); | ||
moveFileItem.setText(Localization.lang("Move file")); |
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.
Why are we binding to isMoveFileDisabled here? It is declared locally so its value (true) won't change.
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.
Hi @HoussemNasri
I bind it to follow the consistency in both the classes: LinkedFilesEditor and LinkedFileViewModel
I am aware that we can disable it directly using setDisable().
I will change it. Let me know your views. Thanks
@koppor and @HoussemNasri. |
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.
an old review comment was not posted - it is still valid.
Pleae also fix buildres/abbrv.jabref.org
} | ||
} | ||
|
||
public void moveToDirectory(MenuItem moveFileItem, MenuItem moveAndRenameFileItem, boolean toRename) { |
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.
I think, a similar function already exists at org.jabref.logic.externalfiles.LinkedFileHandler#copyOrMoveToDefaultDirectory
. One "just" needs to make it more generic to get the target directory (instead of databaseContext.getFirstExistingFileDir(filePreferences);
).
With the record I proposed, this should be very easy.
@@ -325,68 +371,96 @@ private ContextMenu createContextMenuForFile(LinkedFileViewModel linkedFile) { | |||
factory.createMenuItem(StandardActions.OPEN_FOLDER, new ContextAction(StandardActions.OPEN_FOLDER, linkedFile, preferences)), | |||
new SeparatorMenuItem(), | |||
factory.createMenuItem(StandardActions.DOWNLOAD_FILE, new ContextAction(StandardActions.DOWNLOAD_FILE, linkedFile, preferences)), | |||
moveFileItems[0], |
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.
No - not any arrays in Java.
Describe the changes you have made here:
Created a separate class to handle movement of files between directories. The class follows the structure and logic mention in issue #571
Class Location: org.jabref/gui/fieldeditors
Class Name: FileDirectoryHandler
Class Description: Primary objective of this class to give target directory based on the current location/path of the file.
Changes in LinkedFilesEditor.java
Changes in LinkedFileViewModel.java
Add some tests to check the functionality of FileDirectoryHandler
Closes koppor#571
Special Note: Want to confirm whether going in right direction or not. Unit Testing is in process.
Manual testing is performed. Creating a draft PR for review.
Screenshots
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)