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.

Sign up for GitHub

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

Jump to bottom

11802: add new KeyBinding for bibliographic data shortcut #11842

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

11802: add new KeyBinding for bibliographic data shortcut #11842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
729aea6
11802: add new KeyBinding for bibliographic data shortcut
HeroxHeruxum Sep 27, 2024
1ea0720
11802: added new StandardAction GET_BIBLIOGRAPHIC_DATA, and keybindin…
HeroxHeruxum Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/gui/actions/StandardActions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Member

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

MANAGE_CUSTOM_EXPORTS(Localization.lang("Manage custom exports")),
MANAGE_CUSTOM_IMPORTS(Localization.lang("Manage custom imports")),
CUSTOMIZE_ENTRY_TYPES(Localization.lang("Customize entry types")),
Expand Down
64 changes: 41 additions & 23 deletions src/main/java/org/jabref/gui/fieldeditors/identifier/IdentifierEditor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
package org.jabref.gui.fieldeditors.identifier;

import java.util.Optional;

import javax.swing.undo.UndoManager;

import com.airhacks.afterburner.injection.Injector;
import com.airhacks.afterburner.views.ViewLoader;
import jakarta.inject.Inject;
import javafx.fxml.FXML;
import javafx.scene.Parent;
import javafx.scene.control.Button;
import javafx.scene.control.Tooltip;
import javafx.scene.input.KeyEvent;
import javafx.scene.layout.HBox;

import org.jabref.gui.DialogService;
import org.jabref.gui.StateManager;
import org.jabref.gui.autocompleter.SuggestionProvider;
Expand All @@ -18,33 +17,40 @@
import org.jabref.gui.fieldeditors.FieldEditorFX;
import org.jabref.gui.fieldeditors.contextmenu.DefaultMenu;
import org.jabref.gui.fieldeditors.contextmenu.EditorMenus;
import org.jabref.gui.keyboard.KeyBinding;
import org.jabref.gui.preferences.GuiPreferences;
import org.jabref.logic.integrity.FieldCheckers;
import org.jabref.logic.l10n.Localization;
import org.jabref.logic.util.TaskExecutor;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.field.Field;

import com.airhacks.afterburner.injection.Injector;
import com.airhacks.afterburner.views.ViewLoader;
import jakarta.inject.Inject;
import javax.swing.undo.UndoManager;
import java.util.Optional;

import static org.jabref.model.entry.field.StandardField.DOI;
import static org.jabref.model.entry.field.StandardField.EPRINT;
import static org.jabref.model.entry.field.StandardField.ISBN;
import static org.jabref.model.entry.field.StandardField.*;

public class IdentifierEditor extends HBox implements FieldEditorFX {

@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;
@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;
Comment on lines +35 to +53
Copy link
Member

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.


private Optional<BibEntry> entry = Optional.empty();

Expand All @@ -56,6 +62,9 @@ public IdentifierEditor(Field field,
// but we need the injected vars to create the viewmodels.
Injector.registerExistingAndInject(this);

initKeyBindings();


Comment on lines +66 to +67
Copy link
Member

Choose a reason for hiding this comment

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

No double empty lines.

switch (field) {
case DOI ->
this.viewModel = new DoiIdentifierEditorViewModel(suggestionProvider, fieldCheckers, dialogService, taskExecutor, preferences, undoManager, stateManager);
Expand All @@ -71,8 +80,8 @@ public IdentifierEditor(Field field,
}

ViewLoader.view(this)
.root(this)
.load();
.root(this)
.load();
Comment on lines -74 to +84
Copy link
Member

Choose a reason for hiding this comment

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

Please no code reformatting


textArea.textProperty().bindBidirectional(viewModel.textProperty());

Expand All @@ -90,6 +99,15 @@ public IdentifierEditor(Field field,
new EditorValidator(preferences).configureValidation(viewModel.getFieldValidator().getValidationStatus(), textArea);
}

private void initKeyBindings() {
addEventFilter(KeyEvent.KEY_PRESSED, event -> {
Optional<KeyBinding> keyBinding = preferences.getKeyBindingRepository().mapToKeyBinding(event);
if (keyBinding.isPresent() && keyBinding.get() == KeyBinding.GET_BIBLIOGRAPHIC_DATA) {
this.fetchInformationByIdentifier();
}
});
Comment on lines +103 to +108
Copy link
Member

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?

Copy link
Author

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:
grafik

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.

Copy link
Member

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?

Copy link
Author

@HeroxHeruxum HeroxHeruxum Oct 8, 2024 •
edited
Loading

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?

Copy link
Member

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

Copy link
Author

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.

Copy link
Member

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?

}

public BaseIdentifierEditorViewModel<?> getViewModel() {
return viewModel;
}
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/jabref/gui/keyboard/KeyBinding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
public enum KeyBinding {
EDITOR_DELETE("Delete", Localization.lang("Delete text"), "", KeyBindingCategory.EDITOR),
// DELETE BACKWARDS = Rubout
GET_BIBLIOGRAPHIC_DATA("Get bibliographic data", Localization.lang("Get bibliographic data"), "ctrl+shift+alt+B", KeyBindingCategory.EDITOR),
EDITOR_BACKWARD("Move caret left", Localization.lang("Move caret left"), "", KeyBindingCategory.EDITOR),
EDITOR_FORWARD("Move caret right", Localization.lang("Move caret right"), "", KeyBindingCategory.EDITOR),
EDITOR_WORD_BACKWARD("Move caret to previous word", Localization.lang("Move caret to previous word"), "", KeyBindingCategory.EDITOR),
Expand Down
Loading