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.

Sign up for GitHub

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

Jump to bottom

Fix for removing several groups deletes only one of them #8801

Merged
merged 8 commits into from
May 30, 2022
Merged

Fix for removing several groups deletes only one of them #8801

merged 8 commits into from
May 30, 2022

Conversation

LIM0000
Copy link
Contributor

@LIM0000 LIM0000 commented May 16, 2022 •
edited
Loading

Previous discussion about removing several selected groups only delete one of them #8390

You are prompted to confirm the deletion of one group. After accepting, this group is deleted, but you are not prompted to delete the other groups.
So, while it seems that several can be removed at the same time, groups can be removed only one by one.

Proposed solution

Fixes #8390

  • When removeGroupAndSubgroups() is called, iterate through all selected groups instead of the group that triggers the event handler.
  • Popup a confirmation dialog and confirm if user decides to delete:
    • single group
    • multiple groups (if multiple groups selection)
  • assign selectedGroups (which is ListProperty<GroupNodeViewModel>) to new array to prevent forEach skips an element when a previous element is deleted

Single group deletion

p1 1
p1 2

Multiple groups deletion

p2 1
p2 2

PR checklist:

  • 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 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.

@ThiloteE ThiloteE mentioned this pull request May 16, 2022
Closed
@ThiloteE
Copy link
Member

I don't know much about coding, so bear with me, if what I say is not possible, but I was just thinking: We do have the option to "remove subgroups". Maybe some code can be borrowed from that part? I crosschecked and the initial reporter of #7556 also confirmed that removing multiple subgroups seems to work correctly :-)

@LIM0000
Copy link
Contributor Author

LIM0000 commented May 16, 2022 •
edited
Loading

Thank you for your review @ThiloteE.

In the latest main development branch c38d18b, "remove subgroups" codes only able to remove subgroups of 1 selected group. Since the code will return subgroups that are children of the selected group, removing multiple subgroups works correctly.

In this PR, the codes have to deal with more than 1 selected group, and these selected groups might not be the children of selected groups.

FYI, both "remove subgroups" and "Remove group -> Also remove subgroups" functions have very similar codes in them.

Thanks for your review again!

calixtus
calixtus reviewed May 20, 2022
View reviewed changes
src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java Outdated
Comment on lines 302 to 307
GroupNodeViewModel[] selectedGroupNodes = new GroupNodeViewModel[selectedGroups.size()];
selectedGroupNodes = selectedGroups.toArray(selectedGroupNodes);
for (GroupNodeViewModel eachNode : selectedGroupNodes) {
removeGroupsAndSubGroupsFromEntries(eachNode);
eachNode.getGroupNode().removeFromParent();
}
Copy link
Member

Choose a reason for hiding this comment

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

why are you creating an array first? You should be able to iterate through the ArrayList selectedGroups directly...
You can probably work with a new ArrayList<>(selectedGroups) too...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback @calixtus.
The first solution came into my mind was C type array to prevent skip element after deletion. Definitely a rookie mistake from me.
I have updated with the use of ArrayList<> instead of array[].
Cheers.

@calixtus
Copy link
Member

Codeweise Looks good so far. Please add the missing language keys to the English localization file to fix the tests and update the changelog.

@LIM0000 LIM0000 marked this pull request as ready for review May 23, 2022 23:57
calixtus
calixtus approved these changes May 24, 2022
View reviewed changes
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I hope you liked working with jabrefs codebase so far. We would love to see more. 😄

@calixtus calixtus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 24, 2022
@Siedlerchr Siedlerchr merged commit 61c5787 into JabRef:main May 30, 2022
Siedlerchr added a commit that referenced this pull request May 30, 2022
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into dependabot/gradle/o…
aac6985 
…rg.apache.lucene-lucene-queries-9.2.0

* upstream/main:
  Fix for removing several groups deletes only one of them (#8801)
  Disable Write XMP Button in General tab of Entry-Editor when action is in progress (#8728)
  Bump jsoup from 1.14.3 to 1.15.1 (#8864)
  Bump unirest-java from 3.13.8 to 3.13.10 (#8869)
  Bump unoloader from 7.3.2 to 7.3.3 (#8863)
  Bump pascalgn/automerge-action from 0.15.2 to 0.15.3 (#8860)
  Bump classgraph from 4.8.146 to 4.8.147 (#8861)
  Bump mockito-core from 4.5.1 to 4.6.0 (#8862)
Siedlerchr added a commit that referenced this pull request Jun 1, 2022
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into fileManager
bca0ac8 
* upstream/main:
  Add an importer for Citavi backup files (#8848)
  Reviewdoc: Comment on PRs (#8878)
  Squashed 'buildres/csl/csl-styles/' changes from 649aac4..e740261
  Use JDK 15 text blocks to improve injected languages readability (#8874)
  Fix fetcher tests (#8877)
  Fix #8390 by allowing multiple group deletion for Remove groups > Kee… (#8875)
  Add restart warning on SSL configuration change (#8871)
  Update to lucene 9.2 (#8868)
  Fix for removing several groups deletes only one of them (#8801)
  Disable Write XMP Button in General tab of Entry-Editor when action is in progress (#8728)
  Bump jsoup from 1.14.3 to 1.15.1 (#8864)
  Bump unirest-java from 3.13.8 to 3.13.10 (#8869)
  Bump unoloader from 7.3.2 to 7.3.3 (#8863)
  Bump pascalgn/automerge-action from 0.15.2 to 0.15.3 (#8860)
  Bump classgraph from 4.8.146 to 4.8.147 (#8861)
  Bump mockito-core from 4.5.1 to 4.6.0 (#8862)
  Lucence dir checkers should only delete lucence dirs (#8854)
  Update README.md (#8858)
  Update adr.md
  Update adr.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calixtus calixtus calixtus approved these changes

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees
No one assigned
Labels
groups status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Removing several groups deletes only one of them
4 participants
@LIM0000 @ThiloteE @calixtus @Siedlerchr