Skip to content
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

Fixes #7016 toggle of special fields does not work for sorted entries #7656

Merged
merged 2 commits into from
Apr 26, 2021

Conversation

JavuesZhang
Copy link
Contributor

Fixes #7016
Make a copy of selected entries and change field in the copy so that the sort strategy will not influence the result.

The original version used for-each on the selected entry list but ConcurrentModificationException happened. Then I tried to replace it with normal for loop but the toggle problem still exist, though the exception did not occur any more. By checking the list in each loop I found that the order of the items in this list was changed during the for loop. Not exactly knowing how the sorting strategy is performed, I make a copy of the list and do field change on the copied list. Finally the toggle action performs correctly.

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Make a copy of selected entries and change field in the copy so that the sort strategy will not influence the result.
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 20, 2021
Copy link
Member

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 left a comment

Choose a reason for hiding this comment

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

This looks good to me.

I think there used to be a different open issue related to the list update on permutation in

// Add the listener that binds selection to state manager (TODO: should be replaced by proper JavaFX binding as soon as table is implemented in JavaFX)
mainTable.addSelectionListener(listEvent -> Globals.stateManager.setSelectedEntries(mainTable.getSelectedEntries()));

Perhaps that one is also solved by this PR or a similar strategy. I'll see if I can find it 😃

@@ -77,6 +77,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
- We fixed an issue with saving large `.bib` files [#7265](https://github.com/JabRef/jabref/issues/7265)
- We fixed an issue with very large page numbers [#7590](https://github.com/JabRef/jabref/issues/7590)
- We fixed an issue where the article title with curly brackets fails to download the arXiv link (pdf file). [#7633](https://github.com/JabRef/jabref/issues/7633)
- We fixed an issue with toggle of special fields does not work for sorted entries [#7016](https://github.com/JabRef/jabref/issues/7016)

Choose a reason for hiding this comment

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

I'd suggest 'We fixed an issue with toggling special fields for sorted entries.'

@Siedlerchr Siedlerchr merged commit eecd082 into JabRef:main Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toggle of special fields does not work for sorted entries
3 participants