Added preset for new entry keybindings and reintroduced defaults by calixtus · Pull Request #7705 · JabRef/jabref

calixtus · 2021-05-05T20:32:55Z

Added preset for new entry keybindings on public demand.

Fixes #7346
Fixes #7439

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

koppor · 2021-05-05T21:03:45Z

Open / Close is new:

New Preset:

koppor

Yeah. Rage fixed!

Siedlerchr · 2021-05-06T05:33:44Z

-    NEW_PHDTHESIS("New phdthesis", Localization.lang("New phdthesis"), "", KeyBindingCategory.BIBTEX),
-    NEW_PROCEEDINGS("New proceedings", Localization.lang("New proceedings"), "", KeyBindingCategory.BIBTEX),
-    NEW_UNPUBLISHED("New unpublished", Localization.lang("New unpublished"), "", KeyBindingCategory.BIBTEX),
+    NEW_INBOOK("New inbook", Localization.lang("New inbook"), "Ctrl+shift+I", KeyBindingCategory.BIBTEX),


I think ctrl needs to be lowercase5, otherwise the Mac replacement with option might not work

I have no idea, I just did it like the other keybindings... Could you test this please? Maybe this is an issue with other keys.

I test it now, works fine. ctrl is mapped to cmd

Siedlerchr · 2021-05-06T06:57:44Z

Opening editor works on mac with cmd + e now, but closing not

tobiasdiez

Some smaller comments, otherwise LGTM

tobiasdiez · 2021-05-06T07:53:40Z

-    NEW_PHDTHESIS("New phdthesis", Localization.lang("New phdthesis"), "", KeyBindingCategory.BIBTEX),
-    NEW_PROCEEDINGS("New proceedings", Localization.lang("New proceedings"), "", KeyBindingCategory.BIBTEX),
-    NEW_UNPUBLISHED("New unpublished", Localization.lang("New unpublished"), "", KeyBindingCategory.BIBTEX),
+    NEW_INBOOK("New inbook", Localization.lang("New inbook"), "ctrl+shift+I", KeyBindingCategory.BIBTEX),


Are these defaults really necessary? They were removed in a recent PR on purpose (because they are not often used). Users can still set keybindings themselves, right?

Carl and me discussed this in length yesterday, too.

I like convention over configuration. If I start a tool, I want to have it behaving "well". It should require configuration only for certain special cases.

In the concrete case, the key bindings are NOT used for some other cases (except the pull from shared database). Thus, I have a strong vote to add the non-conflicting key bindings back as default.

The other option is that I write a lengthy manual on key bindings and explain each (power) user why they have to configure key bindings. - IMHO JabRef should not require much documentation. Only for the hard cases.

See also #7439 and #7346. Thus, there are multiple users used to the key bindings.

There are too many entry types, especially if you use biblatex, to have keybindings for all types. Thus you need to make a cutoff somewhere, and in my opinion it should be sufficient to have bindings for the 5 most commonly used types. This would also be consistent with the "new entry" dialog where the following types are treated specially:

jabref/src/main/java/org/jabref/model/entry/types/BiblatexEntryTypeDefinitions.java

Line 446 in d9964c6

public static final List<BibEntryType> RECOMMENDED = Arrays.asList(ARTICLE, BOOK, INPROCEEDINGS, REPORT, MISC);

The "most commonly used types" are for me the ones where keybindings existed for more than a decade. IMHO there is no need to remove functionality in this case.

From this perspective, we shouldn't have changed the new entry dialog as well...

tobiasdiez · 2021-05-06T07:56:09Z

+
+    @Override
+    public String getName() {
+        return "New Entries";


I find "new entries" a bit confusing, maybe something longer that better explains the purpose? Should be localized as well.

Please make a suggestion. We did not come up with something better yet.

New Entry types ?

Something like "Bindings for creating all entry types"?

Remember this is just shown in the small menubutton below the the keybindings table

A long text would just repeat information the user already has and would be imho extremely annoying.

Yeah, I couldn't find a nice formulation that doesn't include "Bindings". But with "New entries" or "New entry types" it's really not clear what this is doing.

Siedlerchr · 2021-05-06T20:10:14Z

+        KEY_BINDINGS.put(KeyBinding.COPY_PREVIEW, "");
+
+        // Add new entry presets
+        KEY_BINDINGS.put(KeyBinding.NEW_ARTICLE, "Ctrl+shift+A");


to be consistent with the other keybindings, please lowercase the Ctrl

…ry-preset

calixtus · 2021-05-06T22:47:30Z

checkstyle failes:

Siedlerchr · 2021-05-07T15:04:46Z

Seem like someone was too fast with merging unit tests , I think Tobi fixed them a few minutes ago

koppor · 2021-05-07T19:30:09Z

Seem like someone was too fast with merging unit tests , I think Tobi fixed them a few minutes ago

Merged main. Hope, checkstyle will be fine 😇

Siedlerchr

form my point of view lgtm.
Maybe reword New entries simply to New Entry by type"?

calixtus · 2021-05-08T19:53:16Z

Before merging we should get to a conclusion about which shortcuts to keep.
Does Ctrl in uppercase work now or should i change them now?

calixtus · 2021-05-08T19:56:05Z

Also i don't really understand why on macos we need two different shortcuts, one for opening the entry editor and one for closing it. This needs to be fixed.

Siedlerchr · 2021-05-08T21:11:25Z

Before merging we should get to a conclusion about which shortcuts to keep.
Does Ctrl in uppercase work now or should i change them now?

Should be lowercase

jabref/src/main/java/org/jabref/gui/keyboard/KeyBindingRepository.java

Lines 127 to 129 in bb011c9

    
           if (OS.OS_X) { 
        
               binding = binding.replace("ctrl", "meta"); 
        
           }

Regarding the ctrl + E (which maps to cmd + e on mac) I think I found the issue and I remember it's a bug in javafx.
The event is fired twice for some reason. That's why after closing it immediately goes into the OpenEntryEditor again and reopens the entry editor.

Edit// Yep: https://bugs.openjdk.java.net/browse/JDK-8205915
This already was an issue with copy paste happening twice.

…ry-preset

* upstream/main: (71 commits) [Bot] Update CSL styles (#7735) Fix for issue 6966: open all files of multiple entries (#7709) Add simple unit tests (#7696) Add simple unit tests (#7543) Update check-outdated-dependencies.yml Added preset for new entry keybindings and reintroduced defaults (#7705) Select the entry which has smaller dictonary order when merge (#7708) Update CHANGELOG.md fix: make more fields, fomatters, ids and languages sorted by alphabetical order (#7717) Bump libreoffice from 7.1.2 to 7.1.3 (#7721) Bump unoloader from 7.1.2 to 7.1.3 (#7724) Bump org.beryx.jlink from 2.23.7 to 2.23.8 (#7723) Bump org.openjfx.javafxplugin from 0.0.9 to 0.0.10 (#7725) fix: make fields sorted by lexicographical order (#7711) Fix tests Refactoring existing unit tests (#7687) Refactoring and addition of unit tests (#7581) Refactor simple Unit Tests (#7571) Add simple unit tests (#7544) add and extend unit tests (#7685) ...

calixtus added 2 commits May 5, 2021 22:29

Added preset for new entry keybindings

2f11418

Added defaults

e358264

koppor added the component: keybinding label May 5, 2021

calixtus added 4 commits May 5, 2021 22:44

CHANGELOG.md

a628236

Fixed inconsistency

6ad98a9

CHANGELOG.md

46a7c6f

CHANGELOG.md

bf001e1

Checkstyle

bb52b54

calixtus changed the title ~~Added preset for new entry keybindings~~ Added preset for new entry keybindings and reintroduced defaults May 5, 2021

koppor approved these changes May 5, 2021

View reviewed changes

Siedlerchr requested changes May 6, 2021

View reviewed changes