Skip to content

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

Merged
calixtus merged 16 commits into
JabRef:masterfrom
leitianjian:jump-in-ordered-author-list-by-typing-letters
May 11, 2020
Merged

Add support for jumping in ordered author list by typing letters#6440
calixtus merged 16 commits into
JabRef:masterfrom
leitianjian:jump-in-ordered-author-list-by-typing-letters

Conversation

@leitianjian

Copy link
Copy Markdown
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

@calixtus calixtus left a comment

Copy link
Copy Markdown
Member

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

Comment thread src/main/java/org/jabref/gui/maintable/MainTable.java Outdated
@calixtus

calixtus commented May 7, 2020

Copy link
Copy Markdown
Member

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

@leitianjian

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread src/main/java/org/jabref/gui/maintable/MainTable.java Outdated
@calixtus

calixtus commented May 8, 2020

Copy link
Copy Markdown
Member

Is this ready for review?

@calixtus

calixtus commented May 8, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@koppor koppor left a comment

Copy link
Copy Markdown
Member

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

Comment thread src/main/java/org/jabref/gui/maintable/MainTable.java Outdated
Comment thread src/main/java/org/jabref/gui/maintable/MainTable.java Outdated
Comment thread src/main/java/org/jabref/gui/maintable/MainTable.java Outdated
@leitianjian leitianjian requested a review from koppor May 9, 2020 02:42

@tobiasdiez tobiasdiez left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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