Skip to content

Conversation

koppor
Copy link
Member

@koppor koppor commented Aug 23, 2025

The PR #12374 had several issues

  • "copy to" menu always enabled (and not only if two libraries shown)
  • "copy to" menu showing the current library only
  • "copy to" menu not really refreshing on library change

This PR fixes it.

Steps to test

  1. Start JabRef
  2. Open more than one library
  3. Try "Copy to" context menu
  4. Change opened libraries (e.g., close one, open another one)
  5. Try "Copy to" context menu

Mandatory checks

  • I own the copyright of the code submitted and I license it under the MIT license
  • [/] Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • [/] Tests created for changes (if applicable)
  • [/] Manually tested changed features in running JabRef (always required)
  • [/] Screenshots added in PR description (if change is visible to the user)
  • [/] 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.

@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Aug 23, 2025
subhramit
subhramit previously approved these changes Aug 23, 2025
Copy link
Member

@subhramit subhramit left a comment

Choose a reason for hiding this comment

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

Code comments were very useful

@subhramit subhramit added status: awaiting-second-review For non-trivial changes and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Aug 23, 2025
ImportHandler importHandler) {
Menu copyToMenu = factory.createMenu(StandardActions.COPY_TO);
copyToMenu.disableProperty().bind(
Bindings.size(stateManager.getOpenDatabases()).lessThan(2)
Copy link
Member

Choose a reason for hiding this comment

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

Use Action Helper needs multiple datahases

public static BooleanExpression needsMultipleDatabases(TabPane tabbedPane) {
return Bindings.size(tabbedPane.getTabs()).greaterThan(1);
}

};

// EasyBind.subscribe() is not available for lists, therefore "manually" triggering rebuild and subscribing
rebuildMenu.run();
Copy link
Member

Choose a reason for hiding this comment

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

You can directly call subscribe on lists:
stateManager.getOpenDatabases().subscribe( Runnable )

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

use existing methods

@koppor koppor added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed status: awaiting-second-review For non-trivial changes labels Aug 24, 2025
);

ObservableList<BibDatabaseContext> openDatabases = stateManager.getOpenDatabases();
// Menu is created on each right-click, thus we can always assume that the list of open databases is up-to-date
Copy link

Choose a reason for hiding this comment

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

Comment restates what is obvious from the code context and doesn't provide additional value or reasoning. Comments should add new information not derivable from code.

@subhramit subhramit added status: awaiting-second-review For non-trivial changes and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Aug 24, 2025
@Siedlerchr Siedlerchr added this pull request to the merge queue Aug 25, 2025
Merged via the queue into main with commit fbbe2b4 Aug 25, 2025
2 checks passed
@Siedlerchr Siedlerchr deleted the fix-copy-to branch August 25, 2025 05:17
Siedlerchr added a commit that referenced this pull request Sep 8, 2025
* upstream/main: (32 commits)
  Fix path (#13769)
  Mode aware consistency check (#13584)
  Refine JBang check (#13765)
  Add Language Server to the UI and add the integrity/consistency check (#13697)
  Fix/remove comment code (#13763)
  New Crowdin updates (#13760)
  Bump org.openrewrite.rewrite from 7.14.0 to 7.14.1 (#13757)
  Bump com.autonomousapps:dependency-analysis-gradle-plugin (#13756)
  Bump dev.langchain4j:langchain4j-bom from 1.2.0 to 1.3.0 in /versions (#13755)
  Bump jablib/src/main/resources/csl-locales from `fa56de1` to `e29c453` (#13754)
  Bump com.autonomousapps:dependency-analysis-gradle-plugin (#13753)
  Bump org.mockito:mockito-core from 5.18.0 to 5.19.0 in /versions (#13752)
  Bump actions/upload-pages-artifact from 3 to 4 (#13751)
  Migrate fetchers to Search.g4 ANTLR parser. (#13691)
  [Junie]: fix: resolve IllegalArgumentException for non-absolute URIs (#13669)
  Add auto-renaming of linked files on entry data change (#13295)
  Walkthrough additions (#13745)
  Switch from zulu to corretto (#13749)
  New Crowdin updates (#13747)
  Fix copy to (#13741)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: awaiting-second-review For non-trivial changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants