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
i set up checkstyle with your guide but it doesn`t show me some problems in test classes, only in main module)
|
|
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 |
calixtus
left a comment
There was a problem hiding this comment.
Thanks for your PR so far.
Some remarks made.
I noticed the Strategy Pattern was somewhat broken there (not by you). Please fix it.
|
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 <50491877+calixtus@users.noreply.github.com>
…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 |
|
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. |
|
yes |
|
@koppor @calixtus @subhramit about merging to package ...logic. Would you mind if i create package in org,jabref.logic ---> or.jabref.logic.files Other way - i found LinkedFileHandler in package org.jabref.logic.externalfiles Or i even can make in this package additional file as LinkedFileCopier, for example What do you prefer? I will do LinkedFileCopier in externalfiles package for now, after your answer we will see 
|
|
Hey, we briefly talked about the refactoring, @koppor suggested. We think we should do this in a follow-up. So forget about moving stuff to logic and just polish this one. Then we finalize and merge. |
|
don't forget the openrewrite test |
Yes, sure, i will check submodules, do rewrite and changelog, ty for notifying |
|
What i did: 1.ContextMenuFactory - added jspecify NonNull and deleted static import, using Object now (Objects.requireNonNull(selection, "...")
Now i will run all tests, test it in application finally and do commit soon! |
|
You may have forgotten to push your commit. |
No-no, I`m sick now (ill), no energy for coding now, i will polish it soon do not worry |
|
Oh ok, sorry. Get well soon! |
|
You need to merge in the latest main, then it will be resolved |
|
@calixtus Done |
|
Hooray! |
* Add context menu for multi-file entries (JabRef#12567) 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 * Apply OpenRewrite recipes * Fixed test and optimized imports * fixed library and space in catch block * Runned rewrite, fixed catch block and optimize logic of test ContextMenuFactory * Fix tests: initialize JavaFX toolkit; replace empty catch with meaningful check * Fix tests: added space after { * Trigger CI * Trigger CI * Update jabgui/src/test/java/org/jabref/gui/copyfiles/CopyMultipleFilesActionTest.java Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> * refactor(gui/contextmenu): restore Strategy pattern for linked files 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 * feat(gui): multi-file context menu for linked files - Introduce selection-aware builders (SingleSelectionMenuBuilder / MultiSelectionMenuBuilder) with SelectionChecks - Wire into LinkedFilesEditor via ContextMenuFactory/ContextAction; proper enablement bindings - Remove CopyMultipleFilesAction; update StandardActions and view models - i18n updates for pluralized items and copy messages - Tests for ContextAction/ContextMenuFactory/Single&MultiSelection builders/SelectionChecks - Changelog + cleanup Closes JabRef#12567 * Address review: Optional/map, non-null params, logger, tests, menus * fixed tests * Update jabgui/src/main/java/org/jabref/gui/copyfiles/CopySingleFileAction.java Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> * Update jabgui/src/main/java/org/jabref/gui/copyfiles/CopySingleFileAction.java Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> * wip: local changes before style sync * Fix styles * Final style and CI checks * docs: sync http-server howto; jablib: sync jspecify @nullable from upstream * Naming convention * Fix LOGGER naming (convension) * fixed submodules * Fix submodules * chore: reset submodules to upstream/main (csl-styles, csl-locales) * Fix submodules * Fix test classes * Fix test classes V2 (forget to update MultiSelectionMenuBuilderTest and SelectionChecksTest) * Apply JSpecify annotations * Fixed bot's cases and fixed submodules * Fix submodules * Migrate to JSpecify annotations * Fix wording * Changed assert methods for readability * interface ---< class, deleted variable * Refactored ContextMenuFactoryTest for readability * Added fixes, deleted SelectionChecks * changelog fixed and localization --------- Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> Co-authored-by: Christoph <siedlerkiller@gmail.com> Co-authored-by: Carl Christian Snethlage <calixtus@users.noreply.github.com>
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