Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Emacs key bindings #6037

Merged
merged 20 commits into from
Nov 6, 2020
Merged

Implement Emacs key bindings #6037

merged 20 commits into from
Nov 6, 2020

Conversation

muachilin
Copy link
Contributor

@muachilin muachilin commented Feb 27, 2020

Implement Emacs key bindings (fixes #6017)

Emacs style key bindings are re-added to JabRef through the preferences
menu. The supported key bindings have feature parity with the previous
implementation in JabRef v<4, and additionally support any class that
extends TextInputControl. In practice, this means that the new
implementation supports both TextFields and TextAreas by default. Some
functionality may still be missing.

Co-authored-by: Felix Luthman 34520175+felixlut@users.noreply.github.com
Co-authored-by: Tommy Samuelsson Zodbigt@users.noreply.github.com
Co-authored-by: muachilin 32566798+muachilin@users.noreply.github.com
Co-authored-by: Kristoffer Gunnarsson kristoffergunnarsson47@gmail.com

stevensdavid and others added 2 commits February 27, 2020 18:30
Emacs style key bindings are re-added to JabRef through the preferences
menu. The supported key bindings have feature parity with the previous
implementation in JabRef v<4, and additionally support any class that
extends TextInputControl. In practice, this means that the new
implementation supports both TextFields and TextAreas by default. Some
functionality may still be missing

Co-authored-by: Felix Luthman <34520175+felixlut@users.noreply.github.com>
Co-authored-by: Tommy Samuelsson <Zodbigt@users.noreply.github.com>
Co-authored-by: muachilin <32566798+muachilin@users.noreply.github.com>
Co-authored-by: Kristoffer Gunnarsson <kristoffergunnarsson47@gmail.com>
@stevensdavid
Copy link
Contributor

I see we have some localization strings to fix, will do that soon.

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! I hope you enjoyed it so far!
In general looks good to me, I have just a few codestyle remarks.
Let's see what the others have to say :)

@koppor koppor changed the title [feat] Implement Emacs key bindings Implement Emacs key bindings Feb 27, 2020
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the work on that and to dive into the depth of JavaFX and keyboard handling.

  • I am sorry that I have you to point to org.jabref.logic.bst.BibtexCaseChanger#changeCase. That functionality really has to be used (instead of rewriting this upper/lower case thing).
  • I think, we did not have code for delete characters until beginning/end of the word. Thus, this really has to be reimplemented. I hope, you checked Google Guava and Apache Commons that there is not such a functionality. - Please add a comment on the implementation that you checked and since it did not exist, you implemented it.
  • I check in the entry editor. Ctrl+A works fine. Alt+B for going backword one word and Alt+F for going forward one word (same algorithm then with alt+d) did not work. Can you add them, too? (Semantics of emacs: Ctrl: One thing, Alt: the next semantic level... Thus, if Ctrl+B is one letter back, Alt+B is one word back).
  • Ctrl/kbd>+Z for "Undo" does not work anymore. Do you have an idea?

src/main/java/org/jabref/JabRefGUI.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/keyboard/KeyBinding.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/keyboard/KeyBinding.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/keyboard/KeyBinding.java Outdated Show resolved Hide resolved
Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like that you added the corresponding key bindings as normal keybindings in the dialog. This gives quite some flexibility and unified user experience. I would actually push this flexibility a bit further and remove the "Emacs" mode completely and simply let users manage the corresponding keybindings on their own. That is, doesn't limit the new keybindings to be used by emacs users. The bindings can be initialized with standard bindings if there are any (like Ctrl + right/left to jump one word) or left empty if they are normally not used but are somewhat emacs specific.

Reasoning: I guess not many people will use the emacs mode, but more users will be happy about the customization of additional keybindings and will adapt them to their liking.

@Siedlerchr @koppor @calixtus what you think?

@calixtus
Copy link
Member

calixtus commented Mar 1, 2020

I agree, since the special implementation of emacs bindings besides the normal KeyBindings introduces a lot of unneccessary complexity. If it's possible to implement the same functionality in the current KeyBindings framework - go for it!

@koppor
Copy link
Member

koppor commented Mar 1, 2020

In case we go that way, we need to have a button in the "customize key bindings" dialog to "Set Emacs keybindings", where a dialog pops up asking whether to rebind C-a and C-f. Maybe, put in the default action description for these: Select all text (Emacs: Go to the beginning of the line), Find text (Emacs: Move cursor one letter forward).

See #6037 (comment) for a screenshot for JabRef 4.3.1

See https://docs.jabref.org/setup/customkeybindings for the reset capability of the dialog.

@tobiasdiez
Copy link
Member

tobiasdiez commented Mar 1, 2020

@koppor why do we need this? Ok, it's a bit more work for someone using emacs bindings to initially configure the bindings by hand for each shortcut. But I would prefer to have less visual distractions for 99.9% of all users instead of a button for the rest. We are not an code editor that we need to support fancy shortcut systems. How many normal user applications actually let you configure keybindings in such a detail?

@koppor koppor force-pushed the master branch 5 times, most recently from b8ef7b7 to 21c6e5e Compare March 4, 2020 17:02
@koppor
Copy link
Member

koppor commented Mar 6, 2020

I would ❤️ to see the button "bash keybings". (I replaced "Emacs" by "bash", maybe this suits better?). See https://gist.github.com/tuxfight3r/60051ac67c5f0445efee for the list of supported key bindings. "By accident" they also work on Emacs.

I personally use the bash keybindings daily in my life. Either in git bash and in cmd.exe (by using clink). Thus, I am really used to them and would like to use them in my favorite literature management software.

Since we dropped the plugin system (#152) users demanding other keyboard shortcuts (Sublime?) can add a similar functionality.

@koppor koppor changed the title Implement Emacs key bindings [WIP] Implement Emacs key bindings Mar 6, 2020
@koppor koppor self-assigned this Mar 31, 2020
@tobiasdiez
Copy link
Member

@muachilin @stevensdavid It would be nice if you could implement the suggested changes so that we can get this feature into JabRef. Thanks!

Thus it would be nice if you could remove most of the emac's specific code and let the user decide about their preferred keybord shortcut for this. If you then still have energy left, a button in the keybinding dialog setting these keybindings to their default emacs value would be nice (and would make @koppor very happy).

@koppor koppor removed their assignment Apr 14, 2020
@tobiasdiez tobiasdiez marked this pull request as draft April 17, 2020 15:56
@koppor koppor changed the title [WIP] Implement Emacs key bindings Implement Emacs key bindings Apr 23, 2020
@koppor
Copy link
Member

koppor commented Jul 7, 2020

@muachilin We would really like to see that this features comes into JabRef. May I ask whether you can find some time to finish this PR? It seems to be close to get it done. We have resources to provide timely feedback.

@calixtus
Copy link
Member

calixtus commented Oct 22, 2020

Except KILL_WORD_BACKWARD everything seems to work now.
I'll see if i can refactor something, to simmplify the code a bit more...

@Siedlerchr
Copy link
Member

Please also check if they work on the code area tab, I noticed for the mac thing that it has some quirks

@calixtus
Copy link
Member

calixtus commented Nov 5, 2020

Worked a little bit on the key bindings dialog too.

grafik

@calixtus calixtus marked this pull request as ready for review November 5, 2020 21:40
@calixtus calixtus added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 5, 2020
@tobiasdiez
Copy link
Member

Thanks a lot for finishing this PR @calixtus! The code looks very good to me, and I have only a few comments - oh wait, I actually don't have any comments...

One suggestion for the UI: What do you think about replacing the preset Label + Dropdown + Load button combination by a MenuButton with label "Presets" and if you click on it you get the list of possible presets; once a preset is selected there it is automatically loaded. You could also try to put this button next to the "Reset Bindings"; or maybe even remove the "Reset bindings" and add a "Default" preset.

@calixtus
Copy link
Member

calixtus commented Nov 6, 2020

I guess a MenuButton should do the thing.

@calixtus
Copy link
Member

calixtus commented Nov 6, 2020

grafik

The clear bindings button to the bottom left is no standard button, but a JavaFX dialog ButtonType. So I cannot put there a custom ui element. Sorry for that.

@koppor
Copy link
Member

koppor commented Nov 6, 2020

Thank you so much for finishing this @calixtus

@koppor koppor merged commit 82e555d into JabRef:master Nov 6, 2020
Siedlerchr added a commit to JabRef/jabref-koppor that referenced this pull request Nov 11, 2020
* upstream/master: (164 commits)
  Bump lucene-queryparser from 8.6.3 to 8.7.0 (JabRef#7089)
  Fix endnote importer when keyword style is null (JabRef#7084)
  Rename Firefox extension file in Windows (JabRef#7092)
  Add support for Microsoft Edge browser in Windows and Linux builds (JabRef#7056)
  Bump unirest-java from 3.11.03 to 3.11.05 (JabRef#7087)
  Bump com.github.ben-manes.versions from 0.33.0 to 0.36.0 (JabRef#7088)
  Bump gittools/actions from v0.9.4 to v0.9.5 (JabRef#7091)
  Feature/add abstract field to complex search (JabRef#7079)
  Add missing authors
  Implement Emacs key bindings (JabRef#6037)
  Special field code maintenance (JabRef#7078)
  Follow up fix for 7077 Reset preview layouts on clear
  Fix preview settings not saved due to l10n (JabRef#7077)
  Add link to existing documentation of filed types
  Remove unused supportsPaging
  Squashed 'src/main/resources/csl-styles/' changes from 5c376b8..f4399aa
  Fix more 404 links
  Fix custom theme styles not applied to the entry preview (JabRef#7071)
  Fix Shared Database Tests (JabRef#7040)
  Bump byte-buddy-parent from 1.10.17 to 1.10.18 (JabRef#7059)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/DefaultInjector.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emacs Keyboard shortcuts for data entry/editing
6 participants