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

Add support for jumping in ordered author list by typing letters #6440

Merged

Conversation

leitianjian
Copy link
Contributor

Fixes #6146

I think I have added the support for jumping to the entry when typing letters.
Should I add some comments to the method I added?
Thanks :D

  • 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.

@leitianjian leitianjian marked this pull request as ready for review May 7, 2020 10:49
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 7, 2020
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

You can also shorten this with some lambda-expression:

        list.getItems().stream().filter(item -> item.getName().toLowerCase().startsWith(target_char))
            .findFirst().ifPresent(item -> { this.scrollTo(item); this.getSelectionModel().clearAndSelect(item); });

for an example, have a look at gui/preferences/PreviewTabView.java::jumpToSearchKey

src/main/java/org/jabref/gui/maintable/MainTable.java Outdated Show resolved Hide resolved
@calixtus
Copy link
Member

calixtus commented May 7, 2020

Hi @leitianjian , thank you very much for your contribution.
Yet I still have a question and a suggestion for your PR.

@leitianjian
Copy link
Contributor Author

Thanks for your suggestions. The method in jumpToSearchKey provides a very good template. I will rewrite the code and try to add test cases to test it.

@leitianjian leitianjian marked this pull request as draft May 8, 2020 12:59
@leitianjian
Copy link
Contributor Author

Hi, I think I have done it now. I used the stream to replace the for loop you have mentioned above, but I cannot construct a test case to test the method I added. I only test manually. I set the time to 700ms because I think 1s is too long for our typing.
Thanks in advance

@calixtus
Copy link
Member

calixtus commented May 8, 2020

Is this ready for review?

@calixtus
Copy link
Member

calixtus commented May 8, 2020

I only test manually. I set the time to 700ms because I think 1s is too long for our typing.

That's fine, There are many scenarios some gui stuff just cannot be tested.

@leitianjian leitianjian marked this pull request as ready for review May 8, 2020 15:15
@leitianjian
Copy link
Contributor Author

I only test manually. I set the time to 700ms because I think 1s is too long for our typing.

That's fine, There are many scenarios some gui stuff just cannot be tested.

Yeah, I found the sad truth finally T^T

Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
@leitianjian
Copy link
Contributor Author

@calixtus Thanks for your suggestions, I made a mistake when I adjust the code format and had not realized that.

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.

Have some code nitpicks. The acutal implementation should be (please) checked by @tobiasdiez

src/main/java/org/jabref/gui/maintable/MainTable.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/maintable/MainTable.java Outdated Show resolved Hide resolved
src/main/java/org/jabref/gui/maintable/MainTable.java Outdated Show resolved Hide resolved
@leitianjian leitianjian requested a review from koppor May 9, 2020 02:42
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.

LGTM. Thanks for your contribution!

(I didn't test if this interferes with typing in the entry editor or searchbar, but it shouldn't as the event is registered on the maintable).

@leitianjian
Copy link
Contributor Author

LGTM. Thanks for your contribution!

(I didn't test if this interferes with typing in the entry editor or searchbar, but it shouldn't as the event is registered on the maintable).

Yeah, I tested it manually, which will not interfere with the searchbar. The target of the input is depended on the focusing window

@calixtus
Copy link
Member

Ok, 2 approvals, @koppor s suggestions are fixed and the change is tested.
I'm going to merge this now.
@leitianjian , thank you very much for your contribution. I hope you liked working on JabRef and consider to contribute more.

@calixtus calixtus merged commit 100f731 into JabRef:master May 11, 2020
Siedlerchr added a commit that referenced this pull request May 11, 2020
…read

* upstream/master:
  Fix label name
  Add support for jumping in ordered author list by typing letters (#6440)
  Bump flexmark-ext-gfm-strikethrough from 0.61.24 to 0.61.26
  Bump org.beryx.jlink from 2.18.0 to 2.19.0
  Bump flexmark from 0.61.24 to 0.61.26
  Bump flexmark-ext-gfm-tasklist from 0.61.24 to 0.61.26
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

can't jump in ordered author list by typing letters
5 participants