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

[WIP] Toggle group intersection/union solved #3739

Closed
wants to merge 8 commits into from

Conversation

stoever
Copy link

@stoever stoever commented Feb 18, 2018

Added new menu entry with the needed action.
The action gets and sets the group intersection/union preferences.

Fixes #3269

PE1_Blatt_5
Aufgabe 7G

Matrikelnummer: 3325847
Name: Tobias Stöver


  • Change in CHANGELOG.md described
  • Tests created for changes
  • Screenshots added (for bigger UI changes)
  • Manually tested changed features in running JabRef
  • Check documentation status (Issue created for outdated help page at help.jabref.org?)

@@ -750,6 +750,13 @@ public void update() {
actions.put(Actions.REMOVE_FROM_GROUP, new GroupAddRemoveDialog(this, false, false));
actions.put(Actions.MOVE_TO_GROUP, new GroupAddRemoveDialog(this, true, true));

actions.put(Actions.INTERSECTION_PREF_GROUP, (BaseAction) () -> {
System.out.println("blubb");
Copy link
Member

Choose a reason for hiding this comment

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

remove

actions.put(Actions.INTERSECTION_PREF_GROUP, (BaseAction) () -> {
System.out.println("blubb");
boolean state = Globals.prefs.getBoolean(JabRefPreferences.GROUP_INTERSECT_SELECTIONS);
state = !state;
Copy link
Member

Choose a reason for hiding this comment

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

This code is hard to understand and not clear on the first sight.
Better create two variables: e.g. union and intersection and then store the corresponding value

Copy link
Member

Choose a reason for hiding this comment

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

Ideally you would have an enum with two states, e.g. GroupState... but in this case this might be an overkill.

@@ -115,6 +115,7 @@ public void setValues() {
keywordSeparator.setText(prefs.get(JabRefPreferences.KEYWORD_SEPARATOR));
autoAssignGroup.setSelected(prefs.getBoolean(JabRefPreferences.AUTO_ASSIGN_GROUP));
multiSelectionModeIntersection.setSelected(prefs.getBoolean(JabRefPreferences.GROUP_INTERSECT_SELECTIONS));
multiSelectionModeUnion.setSelected(!prefs.getBoolean(JabRefPreferences.GROUP_INTERSECT_SELECTIONS));
Copy link
Member

Choose a reason for hiding this comment

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

this is really not a good idea. If you quickly go over the code you most certainly will oerlook the not sign.
The goal is to have clear code which is ideally understanding at first sight.
https://8thlight.com/blog/dariusz-pasciak/2015/05/28/alternatives-to-boolean-parameters.html

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.

See my comments about the handling of the boolean paramters.

@koppor koppor added the PE1718 label Feb 19, 2018
Added an new boolean variable to make the code more readable.
Added the two @author javadoc comments.
Added one @author javadoc comment.
Added @author javadoc comment.
Set the groups union preference radiobutton, because if the group state was on union, the radiobutton was not activated.
Used an own boolean variable for the state of the union preference.
@@ -750,6 +750,15 @@ public void update() {
actions.put(Actions.REMOVE_FROM_GROUP, new GroupAddRemoveDialog(this, false, false));
actions.put(Actions.MOVE_TO_GROUP, new GroupAddRemoveDialog(this, true, true));

/**
Copy link
Member

Choose a reason for hiding this comment

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

Remove your author comments.
We keep our authors in a separate file: https://github.com/JabRef/jabref/blob/master/AUTHORS

@lenhard lenhard changed the title Issue3269: Toggle group intersection/union solved [WIP] Toggle group intersection/union solved Feb 21, 2018
@koppor
Copy link
Member

koppor commented Mar 9, 2018

@Siedlerchr please decide whether you take over or just close the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants