Skip to content

Fix #8390 by allowing multiple group deletion for Remove groups > Kee…#8875

Merged
Siedlerchr merged 3 commits into
JabRef:mainfrom
LIM0000:fix-issue-8390
May 31, 2022
Merged

Fix #8390 by allowing multiple group deletion for Remove groups > Kee…#8875
Siedlerchr merged 3 commits into
JabRef:mainfrom
LIM0000:fix-issue-8390

Conversation

@LIM0000

@LIM0000 LIM0000 commented May 31, 2022

Copy link
Copy Markdown
Contributor

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

Thank you LIM0000 for your PR.
Indeed, this solves the issue for Remove groups > Also remove subgroups.
However, a similar issue remains for Remove groups > Keep subgroups.
Please, could you look at this closely-related issue?

Proposed solution:

Fixes #8390

  • When removeGroupKeepSubgroups() 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)
  • All subgroups are assigned to their corresponding grandparents if there are any. Otherwise, they are assigned to All entries.

Single group deletion

3
4

Multiple groups deletion

1
2

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

@LIM0000 LIM0000 marked this pull request as ready for review May 31, 2022 09:01
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 31, 2022

@koppor koppor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks gut - just a nitpick

Comment thread src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java Outdated
Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>
@Siedlerchr Siedlerchr merged commit 538843a into JabRef:main May 31, 2022
@Siedlerchr

Copy link
Copy Markdown
Member

thanks for the quick follow up!

Siedlerchr added a commit that referenced this pull request Jun 1, 2022
* 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

Labels

component: groups 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.

Removing several groups deletes only one of them

4 participants