Add context menu for multi-file entries (#12567) #13726
Conversation
Extend the Entry Editor context menu to handle entries with multiple linked files. This improves UX when managing more than one file per entry and aligns behavior across single- and multi-file scenarios. Changes: - Add plural actions to StandardActions enum - Rewrite ContextAction.execute() for multi-file cases - Extend ContextMenuFactory to build multi-file items - Rework MultiContextAction to operate on selections - Introduce CopyMultipleFilesAction (new class) - Update/add localization keys; tests pass - Add unit tests: ContextActionTest, ContextMenuFactoryTest, MultiContextActionTest, CopyMultipleFilesActionTest - Guard LinkedFileViewModel to avoid a JavaFX crash Fixes JabRef#12567 Keywords: context menu, linked files, multi-selection, UI
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
jabgui/src/main/java/org/jabref/gui/copyfiles/CopyMultipleFilesAction.java
Outdated
Show resolved
Hide resolved
jabgui/src/test/java/org/jabref/gui/fieldeditors/contextmenu/MultiContextActionTest.java
Outdated
Show resolved
Hide resolved
jabgui/src/test/java/org/jabref/gui/fieldeditors/contextmenu/MultiContextActionTest.java
Outdated
Show resolved
Hide resolved
i set up checkstyle with your guide but it doesn`t show me some problems in test classes, only in main module)
|
jabgui/src/main/java/org/jabref/gui/copyfiles/CopyMultipleFilesAction.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/copyfiles/CopyMultipleFilesAction.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/fieldeditors/contextmenu/MultiContextAction.java
Outdated
Show resolved
Hide resolved
|
Hi @koppor, all checks have passed ✅ Could you please review and approve when you have time? |
Check this setting on the right (screenshot taken from here) |
thank you anyway, but all check are done succesefully, so i wait for review |
jabgui/src/main/java/org/jabref/gui/fieldeditors/contextmenu/MultiContextAction.java
Outdated
Show resolved
Hide resolved
|
|
||
| OPEN_DATABASE_FOLDER(Localization.lang("Reveal in file explorer")), | ||
| OPEN_FOLDER(Localization.lang("Open folder"), Localization.lang("Open folder"), IconTheme.JabRefIcons.FOLDER, KeyBinding.OPEN_FOLDER), | ||
| OPEN_FOLDERS(Localization.lang("Open folders"), Localization.lang("Open folders"), IconTheme.JabRefIcons.FOLDER, KeyBinding.OPEN_FOLDER), |
There was a problem hiding this comment.
The task description suggests renaming the commands instead of adding new ones.
Adding new StandardActions just with a little nuance in the name is increasing maintenance debt with no real benefit for the user. Please use the existing commands, just with different behaviour for multiple files.
There was a problem hiding this comment.
I deleted additional commands, using only existing (except REMOVE_LINKS, it was implemented not by me so decided to let it go), fixed enum and localisation, implemented with Strategy pattern also
jabgui/src/main/java/org/jabref/gui/copyfiles/CopyMultipleFilesAction.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/fieldeditors/contextmenu/MultiContextAction.java
Outdated
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/fieldeditors/contextmenu/MultiContextAction.java
Outdated
Show resolved
Hide resolved
jabgui/src/test/java/org/jabref/gui/copyfiles/CopyMultipleFilesActionTest.java
Outdated
Show resolved
Hide resolved
jabgui/src/test/java/org/jabref/gui/copyfiles/CopyMultipleFilesActionTest.java
Outdated
Show resolved
Hide resolved
jabgui/src/test/java/org/jabref/gui/fieldeditors/contextmenu/ContextMenuFactoryTest.java
Outdated
Show resolved
Hide resolved
|
I mean reuse existing commands with changed text: eg 'Download file(s)' |
|
Thx for your feedback, i will research the problems you marked and try to solve, if it`s possible, thx a lot |
…sActionTest.java Co-authored-by: Carl Christian Snethlage <[email protected]>
…menu - Introduce ContextMenuBuilder + SingleSelectionMenuBuilder + MultiSelectionMenuBuilder - Extract shared checks/openContainingFolders into SelectionChecks - ContextMenuFactory delegates to strategies; no more branching by selection size - LinkedFilesEditor initializes ContextMenuFactory once and just requests menus on right-click - Replace plural menu commands with single StandardActions; multi-selection handled inside builders - Fix NPE in ContextAction executable binding by removing null observables and binding menu disable state properly - Remove obsolete MultiContextAction and its tests Follow-ups: - Convert hardcoded labels to i18n keys (Download file(s), Open folder(s), etc.) - Consider removing *_FILES actions from enum if unused elsewhere - Re-add unit tests around builders/factory (TestFX/JUnit5) once API stabilized
ContextMenuFactory now delegates to ContextMenuBuilder strategies (SingleSelectionMenuBuilder / MultiSelectionMenuBuilder) with shared checks in SelectionChecks. |
|
All the review has been done, i will test it manually and do unit test, after make a PR if all is OK |
jabgui/src/main/java/org/jabref/gui/fieldeditors/contextmenu/SelectionChecks.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/fieldeditors/contextmenu/SelectionChecks.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/fieldeditors/contextmenu/SelectionChecks.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/fieldeditors/contextmenu/SingleSelectionMenuBuilder.java
Show resolved
Hide resolved
| assertEquals(false, | ||
| builder.supports(FXCollections.observableArrayList()), | ||
| "Empty selection should not be supported"); |
There was a problem hiding this comment.
The assertion should use assertFalse instead of assertEquals for better readability and to follow the special instruction on using plain JUnit assert.
| assertEquals(false, | ||
| builder.supports(single), | ||
| "Single selection should not be supported"); |
There was a problem hiding this comment.
The assertion should use assertFalse instead of assertEquals for better readability and to follow the special instruction on using plain JUnit assert.
| assertEquals(false, | ||
| builder.supports(FXCollections.observableArrayList()), | ||
| "Empty selection should not be supported"); |
There was a problem hiding this comment.
The use of assertEquals with boolean values is discouraged. Instead, assertFalse should be used for better readability and consistency with JUnit practices.
| assertEquals(true, | ||
| builder.supports(multiple), | ||
| "Multiple selection should be supported"); |
There was a problem hiding this comment.
The assertion should use assertTrue instead of assertEquals for better readability and to follow the special instruction on using plain JUnit assert.
| assertEquals(true, | ||
| builder.supports(multiple), | ||
| "Multiple selection should be supported"); |
There was a problem hiding this comment.
The use of assertEquals with boolean values is discouraged. Instead, assertTrue should be used for better readability and consistency with JUnit practices.
| assertEquals(false, itemsA.get(1).isDisable(), | ||
| "OPEN_FOLDER should be enabled when at least one local existing file is present"); |
There was a problem hiding this comment.
The assertion should use assertFalse instead of assertEquals for better readability and to follow the special instruction on using plain JUnit assert.
| assertEquals(false, itemsA.get(1).isDisable(), | ||
| "OPEN_FOLDER should be enabled when at least one local existing file is present"); |
There was a problem hiding this comment.
The use of assertEquals with boolean values is discouraged. Instead, assertFalse should be used for better readability and consistency with JUnit practices.
| assertEquals(true, itemsB.get(1).isDisable(), | ||
| "OPEN_FOLDER should be disabled when there are no local existing files"); |
There was a problem hiding this comment.
The assertion should use assertTrue instead of assertEquals for better readability and to follow the special instruction on using plain JUnit assert.
| assertEquals(true, itemsB.get(1).isDisable(), | ||
| "OPEN_FOLDER should be disabled when there are no local existing files"); |
There was a problem hiding this comment.
The use of assertEquals with boolean values is discouraged. Instead, assertTrue should be used for better readability and consistency with JUnit practices.
|
@subhramit @calixtus i changed interface to class, fixed few moments (cause i did test inside GUI really long time ago), deleted variable we discussed, i let .max() to be cause all logic works correctly. Video is too big, so i prepared google drive link for you: https://drive.google.com/file/d/14NRhxLdCbD-H9YNAt7p7ycETdTHrQBS_/view?usp=sharing I had lags (caused not by JabRef app), so some moments caused by it (you will see in video) P.S. bot now suggest to switch for assertTrue/False, but ihad it before, funny, i will ignore it if you don`t mind in my point of view all is ok and usable enough, logic is correct, i tried to optimize all i could (i mean you Set for folders etc.) |
|
Tragbot only offers suggestions. Just comment under it and close. |
|
Comment in github about tragbot, not in code |
it w8iting for feedback |
2:32 onwards, when doing "copy linked file(s) to folder", only one file is copied out of all selected. Why? |
| public ContextMenu createMenuForSelection(ObservableList<LinkedFileViewModel> selection) { | ||
| Objects.requireNonNull(selection, "selection must not be null"); |
There was a problem hiding this comment.
Use jspecify annotations.
This has been mentioned in comment #13726 (comment) and demonstrated in commit c327bd8.
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import static java.util.Objects.requireNonNull; |
There was a problem hiding this comment.
- Use jspecify annotations
- Don't import static.
| menuItems.add(buildCopyToFolderItem(actionFactory, selection)); | ||
| SelectionChecks checks = new SelectionChecks(databaseContext, preferences); | ||
|
|
||
| menuItems.add(batchCommandItem(actionFactory, StandardActions.OPEN_FILE, selection, _ -> true)); |
There was a problem hiding this comment.
Can you explain this change to me? You changed this::isLocalAndExists to _ -> true??
There was a problem hiding this comment.
@subhramit it worked that for selected files (only LOCAL!) it was working correctly, i deleted predicate cause we can open all files, URL files and Local files, so now it works correctly
There was a problem hiding this comment.
ok, for the sake of clean and readable, code maybe just add a dummy method alwaysEnabled to SelectionChecks.
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| Logger LOGGER = LoggerFactory.getLogger(SelectionChecks.class); | ||
| public record SelectionChecks(BibDatabaseContext databaseContext, GuiPreferences preferences) { |
There was a problem hiding this comment.
What was the rationale behind converting this to a record?
You are still using the methods here in the multiselectionmenubuilder class only. Why this additional layer of abstraction?
There was a problem hiding this comment.
I CAN MERGE theese methods in multiselection menu builder if you want, i guess it will reduce code and abstraction as you said. IT was planned to use in other class for Single too but now there is no need.
| switch (keyBinding.get()) { | ||
| case DELETE_ENTRY: | ||
| deleteAttachedFilesWithConfirmation(); | ||
| event.consume(); | ||
| break; | ||
| default: | ||
| // Pass other keys to children | ||
| if (keyBinding.get() == KeyBinding.DELETE_ENTRY) { | ||
| deleteAttachedFilesWithConfirmation(); | ||
| event.consume(); |
There was a problem hiding this comment.
Why was the default case handling not needed anymore?
There was a problem hiding this comment.
@subhramit there are 1 switch statement (1 case and default empty), i replaced it for the same exectly but with if statement which is more readable as for me
There was a problem hiding this comment.
We kept this deliberatly a switch to keep it consistent with keybindings check in other ui components.
@subhramit this is case with 1 online file and 1 local file, copied only 1 local file, guess this is all explanation i mean the online file is not downloaded I need to change only things above right? Don |
In the issue, the requirement comment says:
So this is wrong/misleading. The option doesn't say "Copy downloaded file(s) to to folder". |
I will do it available only for local tho |
I didn't get you, could you elaborate what you mean |
|
This PR seems to be more hard than thought.
I forgot the requirement that files should be moved -- e.g., when splitting a library into sub libraries. Not sure if this is really good first issue. |
|
@subhramit in case you mentioned 1 file is in browser, that @koppor i will try to do it anyway, doesn`t matter first issue or not BUT! Could you please give a bit of explanation about you said splitting libraries @subhramit @calixtus @koppor i will do changes we discussed. I really ask you to do after MY MAJOR CHANGES WILL BE PUSHED FULL REVIEW, to not extend work with it cause it took much more time i though and you i guess. So would be nice if i push major changes we discussed above, you all left comments what we fix and after i will try to do it once (maybe will be not 1 fix but anyway it will reduce our time! It`s a proposition) What guys do you think? |
calixtus
added
the
dev: no-bot-comments
|
Hi, i think the next iteration will probably be the last one. I just made some quick refactorings to ContextMenuFactoryTest to make it a bit more readable. It was just like a wall that hit me will all the declarations in the single tests. Please apply the suggested changes. After that, we will finish this PR and get it merged. Most is already good and you did a great job in this PR. |
Extend the Entry Editor context menu to handle entries with multiple linked files. This improves UX when managing more than one file per entry and aligns behavior across single- and multi-file scenarios.
Changes:
MultiSelectionMenuBuilderTest, SelectionChecksTest
Fixes #12567
Keywords: context menu, linked files, multi-selection, UI
Steps to test
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)