-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
11802: add new KeyBinding for bibliographic data shortcut #11842
base: main
Are you sure you want to change the base?
11802: add new KeyBinding for bibliographic data shortcut #11842
Conversation
NO real changes made yet, just a hull for now, to see if my config works |
To ease organizational workflows I have linked the pull-request to the issue with syntax as described in https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue
Example:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
You can check review dog's comments at the tab "Files changed" of your pull request.
I added a standard action and copied the initKeybindings method (kinda) from JabRefFrame.java into IdentifierEditor.java, The issue with the current state is, that eventhough it works, you would need to focus the editor. In my oppinion, an open General-Tab should suffice. Is there a way how i can implement that with initKeybindings? Or is org.jabref.gui.actions.ActionFactory#configureIconButton the only correct way. |
@FXML | ||
private BaseIdentifierEditorViewModel<?> viewModel; | ||
@FXML | ||
private EditorTextArea textArea; | ||
@FXML | ||
private Button fetchInformationByIdentifierButton; | ||
@FXML | ||
private Button lookupIdentifierButton; | ||
|
||
@Inject | ||
private DialogService dialogService; | ||
@Inject | ||
private TaskExecutor taskExecutor; | ||
@Inject | ||
private GuiPreferences preferences; | ||
@Inject | ||
private UndoManager undoManager; | ||
@Inject | ||
private StateManager stateManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not reformat the code. All JabRef code should be consistently formatted. Check our guidelines for setting up a workspace. With that configuration, this should not happen.
addEventFilter(KeyEvent.KEY_PRESSED, event -> { | ||
Optional<KeyBinding> keyBinding = preferences.getKeyBindingRepository().mapToKeyBinding(event); | ||
if (keyBinding.isPresent() && keyBinding.get() == KeyBinding.GET_BIBLIOGRAPHIC_DATA) { | ||
this.fetchInformationByIdentifier(); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels wrong. All other keybindings work differently. You already correctly added it into KeyBinding
. - And you say, you need this peace of code here to get if fully working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koppor Just because I couldnt get it to work with org.jabref.gui.actions.ActionFactory#configureIconButton (locally).
And debugging it, didnt help either, i tried to understand it by using org.jabref.gui.groups.GroupDialogView#GroupDialogView (it configures a HelpAction) as a reference.
But also this doesnt seem to work for me locally.
When i have following modal open:
and press F1, nothing happens.
Then i stumpled upon org.jabref.gui.frame.JabRefFrame#initKeyBindings
which was the only occurence where I could observe a keybinding to be actually caught.
And then I copied that code and at least i now i have the wanted behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like the keyevent is consumed before reaching the Dialog... Maybe you can try to follow the keyevent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event doesnt Seem to be Consumed.
I also debugged a bit further, it seems like this issue affects all Buttons that are in some sort of a dialog.
Buttons that did not work:
autosaveLocalLibrariesHelp in org.jabref.gui.preferences.general.GeneralTab
remoteHelp in org.jabref.gui.preferences.general.GeneralTab
But there are also buttons that work that way, like:
generateCitationKeyButton in org.jabref.gui.fieldeditors.CitationKeyEditor
I Suspect that it matters where The Button is mounted.
But this for sure seems like a bug (in JabRef) to me, could some try to reproduced it to verify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a mac and there the KeyEvent is always going through the top menu, so F1 always triggers the "online help" menu item and thus opens the default help page.
So I cannot help here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll reset the branch and, try again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only hint I currently have is that one should try with a minimal JavaFX app. Maybe https://github.com/Siedlerchr/javafxreproducer?
.root(this) | ||
.load(); | ||
.root(this) | ||
.load(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please no code reformatting
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No double empty lines.
@@ -113,7 +113,7 @@ public enum StandardActions implements Action { | |||
ABBREVIATE_DOTLESS(Localization.lang("dotless"), Localization.lang("Abbreviate journal names of the selected entries (DOTLESS abbreviation)")), | |||
ABBREVIATE_SHORTEST_UNIQUE(Localization.lang("shortest unique"), Localization.lang("Abbreviate journal names of the selected entries (SHORTEST UNIQUE abbreviation)")), | |||
UNABBREVIATE(Localization.lang("Unabbreviate journal names"), Localization.lang("Unabbreviate journal names of the selected entries"), KeyBinding.UNABBREVIATE), | |||
|
|||
GET_BIBLIOGRAPHIC_DATA(Localization.lang("Get bibliographic data"), IconTheme.JabRefIcons.SEARCH,KeyBinding.GET_BIBLIOGRAPHIC_DATA), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix checkstyle: space messing
Fixes #11802
Resolves https://discourse.jabref.org/t/shortcuts-for-get-bibliographic-data-from-doi-and-look-up-doi/4468
Resolves https://discourse.jabref.org/t/how-to-assign-a-keyboard-shortcut-to-search-document-identifier-online-doi/5244
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)