Group interface: Usability tweaks#3715
Conversation
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
left a comment
There was a problem hiding this comment.
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.
| */ | ||
| 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"), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks, i think so too. The only alternative would have been to create a separate method just to determine the name of the frame.
| Save\ file=Save file | ||
| Edit\ file\ type=Edit file type | ||
|
|
||
| Add\ group=Add group |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm not happy with having the essentially same translation key twice, but I get your point. We can leave this as it is.
| row.setOnMouseClicked((mouseClickedEvent) -> { | ||
| if (mouseClickedEvent.getButton().equals(MouseButton.PRIMARY) | ||
| && (mouseClickedEvent.getClickCount() == 2)) { | ||
| GroupNodeViewModel groupToEdit = EasyBind.monadic(row.itemProperty()).getValue(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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! |
|
@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. 😇 |
…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
Fixes koppor#277
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.
Credit (for an exercise):
Rico Haas
David Zeck