Group interface: Usability tweaks #3715

Escoul · 2018-02-13T12:22:19Z

This fixes the name of the group editing window to "Add group" instead of "Edit Group" when adding a new group.
The group editing window can now also be called by double-clicking the group to be edited.

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?)

Credit (for an exercise):
Rico Haas
David Zeck

Fix the name of the group editing window to "Add group" instead of "Edit Group" when adding a new group. The group editing window can now also be called by double-clicking the group to be edited.

lenhard

Thank you very much for your contribution!

I have tested it locally and the dialogs work as expected. Good job overall! I have two minor issues that should be fixed and once this is done, the PR can be merged.

lenhard · 2018-02-13T13:25:15Z

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

@@ -148,7 +148,8 @@ public Dimension getPreferredSize() {
     *                    created.
     */
    public GroupDialog(JabRefFrame jabrefFrame, AbstractGroup editedGroup) {
-        super(jabrefFrame, Localization.lang("Edit group"), true, GroupDialog.class);
+        super(jabrefFrame, (editedGroup == null) ? Localization.lang("Add group") : Localization.lang("Edit group"),


I am really no friend of the inline syntax for conditional branching. In this special case (calling super), I'll go with it though. The reason for this is it's not possible to do a proper branching before the super call and the line is actually quite readable.

Thanks, i think so too. The only alternative would have been to create a separate method just to determine the name of the frame.

lenhard · 2018-02-13T13:26:35Z

src/main/resources/l10n/JabRef_en.properties

@@ -386,6 +386,7 @@ Edit\ entry=Edit entry
 Save\ file=Save file
 Edit\ file\ type=Edit file type

+Add\ group=Add group


There is already add\ group in line 57 of the same file. We really don't need localizations that differ only in the casing of the first word. Please remove the old (lower case) localization and make sure that everything still works.

The old localization line is used by gui.groups.UndoableAddOrRemoveGroup.getPresentationName() to name a undoable/redoable action. The problem is, that all actions of this kind are named in lowercase, it would be awkward to change just this name to uppercase.

I'm not happy with having the essentially same translation key twice, but I get your point. We can leave this as it is.

lenhard · 2018-02-13T13:29:58Z

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

+            row.setOnMouseClicked((mouseClickedEvent) -> {
+                if (mouseClickedEvent.getButton().equals(MouseButton.PRIMARY)
+                        && (mouseClickedEvent.getClickCount() == 2)) {
+                    GroupNodeViewModel groupToEdit = EasyBind.monadic(row.itemProperty()).getValue();


Why is there a call to Easybind.monadic? To get the selected group, you only need to call row.itemProperty().getValue() and that's it. Please change the line accordingly.

Good point, fixed in 3ae9885.

tobiasdiez

Thanks for your contribution and the quick follow-up. The code looks good to me now and I find your responses to Jörg's remarks convincing, but I'll let @lenhard to have the last word and put the decision to merge in his hand.

lenhard · 2018-02-14T08:58:26Z

Ok, with these changes the code is good to merge and, thus, we have positive reviews from two developers. So, I'll merge the PR. Once again, thanks for your contribution!

koppor · 2018-02-14T10:31:47Z

@Escoul Thank you for working on that and giving good answers to the questions. What would you think to add an architectural decision on your conditional if? https://github.com/JabRef/jabref/blob/master/CONTRIBUTING.md#when-making-an-architectural-decision

All in all, we would be happy to see more contributions from your side. We use the label good-first-issue, where more issues will be tagged. You can also have a look at https://github.com/koppor/jabref/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22. Or hop by at my office in V38 in Stuttgart. 😇

…lapse it

…lapse it (#3834)

…drop * upstream/maintable-beta: (97 commits) Update Eclipse style to intellij style (#3827) fix some not on fx thread dialogs in preferences fix inital save error Convert last dialog to javafx refactor exception Merge changes for renamed actions New Crowdin translations (#3835) Partial revert of #3715 since double click on group should expand/collapse it (#3834) Add Spanish translations (#3833) update applicationsinsights update javafxsvg to 1.3.0 Disable FTP Download on CI Apply copy linked files dialog fix from master Show dialog when copy files did not found file (#3826) Remove customjfx (#3779) Disable FTP Download on CI Remove color customization for maintable from preferences (#3808) Remove erroneous escape character (#3831) New translations JabRef_en.properties (French) New translations JabRef_en.properties (Tagalog) New translations JabRef_en.properties (Italian) New translations JabRef_en.properties (Indonesian) ... # Conflicts: # src/main/java/org/jabref/gui/maintable/MainTable.java

Changes regarding issue koppor#277:

1dfc4e5

Fix the name of the group editing window to "Add group" instead of "Edit Group" when adding a new group. The group editing window can now also be called by double-clicking the group to be edited.

lenhard suggested changes Feb 13, 2018

View reviewed changes

lenhard added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Feb 13, 2018

Remove unnecessary Easybind.monadic call.

3ae9885

tobiasdiez approved these changes Feb 13, 2018

View reviewed changes

lenhard approved these changes Feb 14, 2018

View reviewed changes

lenhard merged commit 243098d into JabRef:master Feb 14, 2018

koppor added PE1718 and removed status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Feb 14, 2018

tobiasdiez mentioned this pull request Feb 25, 2018

Group interface: Usability koppor/jabref#277

Closed

2 tasks

tobiasdiez added a commit that referenced this pull request Mar 11, 2018

Partial revert of #3715 since double click on group should expand/col…

4eb8950

…lapse it

tobiasdiez mentioned this pull request Mar 11, 2018

Partial revert of #3715 since double click on group should expand/collapse it #3834

Merged

6 tasks

Siedlerchr pushed a commit that referenced this pull request Mar 12, 2018

Partial revert of #3715 since double click on group should expand/col…

9b310a7

…lapse it (#3834)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Group interface: Usability tweaks #3715

Group interface: Usability tweaks #3715

Escoul commented Feb 13, 2018 •

edited

Loading

lenhard left a comment

lenhard Feb 13, 2018

Escoul Feb 13, 2018

lenhard Feb 13, 2018

Escoul Feb 13, 2018 •

edited

Loading

lenhard Feb 14, 2018

lenhard Feb 13, 2018

Escoul Feb 13, 2018

tobiasdiez left a comment •

edited

Loading

lenhard commented Feb 14, 2018

koppor commented Feb 14, 2018

Group interface: Usability tweaks #3715

Group interface: Usability tweaks #3715

Conversation

Escoul commented Feb 13, 2018 • edited Loading

lenhard left a comment

Choose a reason for hiding this comment

lenhard Feb 13, 2018

Choose a reason for hiding this comment

Escoul Feb 13, 2018

Choose a reason for hiding this comment

lenhard Feb 13, 2018

Choose a reason for hiding this comment

Escoul Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

lenhard Feb 14, 2018

Choose a reason for hiding this comment

lenhard Feb 13, 2018

Choose a reason for hiding this comment

Escoul Feb 13, 2018

Choose a reason for hiding this comment

tobiasdiez left a comment • edited Loading

Choose a reason for hiding this comment

lenhard commented Feb 14, 2018

koppor commented Feb 14, 2018

Escoul commented Feb 13, 2018 •

edited

Loading

Escoul Feb 13, 2018 •

edited

Loading

tobiasdiez left a comment •

edited

Loading