Ported the KeyBindings dialog to JavaFX #1390

boceckts · 2016-05-12T13:45:36Z

The KeyBindings are now organized in categories referring to the JabRef menu titles.
The button graKey has been removed and once a entry is selected the table listens for valid key combinations. The FXAlert class has also been adjusted to listen to the closeDialog shortcut.

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)

Siedlerchr · 2016-05-12T15:54:50Z

UI Looks good 👍 Just one thing you should change regarding the UI: Don't ask questions in dialogs. Because that is too much "cognitive expenditure" for the user. Label the buttons with a clear description: (e.g. Reset Keybinding, Back to Dialog).
Maybe offer a button "Restart JabRef now", if this is technically possible.

See here for a discussion on Why Questions with Yes/No are a bad idea
http://ux.stackexchange.com/questions/9946/should-i-use-yes-no-or-ok-cancel-on-my-message-box

tschechlovdev · 2016-05-16T09:21:26Z

src/main/java/net/sf/jabref/gui/FXAlert.java

+            return true;
+        } else {
+            return false;
+        }


if statement is not necessary, this could be done in one line return combination.equals(pressedCombination)

tobiasdiez · 2016-05-20T10:03:16Z

A few comments regarding the UI:

Unify capitalization of headings (File vs EDIT), preferably via a global css property
Better formatting of shortcuts, i.e. Ctrl + C instead of ctrl C
Move Default button more to the left and rename it to something like Reset all
Rename OK button to Save
Remove light gray column separation
Increase space between list and the buttons at the bottom (are there standards for such things? research and document them)
The error/information dialogs look a bit bloated since they have two headings (in the dialog window header and in the first row next to the icon). If you can style these dialogs, I would prefer a flat layout without the window border (similar to https://developers.google.com/apps-script/images/style-dialogs-do.svg or https://debajitcrm.files.wordpress.com/2015/01/image_thumb7.png?w=757&h=396) or without the first row (https://i-msdn.sec.s-msft.com/dynimg/IC725781.png)

boceckts · 2016-05-25T15:10:21Z

@tobiasdiez I have now adressed most of your comments. I called the default button Reset binding since it resets the selected binding only, except there is none selected. Also I wasn't really sure what you meant with "remove light gray column separation".
Also the issue with blank symbols in localized strings in fxml files as described in #968 still exists. For this reason you will see a '_' in the screenshots below in the Reset binding button, otherwise the fxml file can't be loaded.

Here are some new screenshots

tobiasdiez · 2016-05-25T15:38:38Z

src/main/java/net/sf/jabref/gui/FXAlert.java

+     * @param keyEvent as KeEvent
+     * @return true if matching, else false
+     */
+    private boolean checkKeyCombinationEquality(KeyCombination combination, KeyEvent keyEvent) {


I would prefer if you move this code to the KeyBindingPreferences class. Than you can write something like if(Globals.getKeyPrefs().checkKeyCombinationEquality(KeyBinding.CLOSE_DIALOG, event)

tobiasdiez · 2016-06-04T12:12:26Z

src/main/java/net/sf/jabref/gui/keyboard/KeyBindingViewModel.java

+    private final KeyBindingCategory category;
+
+
+    public KeyBindingViewModel(KeyBinding keyBinding, String binding, KeyBindingCategory category) {


The third parameter is not required as far as I can see (if it's a binding, then its category is stored in it)

Yes you are right, I forgot to remove it when adding the category constructor.

tobiasdiez · 2016-06-04T13:05:10Z

Good work!
I just have a few remarks about the general usage of view models and databinding.

The idea of this ViewModels is the following:

Expose fields (as properties) so that the view can easily bind to them (but only expose things you really need in the view)
Listen to changes to the underlying data and update accordingly. These changes are then propagated to the view via the binding.
Expose methods which modify the underlying data, the view never calls the logic methods themselves.
In the best case scenario, you never access the view or its controls directly but for setting up the binding (so only in initialize()).

So in your case you have two ViewModels:
For the whole dialog:

Expose bindingsTree and selectedItem
Listen to external changes (which is setKeyBindingsRepository) and update bindingsTree then.
Expose methods for resetting all keys, save changes, closing the dialog and relaying key pressed events to the selected item

For the item in the table: (KeyBindingViewModel)

Expose displayName and binding
No need to listen for changes since keybindings cannot be modified except in this dialog
Expose methods for changing the keybinding

The reason for this "binding over everything else" philosophy is that you can easily write tests. For example:

KeyBindingDialogViewModel dialog = new blabla();
dialog.setRepository(some mocked rep);
AssertEqual(expectedTree, dialog.bindingsTree);
% If this test passes then you know that the correct things are displayed in the dialog (except if you forgot to bind the vew to `bindingsTree`)

KeyBindingDialogViewModel dialog = new blabla();
dialog.setRepository(some mocked rep);
AssertCalls(mockedRep.resetBindinings(), dialog.resetBinding());
AssertEquals(defaultTree, dialog.bindingsTree);
% Check that the dialog relays the click to the correct method and updates view (only thing that can fail is again that the binding of the buttonclick is not bound to  dialog.resetBinding()

KeyBindingDialogViewModel dialog = new blabla();
dialog.setRepository(some mocked rep);
dialog.selectedIBinding = some mocked binding view model
AssertCalls(mockedBinding.setNewBinding, dialog.keyPressed(event))

KeyBindingViewModel binding = new blabla();
AssertCalls(repository.change(), binding.setNewBinding(event))
AssertEquals("Ctrl + Alt + f4", binding.KeyBinding);

Can you please try to add these tests?

tobiasdiez · 2016-06-05T10:02:03Z

src/main/java/net/sf/jabref/gui/keyboard/KeyBindingsDialogView.java

+        controller.setKeyBindingPreferences(keyBindingPreferences);
+        controller.initializeView();
+        keyBindingsDialog.setResizable(true);
+        ((Stage) keyBindingsDialog.getDialogPane().getScene().getWindow()).setMinHeight(475);


Can this be set in the fxml file?

Well the minimum size of the dialog pane is already set to this dimension in the fxml file. However if you omit these lines you are able to resize the dialog window to whatever you like but not the dialog content. So all properties set in the fxml file only affect the dialog content but not the dialog window itself. With these lines omitted you could resize the dialog to a point where the content doesn't resize and it may look bugged.

boceckts · 2016-06-07T15:23:44Z

I added some tests for the dialog view model

tobiasdiez · 2016-06-07T15:57:39Z

src/test/java/net/sf/jabref/gui/keyboard/KeyBindingsDialogViewModelTest.java

+    @Before
+    public void setUp() {
+        model = new KeyBindingsDialogViewModel();
+        JabRefPreferences preferences = JabRefPreferences.getInstance();


Try to pass a mockito mock of the preferences to the keybindingpreferences (see wiki) or mock the repository itself.

tobiasdiez · 2016-06-07T16:22:49Z

Nice! I only have some minor remarks about the naming of the tests and their general structure (only one assert per test). If these small issues are fixed, this PR can be merged from my point of view!

boceckts · 2016-06-07T17:47:34Z

I refactored the test class.
Also I changed the FXDialogs to always show blocking dialogs and changed the way to create camel case bind names as there was a problem with the import of WordUtils.

tobiasdiez · 2016-06-07T18:57:28Z

src/test/java/net/sf/jabref/gui/keyboard/KeyBindingsDialogViewModelTest.java

        try {
-            preferences.clear();
+            mockedPreferences.clear();


No need to clear it.

tobiasdiez · 2016-06-07T19:00:24Z

Ok, ready to go from my point of view! @JabRef/developers what do you think?

tobiasdiez · 2016-06-26T17:20:47Z

Please rebase and remove the BibTeX localization key. Then I'll merge this in. (except if @JabRef/developers has more remarks)

codecov-io · 2016-06-27T11:53:40Z

Current coverage is 28.64%

Merging #1390 into javafx will increase coverage by 17.84%

@@             javafx      #1390   diff @@
==========================================
  Files           703        704      +1   
  Lines         46304      46395     +91   
  Methods           0          0           
  Messages          0          0           
  Branches       7634       7636      +2   
==========================================
+ Hits           5000      13288   +8288   
+ Misses        40857      31976   -8881   
- Partials        447       1131    +684

Powered by Codecov. Last updated by e22433f...15c7522

boceckts · 2016-06-27T12:01:18Z

@tobiasdiez The BibTeX localization key is used in the KeyBindingCategory class.

boceckts · 2016-06-27T13:54:29Z

I found out travis ci uses different import orders for javafx than what is specified in eclipse.settings. I quickly changed it in #1518 so using "organize imports" in eclipse will work for javafx.

koppor · 2016-08-08T18:53:30Z

Please be aware that menu translations are done using Localization.menuTitle and not by Localization.lang. See cb37296.

* Initial port of the keybindings dialog to javafx * Test javafx import order

boceckts added ui stupro labels May 12, 2016

boceckts changed the title ~~Ported the KeyBindings dialog to JavaFX~~ [WIP] Ported the KeyBindings dialog to JavaFX May 12, 2016

tschechlovdev reviewed May 16, 2016
View reviewed changes

tschechlovdev added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed stupro-ready-for-internal-review labels May 17, 2016

tschechlovdev changed the title ~~[WIP] Ported the KeyBindings dialog to JavaFX~~ Ported the KeyBindings dialog to JavaFX May 17, 2016

tobiasdiez reviewed May 25, 2016
View reviewed changes

tobiasdiez reviewed Jun 4, 2016
View reviewed changes

tobiasdiez reviewed Jun 5, 2016
View reviewed changes

tobiasdiez reviewed Jun 7, 2016
View reviewed changes

boceckts mentioned this pull request Jun 20, 2016

[WIP] Groups JavaFX Rework #1438

Closed

3 tasks

Initial port of the keybindings dialog to javafx

7564862

boceckts force-pushed the KeyBindingsFX branch from 15c7522 to 7564862 Compare June 27, 2016 11:53

Test javafx import order

642f6ba

boceckts force-pushed the KeyBindingsFX branch from a8662e1 to 642f6ba Compare June 27, 2016 13:17

tobiasdiez merged commit bc6f0c9 into JabRef:javafx Jun 27, 2016

boceckts deleted the KeyBindingsFX branch July 18, 2016 10:00

boceckts mentioned this pull request Aug 26, 2016

Implemented error console in JavaFX #1383

Merged

2 tasks

bruehldev pushed a commit to bruehldev/jabref that referenced this pull request Aug 30, 2016

Ported the KeyBindings dialog to JavaFX (JabRef#1390)

b431a6e

* Initial port of the keybindings dialog to javafx * Test javafx import order

boceckts mentioned this pull request Oct 26, 2016

Integrity hotkey #2198

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ported the KeyBindings dialog to JavaFX #1390

Ported the KeyBindings dialog to JavaFX #1390

boceckts commented May 12, 2016 •

edited

Loading

Siedlerchr commented May 12, 2016

tschechlovdev May 16, 2016

tobiasdiez commented May 20, 2016

boceckts commented May 25, 2016

tobiasdiez May 25, 2016

tobiasdiez Jun 4, 2016

boceckts Jun 4, 2016

tobiasdiez commented Jun 4, 2016 •

edited

Loading

tobiasdiez Jun 5, 2016

boceckts Jun 7, 2016

boceckts commented Jun 7, 2016

tobiasdiez Jun 7, 2016 •

edited

Loading

tobiasdiez commented Jun 7, 2016 •

edited

Loading

boceckts commented Jun 7, 2016

tobiasdiez Jun 7, 2016

tobiasdiez commented Jun 7, 2016

tobiasdiez commented Jun 26, 2016

codecov-io commented Jun 27, 2016 •

edited

Loading

boceckts commented Jun 27, 2016

boceckts commented Jun 27, 2016

koppor commented Aug 8, 2016

		private final KeyBindingCategory category;


		public KeyBindingViewModel(KeyBinding keyBinding, String binding, KeyBindingCategory category) {

Ported the KeyBindings dialog to JavaFX #1390

Ported the KeyBindings dialog to JavaFX #1390

Conversation

boceckts commented May 12, 2016 • edited Loading

Siedlerchr commented May 12, 2016

tschechlovdev May 16, 2016

Choose a reason for hiding this comment

tobiasdiez commented May 20, 2016

boceckts commented May 25, 2016

tobiasdiez May 25, 2016

Choose a reason for hiding this comment

tobiasdiez Jun 4, 2016

Choose a reason for hiding this comment

boceckts Jun 4, 2016

Choose a reason for hiding this comment

tobiasdiez commented Jun 4, 2016 • edited Loading

tobiasdiez Jun 5, 2016

Choose a reason for hiding this comment

boceckts Jun 7, 2016

Choose a reason for hiding this comment

boceckts commented Jun 7, 2016

tobiasdiez Jun 7, 2016 • edited Loading

Choose a reason for hiding this comment

tobiasdiez commented Jun 7, 2016 • edited Loading

boceckts commented Jun 7, 2016

tobiasdiez Jun 7, 2016

Choose a reason for hiding this comment

tobiasdiez commented Jun 7, 2016

tobiasdiez commented Jun 26, 2016

codecov-io commented Jun 27, 2016 • edited Loading

Current coverage is 28.64%

boceckts commented Jun 27, 2016

boceckts commented Jun 27, 2016

koppor commented Aug 8, 2016

boceckts commented May 12, 2016 •

edited

Loading

tobiasdiez commented Jun 4, 2016 •

edited

Loading

tobiasdiez Jun 7, 2016 •

edited

Loading

tobiasdiez commented Jun 7, 2016 •

edited

Loading

codecov-io commented Jun 27, 2016 •

edited

Loading