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

Fix binding issue for the regex and case sensitive search buttons #7125

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Fix binding issue for the regex and case sensitive search buttons #7125

merged 1 commit into from
Nov 26, 2020

Conversation

DominikVoigt
Copy link
Contributor

@DominikVoigt DominikVoigt commented Nov 24, 2020 •
edited
Loading

This PR fixes the issue related to the regex and case sensitive search buttons disappearing if the search bar is not empty. This addresses the issue raised by @k3KAW8Pnf7mkmdSMPHz27 in #7123.

Here how it currently looks if something else (WebSearchBar) is focused and the bar is nonempty:
image

In case of an empty search bar:
image

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

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member

Hi @DominikVoigt, thank you for looking at it!

A stylistic question (I am asking for your opinion on this, it is not a suggestion 😛 ), what are your thoughts on using something along the lines of searchField.textProperty().isEmpty() with focusBinding to make the icons show when there is an active search or when the field is focused? Feel free to ignore the question if you want to, I am already very happy that things gets fixed ^^

@DominikVoigt DominikVoigt force-pushed the fix-binding-for-search-bar branch from 71f3d4c to d4f82c0 Compare November 25, 2020 02:35
@DominikVoigt
Copy link
Contributor Author

Hey @k3KAW8Pnf7mkmdSMPHz27,
Thanks for your feedback!
I like the idea and implemented it as such.
The buttons are now visible if either the field is focused or the field is non-empty :)

LinusDietz
LinusDietz reviewed Nov 25, 2020
View reviewed changes
Copy link
Member

@LinusDietz LinusDietz left a comment

Choose a reason for hiding this comment

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

I've tried to replicate the problem mentioned in #6791, but it seems that in MacOS, the problem does not exist. If this solves the problem on other platforms, I would say code-wise it can be merged.

k3KAW8Pnf7mkmdSMPHz27
k3KAW8Pnf7mkmdSMPHz27 requested changes Nov 25, 2020
View reviewed changes
src/main/java/org/jabref/gui/search/GlobalSearchBar.java
Comment on lines -130 to -131
.or(regularExpressionButton.focusedProperty()
.or(caseSensitiveButton.focusedProperty()));
Copy link
Member

Choose a reason for hiding this comment

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

One nitpick, you need to re-add the focusedProperty() of regularExpressionButton and caseSensitiveButton with the new changes. Otherwise, they will disappear if you click on them while the search field is empty.

Sorry about making your life difficult =/

Except that it looks really good. I like this behaviour 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries!
Thank you for your feedback ^^
I now added the focused properties back in 😃!

@DominikVoigt DominikVoigt force-pushed the fix-binding-for-search-bar branch from d4f82c0 to 446c94b Compare November 25, 2020 13:00
@DominikVoigt DominikVoigt force-pushed the fix-binding-for-search-bar branch from 446c94b to 51a55b4 Compare November 25, 2020 13:01
@k3KAW8Pnf7mkmdSMPHz27

This comment has been minimized.

Sign in to view
@tobiasdiez
Copy link
Member

I think this behavior is by design. The point is that as you user you normally don't care about the regex toogle etc. Thus, to reduce visual clutter it is only shown when you focus the search bar (which is the only instance when you might care about it).

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member

k3KAW8Pnf7mkmdSMPHz27 commented Nov 25, 2020 •
edited
Loading

I believe a lot of this confusion is my fault, sorry about that.
This PR addresses the screenshot in #7123 (comment) , when the "X" (clear) in the search bar is shown but not the icons.
Skärmavbild 2020-11-24 kl 14 21 53

(which occurs when there is text in the search field, but it is not focused)

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Member

k3KAW8Pnf7mkmdSMPHz27 commented Nov 25, 2020 •
edited
Loading

I marked #7125 (comment) as off-topic (because it is), and if it serves a point, I can repost it with more details in #6791. I did not post it there since someone is already working on that issue, and therefore I did not think it would benefit from more screenshots.

@DominikVoigt
Copy link
Contributor Author

DominikVoigt commented Nov 25, 2020 •
edited
Loading

Yeah, this the current goal of this PR, it makes the regex and case sensitive search buttons visible if the search field is nonempty :)
I adapted the PR description accordingly :)
I hope this kinda clears the confusion? ^^

tobiasdiez
tobiasdiez approved these changes Nov 26, 2020
View reviewed changes
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.

Ok, thanks for the clarification. LGTM

@tobiasdiez tobiasdiez merged commit f1a2fa7 into JabRef:master Nov 26, 2020
Siedlerchr added a commit that referenced this pull request Dec 5, 2020
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into refactor-prefs-calls
0d03690 
* upstream/master: (36 commits)
  Fix remembering password for sql db (#7154)
  Update to libre office 7.0.3 (#7150)
  Add IdBasedSearchFetcher to jstor (#7145)
  Squashed 'src/main/resources/csl-styles/' changes from 55200d0..a20406d
  Bump antlr4-runtime from 4.8-1 to 4.9 (#7136)
  Bump antlr4 from 4.8-1 to 4.9 (#7138)
  Bump mariadb-java-client from 2.7.0 to 2.7.1 (#7134)
  Bump classgraph from 4.8.90 to 4.8.92 (#7139)
  Bump mockito-core from 3.6.0 to 3.6.28 (#7135)
  Bump gittools/actions from v0.9.6 to v0.9.7 (#7144)
  Bump checkstyle from 8.37 to 8.38 (#7142)
  Add missing author
  Fix document viewer not showing first page (#7132)
  Add githandler mock to crawler test to fix NPE (#7133)
  Searchbar glyph icon colors in Dark Theme [FIXED] (#7131)
  Fix binding issue for the regex and case sensitive search buttons (#7125)
  Enable automated cross library search using a cross library query lan… (#7124)
  Add tracking
  Update Java Version
  Welcome Dominik ✌
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LinusDietz LinusDietz LinusDietz left review comments

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 requested changes

@tobiasdiez tobiasdiez tobiasdiez approved these changes

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DominikVoigt @k3KAW8Pnf7mkmdSMPHz27 @tobiasdiez @Siedlerchr @LinusDietz