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

When changing a group color, no dialog should appear #8189

Closed
2 tasks done
koppor opened this issue Oct 25, 2021 · 9 comments · Fixed by #8909
Closed
2 tasks done

When changing a group color, no dialog should appear #8189

koppor opened this issue Oct 25, 2021 · 9 comments · Fixed by #8909
Assignees
Labels
component: groups component: ui good first issue An issue intended for project-newcomers. Varies in difficulty.

Comments

@koppor
Copy link
Member

koppor commented Oct 25, 2021

JabRef version

Latest development branch build (please note build date below)

Operating system

Windows

Details on version and operating system

Windows 10

Checked with the latest development build

  • I made a backup of my libraries before testing the latest development version.
  • I have tested the latest development version and the problem persists

Steps to reproduce the behaviour

  1. Edit the properties of a group
  2. Change the color
  3. Click "OK"
  4. Dialog "Change of Grouping Method" appears
    grafik

Expected behavior: No dialog appears, just the group color should change

Refs #7325

Appendix

No response

@koppor koppor added the good first issue An issue intended for project-newcomers. Varies in difficulty. label Oct 25, 2021
@koppor
Copy link
Member Author

koppor commented Oct 25, 2021

No triggered reassignment should then also keep the git diff noise to a minimum

Currently, following diff is generated:

grafik

@Siedlerchr
Copy link
Member

I looked a bit into it, the dialog is called here:

A solution would be to check if there have been changes to the group type or group keywords etc and only display that dialog in this cases.

public void editGroup(GroupNodeViewModel oldGroup) {
currentDatabase.ifPresent(database -> {
Optional<AbstractGroup> newGroup = dialogService.showCustomDialogAndWait(new GroupDialogView(
dialogService,
database,
preferences,
oldGroup.getGroupNode().getGroup(),
GroupDialogHeader.SUBGROUP));
newGroup.ifPresent(group -> {
// TODO: Keep assignments
boolean keepPreviousAssignments = dialogService.showConfirmationDialogAndWait(
Localization.lang("Change of Grouping Method"),
Localization.lang("Assign the original group's entries to this group?"));
// WarnAssignmentSideEffects.warnAssignmentSideEffects(newGroup, panel.frame());
boolean removePreviousAssignments = (oldGroup.getGroupNode().getGroup() instanceof ExplicitGroup)
&& (group instanceof ExplicitGroup);

@dimitrisdimos00
Copy link
Contributor

Hello there. In which cases is a warning useless except from when changing the color? For example, I'm thinking that when changing the icon of the group, no warning should be shown either. Do we need a warning only for the "Hierarchical context" and "Collect by" options?

dimitrisdimos00 added a commit to dimitrisdimos00/jabref that referenced this issue Jan 13, 2022
@dimitrisdimos00
Copy link
Contributor

Hello?

@Siedlerchr
Copy link
Member

Sorry, missed your question.
I guess it makes only sense when the group type or for example the keywords or the separator changed. For color and icon not

@Surabhiramesh
Copy link

Hi, we are a group of students studying Master of Computer Science in University of Adelaide and we wanted to check if this issue is available for us to work on for our assignment. And if it is available, we would like to get some valuable inputs based on the previous contributions to this issue.

yukioz added a commit to yukioz/jabref that referenced this issue Apr 3, 2022
yukioz added a commit to yukioz/jabref that referenced this issue Apr 3, 2022
@koppor
Copy link
Member Author

koppor commented Apr 3, 2022

In which cases is a warning useless except from when changing the color? For example, I'm thinking that when changing the icon of the group, no warning should be shown either.

You are right. Please make a list of all possible cases, where a) no dialog should be opened and b) where a dialog should be opened

Do we need a warning only for the "Hierarchical context" and "Collect by" options?

I am not sure. It should be properly documented and JUnit tests should exist.

@koppor
Copy link
Member Author

koppor commented Apr 3, 2022

@Surabhiramesh The issue is available. Please go ahead. Please also see my last comment.

@ThiloteE
Copy link
Member

ThiloteE commented Apr 3, 2022

Welcome and thank you for your engagement. Guidelines for contributing can be found here: https://github.com/JabRef/jabref/blob/main/CONTRIBUTING.md. See here for a rough outline of this process. In general, it is advised to open a (draft) pull request early on so that reviewers have time to comment and the general direction of the request becomes clear. This will allow you to receive valuable feedback!

If you have any questions, feel free to ask! Either here at GitHub, or you also can join our gitter chat.

LIM0000 added a commit to LIM0000/jabref that referenced this issue Jun 19, 2022
Siedlerchr added a commit that referenced this issue Oct 10, 2022
* Fix #8189 by checking group type

* Update different solution that is capable to prevent warning dialog without resetting to ExplitGroup

* Split groupType checking and groupFields checking into 2 different functions

* add check before dialog

* write groups always

* refersh

* store group changes

* rfactor code

* refactor with intanceof

* refactor

* fix stupid instanceof error^^

* checkstyle

* use getclass everywhere add comment

* Change or to and, only return true if no changes
add tests

* fuu checkstyle

* javadoc checkstyle

* TODO: clarifiy behavior when changing the groups name

not really sure what happns when you have more than 2 groups

* automatically assign when more than one group

* use new name

* todo

* we need to check old group name
rename methods

* checkstyle

* Only check for minor mods if group types are equal

Co-authored-by: Siedlerchr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: groups component: ui good first issue An issue intended for project-newcomers. Varies in difficulty.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants