Corrected shortcut #6960

Gena928 · 2020-09-29T09:21:32Z

Fixes #6867
A new shortcut combination added for "Pull changes from shared database", because current ctrl+shift+R is already assigned for "Create tech report" command.

Change in CHANGELOG.md described (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.

tobiasdiez · 2020-09-29T10:58:33Z

I would remove all the standard keybindings for https://github.com/JabRef/jabref/pull/6960/files#diff-920a3cf78140f82ffcd3ba5b495cf73fL49-L54. Then you can leave the pull changes keybinding at "ctrl + shift + r". What do you think?

Siedlerchr · 2020-09-29T11:00:55Z

I'm against removing the shortcuts, at least article should be kept. Let's say what the other @JabRef/developers say about this

tobiasdiez · 2020-09-29T11:04:15Z

Yes, article, book, new entry and new entry from plaintext should be kept. But masterthesis, phdthesis etc are not that important to have a default keybinding. (I don't suggest to completely remove these bindings, just their default value so that users can still activate them if they want).

Gena928 · 2020-10-05T07:53:28Z

@tobiasdiez ,
So the following "new" shortcuts will be removed (see gray color):

correct?

calixtus · 2020-10-05T10:27:29Z

I wouldn't remove them at all, but their default value.
So the user is still able to bind keys to the shortcuts, if he likes.

tobiasdiez · 2020-10-05T12:13:05Z

Yes, just provide an empty string as the third argument of these enum values, and include NEW_TECHREPORT as well.

Gena928 · 2020-10-05T13:27:48Z

Empty string doesn't works. )) When I try to open the menu, it shows an error.
I'll try to find the problem.

java.lang.StringIndexOutOfBoundsException: begin 0, end 1, length 0
at java.base/java.lang.String.checkBoundsBeginEnd(String.java:3756)
at java.base/java.lang.String.substring(String.java:1902)
at javafx.graphics/javafx.scene.input.KeyCombination.normalizeToken(KeyCombination.java:630)
at javafx.graphics/javafx.scene.input.KeyCombination.valueOf(KeyCombination.java:409)
at org.jabref@100.0.0/org.jabref.gui.keyboard.KeyBindingRepository.getKeyCombination(KeyBindingRepository.java:129)
at org.jabref@100.0.0/org.jabref.gui.keyboard.KeyBindingRepository.checkKeyCombinationEquality(KeyBindingRepository.java:140)
at org.jabref@100.0.0/org.jabref.gui.keyboard.KeyBindingRepository.mapToKeyBinding(KeyBindingRepository.java:116)
at org.jabref@100.0.0/org.jabref.gui.JabRefFrame.lambda$initKeyBindings$5(JabRefFrame.java:236)
at javafx.base/com.sun.javafx.event.CompositeEventHandler$NormalEventFilterRecord.handleCapturingEvent(CompositeEventHandler.java:321)
at javafx.base/com.sun.javafx.event.CompositeEventHandler.dispatchCapturingEvent(CompositeEventHandler.java:98)
at javafx.base/com.sun.javafx.event.EventHandlerManager.dispatchCapturingEvent(EventHandlerManager.java:221)
at javafx.base/com.sun.javafx.event.EventHandlerManager.dispatchCapturingEvent(EventHandlerManager.java:180)
at javafx.base/com.sun.javafx.event.CompositeEventDispatcher.dispatchCapturingEvent(CompositeEventDispatcher.java:43)
at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:52)
at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
at javafx.base/com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
at javafx.base/com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
at javafx.base/com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
at javafx.base/com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
at javafx.base/javafx.event.Event.fireEvent(Event.java:198)
at javafx.graphics/javafx.scene.Scene$KeyHandler.process(Scene.java:4098)
at javafx.graphics/javafx.scene.Scene.processKeyEvent(Scene.java:2157)
at javafx.graphics/javafx.scene.Scene$ScenePeerListener.keyEvent(Scene.java:2625)
at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler$KeyEventNotification.run(GlassViewEventHandler.java:217)
at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler$KeyEventNotification.run(GlassViewEventHandler.java:149)
at java.base/java.security.AccessController.doPrivileged(AccessController.java:391)
at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler.lambda$handleKeyEvent$1(GlassViewEventHandler.java:248)
at javafx.graphics/com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:412)
at javafx.graphics/com.sun.javafx.tk.quantum.GlassViewEventHandler.handleKeyEvent(GlassViewEventHandler.java:247)
at javafx.graphics/com.sun.glass.ui.View.handleKeyEvent(View.java:547)
at javafx.graphics/com.sun.glass.ui.View.notifyKey(View.java:971)

