Add button-icon for union/intersection in the groups side panel #3954

Siedlerchr · 2018-04-14T18:41:47Z

Fixes #3269
Fix display of state for union/intersection in preferences

As we currently unfortunately don't have Icons for Set Intersection/Union
set-center and set-all https://materialdesignicons.com/icon/set-center
in the Material Design Font, I used the WWW and the twitter icon to demonstrate the purpose. @halirutan will then try to integrate the correoct icons in JabRef

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Ensured that the git commit message is a good one
Check documentation status (Issue created for outdated help page at help.jabref.org?)

Fix display ofs state in preferences

tobiasdiez

In general the code looks good.

Of course, the icons have to be replaced by proper ones before merge. It would be nice if somehow can update fontawesomefx to include the most recent icons since there are other instances where newer icons are more suitable (see IconTheme).

tobiasdiez · 2018-04-15T10:29:41Z

src/main/java/org/jabref/gui/SidePaneComponent.java

@@ -88,7 +90,7 @@ public final Node getContentPane() {
    /**
     * @return the header pane for this component
     */
-    public final Node getHeader() {
+    public Node getHeader() {


Instead of overriding this method, I would propose to create a new (overridable) method getHeaderButtons that results additional header button to be displayed. In this way you ensure consistency as well as reduce code duplication.

Added a protected method for gettingAddtionalHeader buttons

tobiasdiez · 2018-04-15T10:29:52Z

src/main/java/org/jabref/gui/SidePaneComponent.java

        buttonContainer.getChildren().addAll(up, down, close);
        BorderPane graphic = new BorderPane();
        graphic.setCenter(icon.getGraphicNode());
-        BorderPane container = new BorderPane();
-//        container.setLeft(graphic);
+        //        container.setLeft(graphic);


tobiasdiez · 2018-04-15T10:30:58Z

src/main/java/org/jabref/gui/groups/GroupSidePane.java

+        boolean newState = !intersectionToggled;
+        preferences.putBoolean(JabRefPreferences.GROUP_INTERSECT_SELECTIONS, newState);
+
+        BasePanel basePanel = frame.getCurrentBasePanel();


Please remove the dependency on frame and replace it by dialog service (which has a notify method).

tobiasdiez · 2018-04-15T10:33:10Z

src/main/java/org/jabref/gui/preftabs/GroupsPrefsTab.java

+
+        boolean intersectionSelected = prefs.getBoolean(JabRefPreferences.GROUP_INTERSECT_SELECTIONS);
+        //Looks odd, but the prefs store true for intersection and false for union
+        if (intersectionSelected) {


Since this is hard to understand, I would propose to add a better named method to JabrefPreferences. Moreover, the code is probably easier to understand if you add an enum for the different modes.

Now introduced an enum with methods for setting/get the pref value
Additionally, the Icons are now directly added in the enum, so it should be easier to change them

Personally, I would separate the group mode enum from the icon. The mode is something that can (in principle) be defined and used in the model/logic package thus I wouldn't mix it with something gui related.

Well, however, this is purely a gui thing. My immediate thoughts were also about storing that in model, but it would provide any additional value. And this makes it easier to directly use the icon,

create enum with icons change booleans to enum remove duplicated code remove old prefs

Siedlerchr · 2018-04-20T15:10:58Z

@tobiasdiez Is this now okay? Then you could merge it with CO-Authors

tobiasdiez

I've still some remarks, but these are very minor. You can directly merge after fixing them.

tobiasdiez · 2018-04-20T15:56:40Z

src/main/java/org/jabref/gui/SidePaneComponent.java


 public abstract class SidePaneComponent {

    protected final SidePaneManager manager;
    protected final ToggleCommand toggleCommand;
-    private final JabRefIcon icon;
-    private final String title;
+    protected final JabRefIcon icon;


make them private again

tobiasdiez · 2018-04-20T15:56:50Z

src/main/java/org/jabref/gui/SidePaneComponent.java

    public SidePaneComponent(SidePaneManager manager, JabRefIcon icon, String title) {
        this.manager = manager;
        this.icon = icon;
        this.title = title;
        this.toggleCommand = new ToggleCommand(this);
+


tobiasdiez · 2018-04-20T15:58:41Z

src/main/java/org/jabref/gui/SidePaneComponent.java

        container.setCenter(label);
        container.setRight(buttonContainer);
        container.getStyleClass().add("sidePaneComponentHeader");

        return container;
    }

+    protected Optional<Node> getAddtionalHeaderButtons() {


Just from the name of the method, I would have expected a list of buttons as the return value and not a single one. I think, supporting multiple buttons make sense and is not a big change overall.

tobiasdiez · 2018-04-20T15:59:46Z

src/main/java/org/jabref/gui/groups/GroupSidePane.java

+        return preferences.getGroupViewMode().getIcon().getGraphicNode();
+    }
+
+    private Tooltip getUnionInterSectionToolTip() {


"Intersection" and "Tooltip"

tobiasdiez · 2018-04-20T16:03:09Z

src/main/java/org/jabref/gui/groups/GroupSidePane.java

@@ -20,15 +25,43 @@
 public class GroupSidePane extends SidePaneComponent {

    private final JabRefPreferences preferences;
+    private final DialogService dialogService;
+    private final Button intersectionUnionToggle = IconTheme.JabRefIcons.WWW.asButton();
+    private final Tooltip toggleUnion = new Tooltip(Localization.lang("Toogle union"));


I think there is no need to have these tooltips as instance variables. I would simply inline them.

tobiasdiez · 2018-04-20T16:04:51Z

src/main/java/org/jabref/gui/groups/GroupSidePane.java

+    }
+
+    private Node getUnionIntersectionGraphic() {
+        return preferences.getGroupViewMode().getIcon().getGraphicNode();


Personally, I would extract these methods to a new class GroupViewModeViewModel which gives you the graphic and the tooltip (constructor accepts the mode). In this way, you clearly separate everything ui-related from the actual model class.

tobiasdiez · 2018-04-20T16:11:32Z

src/main/java/org/jabref/preferences/JabRefPreferences.java

@@ -193,7 +194,8 @@
    public static final String EDITOR_EMACS_KEYBINDINGS_REBIND_CA = "editorEMACSkeyBindingsRebindCA";
    public static final String EDITOR_EMACS_KEYBINDINGS_REBIND_CF = "editorEMACSkeyBindingsRebindCF";
    public static final String GROUPS_DEFAULT_FIELD = "groupsDefaultField";
-    public static final String GROUP_INTERSECT_SELECTIONS = "groupIntersectSelections";
+    public static final String GROUP_INTERSECT_UNION_VIEW_MODE = "groupIntersectUnionViewModes";


this can be private now, right?

…rsectionnew * upstream/maintable-beta: fix FX thread error on save database dlg Fix close confirmation Bring back special fields in menu Remove unused methods add search performance tweak from master

return headerbuttons as list

Siedlerchr · 2018-04-26T13:45:49Z

@tobiasdiez Fixed the issues you m entionend.

tobiasdiez

Only a few very minor things are remaining. Please merge directly after fixing them.

tobiasdiez · 2018-05-02T12:58:57Z

src/main/java/org/jabref/gui/groups/GroupModeViewModel.java

+
+    public static Tooltip getUnionIntersectionTooltip(GroupViewMode mode) {
+        if (mode == GroupViewMode.UNION) {
+            return new Tooltip(Localization.lang("Toogle intersection"));


The texts seem to be interchanged by mistake.

Nope, it's right. As the tooltip should indicate what happens when you press the button,
If you are in union mode currently -> togle intersection
If you are in intersection mode -> toggle union

tobiasdiez · 2018-05-02T13:00:32Z

src/main/java/org/jabref/gui/groups/GroupModeViewModel.java

+    }
+
+    public static Node getUnionIntersectionGraphic(GroupViewMode mode) {
+        return mode.getIcon().getGraphicNode();


Don't you agree that a code similar to the above one for the tooltip is better than storing the icon directly in the groupviewmode?

tobiasdiez · 2018-05-02T13:01:42Z

src/main/java/org/jabref/preferences/JabRefPreferences.java

@@ -193,7 +194,8 @@
    public static final String EDITOR_EMACS_KEYBINDINGS_REBIND_CA = "editorEMACSkeyBindingsRebindCA";
    public static final String EDITOR_EMACS_KEYBINDINGS_REBIND_CF = "editorEMACSkeyBindingsRebindCF";
    public static final String GROUPS_DEFAULT_FIELD = "groupsDefaultField";
-    public static final String GROUP_INTERSECT_SELECTIONS = "groupIntersectSelections";
+    public static final String GROUP_INTERSECT_UNION_VIEW_MODE = "groupIntersectUnionViewModes";


…rsectionnew * upstream/maintable-beta: (88 commits) set look and feel to windows, aqua or nimbus for swing in case fix import remove look and feel New translations JabRef_en.properties (French) (#4009) Fix Look and Feel related issues (#4002) Fix statement in changelog [WIP] Add Text File Export for "Find Unlinked Files" (#3979) Fix IEEE preview does not display month (#3983) Activate context menu on key press (#4004) Improve code layout Fix concurrent modification exception when adding entries to groups Fix build Typo Add fix Rename test Fix #3994 Duplicate unmodifiable list for sorting (#3996) Remove deprecated and unused method (#3993) Improvements around external file types (#3887) Migrate to native gradle test task (#3987) Do not run checkstyle as part of the gradle check task (#3985) ...

make prefs private

Siedlerchr · 2018-05-06T10:30:18Z

I fixed the remaining issues and I am going to merge this one now

* Add button for union/intersection in the groups side panel Fix display ofs state in preferences * add method for getting addtionalHeaderButtons create enum with icons change booleans to enum remove duplicated code remove old prefs * remove uncommented code * fix checkstyle * Add group intersection and union icons * add tooltips for side pane and groups pane * Extract get graphics and tooltip to new class return headerbuttons as list * refactor and extract icon method to view model make prefs private

…drop * upstream/maintable-beta: (174 commits) Fix ACM fetcher import of entries (#4019) Reimplement tooltips for file and identifier columns (#4011) Add button-icon for union/intersection in the groups side panel (#3954) Update Dependencies (#4012) set look and feel to windows, aqua or nimbus for swing in case fix import remove look and feel New translations JabRef_en.properties (French) (#4009) Fix Look and Feel related issues (#4002) Fix statement in changelog [WIP] Add Text File Export for "Find Unlinked Files" (#3979) Fix IEEE preview does not display month (#3983) Activate context menu on key press (#4004) Improve code layout Fix concurrent modification exception when adding entries to groups Fix build Typo Add fix Rename test Fix #3994 Duplicate unmodifiable list for sorting (#3996) ... # Conflicts: # src/main/java/org/jabref/gui/actions/CleanupAction.java # src/main/java/org/jabref/gui/maintable/MainTable.java

Add button for union/intersection in the groups side panel

8b9e6a9

Fix display ofs state in preferences

Siedlerchr changed the title ~~Add button for union/intersection in the groups side panel~~ Add button-icon for union/intersection in the groups side panel Apr 14, 2018

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 14, 2018

Siedlerchr requested a review from tobiasdiez April 14, 2018 18:45

tobiasdiez requested changes Apr 15, 2018

View reviewed changes

Siedlerchr and others added 5 commits April 15, 2018 18:02

add method for getting addtionalHeaderButtons

1a72f7d

create enum with icons change booleans to enum remove duplicated code remove old prefs

remove uncommented code

04ed6b3

fix checkstyle

971f384

Add group intersection and union icons

4e3757b

add tooltips for side pane and groups pane

c2ec176

tobiasdiez requested changes Apr 20, 2018

View reviewed changes

Siedlerchr added 2 commits April 21, 2018 10:08

Merge remote-tracking branch 'upstream/maintable-beta' into unioninte…

099d579

…rsectionnew * upstream/maintable-beta: fix FX thread error on save database dlg Fix close confirmation Bring back special fields in menu Remove unused methods add search performance tweak from master

Extract get graphics and tooltip to new class

ae1a438

return headerbuttons as list

tobiasdiez requested changes May 2, 2018

View reviewed changes

tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 2, 2018

Siedlerchr added 2 commits May 5, 2018 22:29

refactor and extract icon method to view model

17a4c77

make prefs private

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 5, 2018

Siedlerchr merged commit 8a8e09a into maintable-beta May 6, 2018

Siedlerchr deleted the unionintersectionnew branch May 6, 2018 10:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add button-icon for union/intersection in the groups side panel #3954

Add button-icon for union/intersection in the groups side panel #3954

Siedlerchr commented Apr 14, 2018 •

edited

Loading

tobiasdiez left a comment

tobiasdiez Apr 15, 2018

Siedlerchr Apr 15, 2018

tobiasdiez Apr 15, 2018

tobiasdiez Apr 15, 2018

tobiasdiez Apr 15, 2018

Siedlerchr Apr 15, 2018 •

edited

Loading

tobiasdiez Apr 15, 2018

Siedlerchr Apr 15, 2018

Siedlerchr commented Apr 20, 2018

tobiasdiez left a comment

tobiasdiez Apr 20, 2018

tobiasdiez Apr 20, 2018

tobiasdiez Apr 20, 2018

tobiasdiez Apr 20, 2018

tobiasdiez Apr 20, 2018

tobiasdiez Apr 20, 2018

tobiasdiez Apr 20, 2018

tobiasdiez May 2, 2018

Siedlerchr commented Apr 26, 2018

tobiasdiez left a comment

tobiasdiez May 2, 2018

Siedlerchr May 2, 2018

tobiasdiez May 2, 2018

tobiasdiez May 2, 2018

Siedlerchr commented May 6, 2018

Add button-icon for union/intersection in the groups side panel #3954

Add button-icon for union/intersection in the groups side panel #3954

Conversation

Siedlerchr commented Apr 14, 2018 • edited Loading

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr Apr 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Apr 20, 2018

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented Apr 26, 2018

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siedlerchr commented May 6, 2018

Siedlerchr commented Apr 14, 2018 •

edited

Loading

Siedlerchr Apr 15, 2018 •

edited

Loading