-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Update PreviewView to listen for library search queries
#11659
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
Conversation
koppor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small comment
| vBox.getChildren().add(label); | ||
| vBox.getChildren().add(preview); | ||
| this.setGraphic(vBox); | ||
| fieldValueLabel.setText(fieldValue + "\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be moved to the if block
Please swap the conditions - typically, one should have the positive case first. Alternatively, "fail fast". Keep the negative thing, but then do "return this;" and then without else do the rest.
The indent of line 37 "this.setGraphic" is off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be moved to the if block
In both cases it should be updated, as it is also used in the tooltipContent line 27.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, wow, something which is not directly seeable while browsing through the github diff 😅
| } | ||
|
|
||
| if (!registered) { | ||
| stateManager.activeSearchQuery(SearchType.NORMAL_SEARCH).addListener(listener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also remove the activeSearchQuery completely from the state manager?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still used by GlobalSearchBar because there is only one bar shared between the open libraries.
|
The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build. |
PreviewViewto listen for library search queries instead of theStateManagerproperty.shouldShowPreviewEntryTableTooltipwas disabled.Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)