koppor · 2020-10-12T21:17:18Z

@Gena928 You pasated a PNG instead of using a link into the code (offered by the GitHub UI) (or maybe additoinally)

The affected keys are at org.jabref.gui.keyboard.KeyBinding#NEW_MASTERSTHESIS.

Can't we just add alt+ at them to a) provide a shortcut (seems to be required according to the comment) and b) having a default short cut surely not conflicting?

@Gena928 While you are on it, could you please add a shortcot for New inproceedings? In computer science we use that type in 80% of our cases and that would make JabRef more comfortable to use.

Gena928 · 2020-10-13T06:27:11Z

@koppor
I'm very sorry for delay with this Issue. I'll try to fix it today, including shortcut for New inprocessing.

tobiasdiez · 2020-10-13T10:54:11Z

@Gena928 You should be able to fix this error by adding a few additional if-statements if the keybinding string is empty.

Gena928 · 2020-10-13T12:08:35Z

@tobiasdiez , @koppor Please review changes.

I solved the problem with empty defaultKeyBinding string by adding an if statement in KeyBindingRepository.java.
Key combinations for NEW_INBOOK...NEW_TECHREPORT changed from ctrl+.... to alt+....
I think now we can keep ctrl+shift+R for PULL_CHANGES_FROM_SHARED_DATABASE.
As it was initially. What do you think?
I also created NEW_INPROCEEDINGS KeyBinding.
But I cant't find this menu item in createMenu() method of JabRefFrame class. ))) Sorry, need help. Do we have such menu item? Where? )))

koppor · 2020-10-14T05:28:57Z

@tobiasdiez , @koppor Please review changes.

Could you please resolve the conflicts. Please merge upstream/master.

1. I solved the problem with empty `defaultKeyBinding` string by adding an `if` statement in `KeyBindingRepository.java`.

LGTM.

2. Key combinations for NEW_INBOOK...NEW_TECHREPORT changed from `ctrl+....` to `alt+....`

It was on purpose to add an additional modifier and not to change the modifier. I was thinking that an additoinal moddifier is easer to remember instead of another modifier.

However, having our discussion in mind, we should just leave these shortcuts empty. A user seldomly will use these shortcuts.

3. I think now we can keep `ctrl+shift+R` for PULL_CHANGES_FROM_SHARED_DATABASE.
   As it was initially. What do you think?

That's good. Please revert your change to change the modifier to alt+shift+D.

4. I also created NEW_INPROCEEDINGS KeyBinding.

Looks good.

   But I cant't find this menu item in `createMenu()` method of `JabRefFrame` class. ))) Sorry, need help. Do we have such menu item? Where? )))

I don't quite get what you are referring to with "this menu item". In the normal entry creation menu, it is already there: https://docs.jabref.org/collect/add-entry-manually I think, your current code enough.

I think, it should also be added to org.jabref.gui.fieldeditors.TypeEditorViewModel#TypeEditorViewModel

Corrected TypeEditorViewModel & KeyBinding

Gena928 · 2020-10-14T07:18:37Z

@koppor
Done. Sorry, for "menu item". I thought you can attach key bindings only to menu items, but now I know about keys for https://docs.jabref.org/collect/add-entry-manually. New InProceedings works via shortcut alt+shift+C.

Please review org.jabref.gui.fieldeditors.TypeEditorViewModel. I added new line there, but I still can't understand where and how it's used. ))) Can you give a small comment?

