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

Pressing "tab" when last textField of entry editor Tab is in focus will move to the next Tab. #12087

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Noah-Martin1
Copy link

@Noah-Martin1 Noah-Martin1 commented Oct 26, 2024

Pressing "tab" when the last TextField of the entry editor Tab is in focus will move to the next Tab.

Closes #11937

What I have done

  • Changed a method in EditorTextField, (EditorTextField()), that adds an EventFilter to the relative TextField, listening for "tab" presses. If "tab" is pressed it checks if the current TextField is the last one of its TabPane by calling EntryEditor.checkLastTextField. This method retrieves the last Field of the current TabPane and compares its name with the ID of the TextField. If true the TextField is the last one of its TabPane and the next TabPane will be selected.

The reason the ID of TextField should be equal to the name of its corresponding Field is because in classes, PersonsEditor, SimpleEditor and CitationKeyEditor the TextField created from the original Field has been set an ID of the Field's display name.

How this changes UI experience

After clicking a TextField and bringing it to focus, pressing "tab" will cycle through all the succeeding TextFields (the ones below it) until it reaches the last one and then it will switch TabPane to the next one toward the right. (see below)
Screenshot 2024-10-26 at 12 35 54 pm

Mandatory checks

  • I own the copyright of the code submitted and I licence it under the MIT license
  • 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 developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Noah Martin added 10 commits October 25, 2024 22:33
…ed fields" because the "CitationKey" isnt created in SimpleEditor.java and therefore hasn't had its textfield id updated.
…ed fields" because the "CitationKey" isnt created in SimpleEditor.java and therefore hasn't had its textfield id updated.
Copy link
Collaborator

@subhramit subhramit left a comment

Choose a reason for hiding this comment

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

Few requests on code quality.

Comment on lines +482 to +487
Field lastField = null;
for (Field field : shownFields) {
lastField = field;
}
if (textField != null && lastField != null) {
if (textField.getId() == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pease use Optionals in place of nulls.

@@ -472,4 +475,20 @@ public void nextPreviewStyle() {
public void previousPreviewStyle() {
this.previewTabs.forEach(OffersPreview::previousPreviewStyle);
}

public static boolean checkLastTextField(TabPane tabs, TextField textField) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make this non-static.

Copy link
Author

Choose a reason for hiding this comment

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

Hey,
Sorry, I'm not sure how. I believe I can make checkLastTextField non-static and use the tabbed TabPane in the EntryEditor instead. But in doing this, I would need to create a new instance of EntryEditor in EditorTextField which will require the following arguments: EntryEditor(LibraryTab libraryTab, UndoAction undoAction, RedoAction redoAction). -> which I'm not sure how to get from EditorTextField.
Please guide me how I could do this.
Thankyou!

Copy link
Member

Choose a reason for hiding this comment

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

Why would you need a new instance of an entry editor? We only have one.
How about this?

Copy link
Author

@Noah-Martin1 Noah-Martin1 Oct 26, 2024

Choose a reason for hiding this comment

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

If im calling the non-static method checkLastTextField that's in EntryEditor, that uses private TabPane tabbed from EditorTextField. It requires me to create a new instance of EntryEditor in EditorTextField. I believe
Screenshot 2024-10-26 at 11 56 59 pm

Copy link
Member

Choose a reason for hiding this comment

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

If you create a new instance of an EntryEditor your changes would not affect the one we are displaying. There is only one EntryEditor. You probably have to inject a reference to the object you need.
But I believe there is a more serious architectural flaw, if you are trying to get a reference to to the big container node.
Why would you need the entry editor? Why would you need the TabbedPane? And what exactly do you need from the TabbedPane?

Copy link
Member

@calixtus calixtus Oct 26, 2024

Choose a reason for hiding this comment

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

You can 'inject' a method selectNextTab or sthg as a functional interface if really necessary.

Copy link
Author

Choose a reason for hiding this comment

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

I need the TabPane to get the last Field of a tab and to select the next Tab

import org.jabref.gui.fieldeditors.contextmenu.EditorContextAction;
import org.jabref.gui.keyboard.KeyBindingRepository;

public class EditorTextField extends TextField implements Initializable, ContextMenuAddable {

public static TabPane tabs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make it non-static.

@@ -38,6 +49,10 @@ public EditorTextField(final String text) {
ClipBoardManager.addX11Support(this);
}

public static void entryContext(TabPane tab) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Non-static.

@koppor
Copy link
Member

koppor commented Oct 26, 2024

For reviewers - technical discussions were done at #11937 (comment), too.

@koppor koppor added the status: changes required Pull requests that are not yet complete label Oct 31, 2024
@subhramit
Copy link
Collaborator

subhramit commented Nov 25, 2024

@Noah-Martin1 hey, sorry that the PR got a little stale. Were you able to figure out a way to conform to the remaining review comments? Did you try injecting the methods you needed using functional interfaces as suggested by @calixtus ?

@Noah-Martin1
Copy link
Author

@Noah-Martin1 hey, sorry that the PR got a little stale. Were you able to figure out a way to conform to the remaining review comments? Did you try injecting the methods you needed using functional interfaces as suggested by @calixtus ?

Hi sorry I have been on break since, and haven’t had a chance to look at it. I will look into the suggestions in a couple weeks. Thankyou

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes required Pull requests that are not yet complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tab in the last text field in a tab should focus the next tab in the tabs of the entry editor.
4 participants