src/main/java/org/jabref/gui/fieldeditors/TypeEditorViewModel.java

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

tobiasdiez · 2020-10-14T08:08:20Z

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

@@ -113,8 +113,10 @@ public int size() {

 public Optional<KeyBinding> mapToKeyBinding(KeyEvent keyEvent) {
 for (KeyBinding binding : KeyBinding.values()) {
- if (checkKeyCombinationEquality(binding, keyEvent)) {
- return Optional.of(binding);
+ if (!binding.getDefaultKeyBinding().isEmpty()){


Are you sure this works? I would gess getDefaultKeyBinding is always empty for NEW_INBOOK even though the user has overwritten it. On first glance, I would suggest to change the getKeyCombination method below to public Optional<KeyCombination> getKeyCombination(KeyBinding bindName) and return an empty optional if binding is empty.

Moreover, get should probably return en empty string instead of "Not associated" to be consistent.

It works. If I change the next statement
from public KeyCombination getKeyCombination(KeyBinding bindName)
to Optional<KeyCombination> getKeyCombination(KeyBinding bindName)

I have to change other parts of code. For example line 141
from KeyCombination keyCombination = getKeyCombination(binding);
to KeyCombination keyCombination = getKeyCombination(binding).get();

Is this what you mean?

Okay, can you briefly explain why it works? I thought "getDefaultKeyBinding" then always returns an empty string, so this check is always false.

Yeah, there are more code changes required for my suggestion. I think it would provide a clean solution to handle the case that for a given keybinding there is not necessarily a keycombination. But I didn't had the time yet to check the code in detail. So if your solution indeed works, then it's fine with me as well.

I started debugger and see that it always returns shortcut from KeyBinding.java file.

Solution with Optional looks more clean. I just want to confirm that if we implement it, we also change other parts of code to getKeyCombination(binding).get().

In case this is done, the code fragment is getKeyCombination(binding).ifpresent(combination -> ...)

@Gena928 Exactly! So if you change the keybinding for "New inbook" to something, then getDefaultKeybinding should still return an empty string, so the code never goes into the if body in this case (although it should to match the new-user defined key).

@tobiasdiez
I agree. If you change default shortcut, this code will not work.

I'll try to fix today.

koppor · 2020-10-14T08:30:42Z

@Gena928 Please try to merge upstream/master to resolve the merge conflicts.

# Conflicts: # src/main/resources/l10n/JabRef_en.properties

…'s dependencies.

Gena928 · 2020-10-15T18:41:21Z

@koppor , @tobiasdiez ,
please review changes. Sorry for delay - I'm trying to learn Java with mentor and become valuable contributor )). So thank you for your comments here. They really help.
I changed getKeyCombination method of KeyBindingRepository class and all it's dependencies.

Also please give your comments for TypeEditorViewModel class - this comment from @tobiasdiez

calixtus

Good work so far, keep on coding and learning.
Got three little remarks.

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

CHANGELOG.md

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

Gena928 · 2020-10-18T18:09:17Z

@calixtus ,
thanks for comments. Please review changes.

calixtus

Still three issues. ;-)

CHANGELOG.md

src/main/resources/l10n/JabRef_en.properties

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

Gena928 · 2020-10-18T18:43:39Z

@calixtus
thanks again, corrected.
Looks like my "CheckStyle" was disabled. Maybe because of Intellij update?

calixtus

LGTM to me now. Thanks for your efforts!

koppor · 2020-10-18T21:50:10Z

Also OK for me. Still have to update CHANGELOG.md, because keyboard shortcuts were removed in this PR.

Corrected shortcut

5158e87

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 29, 2020

6796 fixed

62a4fa8

Added new entry in JabRef_en.properties file

692b398

Corrected TypeEditorViewModel & KeyBinding

tobiasdiez requested changes Oct 14, 2020

View reviewed changes

Gena928 added 3 commits October 14, 2020 11:33

Merge remote-tracking branch 'JabRefMaster/master' into 6867(2)

87df8c7

# Conflicts: # src/main/resources/l10n/JabRef_en.properties

Save before making changes

bf6c0f7

Changed "getKeyCombination" method of KeyBindingRepository and all it…

3c3f918

…'s dependencies.

calixtus requested changes Oct 18, 2020

View reviewed changes

src/main/java/org/jabref/gui/keyboard/KeyBindingRepository.java Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

src/main/java/org/jabref/gui/keyboard/KeyBinding.java Show resolved Hide resolved

Corrected pull requests (comments from @calixtus)

d480045

calixtus requested changes Oct 18, 2020

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

src/main/resources/l10n/JabRef_en.properties Outdated Show resolved Hide resolved

src/main/java/org/jabref/gui/keyboard/KeyBindingRepository.java Outdated Show resolved Hide resolved

Corrected pull requests (comments from @calixtus)

3d120ac

calixtus added 2 commits October 18, 2020 21:02

Merge branch 'master' into 6867(2)

3c9981c

Add whitespace in KeyBindingRepository

2abea9e

calixtus approved these changes Oct 18, 2020

View reviewed changes

koppor approved these changes Oct 18, 2020

View reviewed changes

koppor merged commit d5922d9 into JabRef:master Oct 18, 2020

koppor mentioned this pull request Jan 14, 2021

Some shortcuts are not working #7346

Closed

koppor mentioned this pull request Feb 15, 2021

Shortcut ctrl+shift+A not working #7439

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Corrected shortcut #6960

Corrected shortcut #6960

Gena928 commented Sep 29, 2020

tobiasdiez commented Sep 29, 2020

Siedlerchr commented Sep 29, 2020

tobiasdiez commented Sep 29, 2020

Gena928 commented Oct 5, 2020

calixtus commented Oct 5, 2020 •

edited

Loading

tobiasdiez commented Oct 5, 2020

Gena928 commented Oct 5, 2020

koppor commented Oct 12, 2020

Gena928 commented Oct 13, 2020

tobiasdiez commented Oct 13, 2020

Gena928 commented Oct 13, 2020 •

edited

Loading

koppor commented Oct 14, 2020

Gena928 commented Oct 14, 2020

tobiasdiez Oct 14, 2020

Gena928 Oct 14, 2020

tobiasdiez Oct 14, 2020

Gena928 Oct 14, 2020

koppor Oct 14, 2020

tobiasdiez Oct 14, 2020

Gena928 Oct 14, 2020

Gena928 Oct 15, 2020

koppor commented Oct 14, 2020

Gena928 commented Oct 15, 2020

calixtus left a comment

Gena928 commented Oct 18, 2020

calixtus left a comment

Gena928 commented Oct 18, 2020 •

edited

Loading

calixtus left a comment

koppor commented Oct 18, 2020

Corrected shortcut #6960

Corrected shortcut #6960

Conversation

Gena928 commented Sep 29, 2020

tobiasdiez commented Sep 29, 2020

Siedlerchr commented Sep 29, 2020

tobiasdiez commented Sep 29, 2020

Gena928 commented Oct 5, 2020

calixtus commented Oct 5, 2020 • edited Loading

tobiasdiez commented Oct 5, 2020

Gena928 commented Oct 5, 2020

koppor commented Oct 12, 2020

Gena928 commented Oct 13, 2020

tobiasdiez commented Oct 13, 2020

Gena928 commented Oct 13, 2020 • edited Loading

koppor commented Oct 14, 2020

Gena928 commented Oct 14, 2020

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

koppor commented Oct 14, 2020

Gena928 commented Oct 15, 2020

calixtus left a comment

Choose a reason for hiding this comment

Gena928 commented Oct 18, 2020

calixtus left a comment

Choose a reason for hiding this comment

Gena928 commented Oct 18, 2020 • edited Loading

calixtus left a comment

Choose a reason for hiding this comment

koppor commented Oct 18, 2020

calixtus commented Oct 5, 2020 •

edited

Loading

Gena928 commented Oct 13, 2020 •

edited

Loading

Gena928 commented Oct 18, 2020 •

edited

Loading