-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Area of preview on hover should be shrink to the size of the text displayed #14478
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
base: main
Are you sure you want to change the base?
Changes from all commits
c4db0b1
5dd02dd
48ee9d5
8d33da7
52c0f05
4459622
696639a
5f2074d
a0f688d
f2b66e7
4ad537b
19a80ad
4ab7398
71cc5f2
083e413
a5f412a
58e073b
7e52ab0
8fe53d8
ae2a21a
ad8c35c
dae4c68
5f9da86
1e220f0
59202f3
d4aa3cb
e3a824b
b20a978
7149bb1
eab90b3
8ce5c92
fd1290d
4c9de0e
b1a3b5a
3619d1b
59c42ff
fe245ff
a8b02aa
ab4baee
42a8e43
d898c3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -4,9 +4,12 @@ | |||
| import java.net.MalformedURLException; | ||||
| import java.util.List; | ||||
| import java.util.Objects; | ||||
| import java.util.Optional; | ||||
|
|
||||
| import javafx.beans.InvalidationListener; | ||||
| import javafx.beans.Observable; | ||||
| import javafx.beans.property.ReadOnlyDoubleProperty; | ||||
| import javafx.beans.property.ReadOnlyDoubleWrapper; | ||||
| import javafx.beans.property.SimpleStringProperty; | ||||
| import javafx.beans.property.StringProperty; | ||||
| import javafx.concurrent.Worker; | ||||
|
|
@@ -85,6 +88,8 @@ function getSelectionHtml() { | |||
| private @Nullable BibEntry entry; | ||||
| private PreviewLayout layout; | ||||
| private String layoutText; | ||||
| private final ReadOnlyDoubleWrapper contentHeight = new ReadOnlyDoubleWrapper(0); | ||||
| private final ReadOnlyDoubleWrapper contentWidth = new ReadOnlyDoubleWrapper(0); | ||||
|
|
||||
| public PreviewViewer(DialogService dialogService, | ||||
| GuiPreferences preferences, | ||||
|
|
@@ -105,8 +110,11 @@ public PreviewViewer(DialogService dialogService, | |||
| this.searchQueryProperty = searchQueryProperty; | ||||
| this.searchQueryProperty.addListener((_, _, _) -> highlightLayoutText()); | ||||
|
|
||||
| setFitToHeight(true); | ||||
| setFitToWidth(true); | ||||
| setFitToHeight(false); | ||||
| setFitToWidth(false); | ||||
|
|
||||
| setHbarPolicy(ScrollBarPolicy.NEVER); | ||||
| setVbarPolicy(ScrollBarPolicy.NEVER); | ||||
| previewView = WebViewStore.get(); | ||||
| setContent(previewView); | ||||
|
|
||||
|
|
@@ -144,9 +152,57 @@ private void configurePreviewView(ThemeManager themeManager) { | |||
| evt.preventDefault(); | ||||
| }, false); | ||||
| } | ||||
|
|
||||
| try { | ||||
| Object heightObj = previewView.getEngine().executeScript("document.getElementById('content').scrollHeight || document.body.scrollHeight"); | ||||
| Object widthObj = previewView.getEngine().executeScript("document.getElementById('content').scrollWidth || document.body.scrollWidth"); | ||||
|
Comment on lines
+157
to
+158
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure, but I think there should be a neater way to do this
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What the comment means - where did you get the hint from? What alternatives did you consider? |
||||
|
|
||||
| Double height = null; | ||||
| if (heightObj instanceof java.lang.Number heightNum) { | ||||
| height = heightNum.doubleValue(); | ||||
| } | ||||
|
|
||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No empty line here
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay |
||||
| Double width = null; | ||||
| if (widthObj instanceof java.lang.Number widthNum) { | ||||
| width = widthNum.doubleValue(); | ||||
| } | ||||
|
|
||||
| // Use a single runLater to update both properties at once. We store the | ||||
| // measured values as Optionals to make the intent explicit. | ||||
| Optional<Double> optHeight = Optional.ofNullable(height); | ||||
| Optional<Double> optWidth = Optional.ofNullable(width); | ||||
|
Comment on lines
+172
to
+173
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is more readable in this case. - you should use optionals in the whole method - and not switch from |
||||
|
|
||||
| javafx.application.Platform.runLater(() -> { | ||||
| optHeight.ifPresent(measuredHeight -> { | ||||
| contentHeight.set(measuredHeight); | ||||
| // small padding so the rendered content is not clipped at edges | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment is off by one line. Use "speaking" variable names: |
||||
| this.setPrefHeight(measuredHeight + 8); | ||||
| }); | ||||
| optWidth.ifPresent(measuredWidth -> { | ||||
| contentWidth.set(measuredWidth); | ||||
| this.setPrefWidth(measuredWidth + 8); | ||||
| }); | ||||
| }); | ||||
|
|
||||
| setHvalue(0); | ||||
| } catch (NullPointerException e) { | ||||
| LOGGER.debug("Null value encountered while computing preview content size", e); | ||||
| } catch (ClassCastException e) { | ||||
| LOGGER.debug("Unexpected type returned from JavaScript while computing preview content size", e); | ||||
| } catch (IllegalStateException e) { | ||||
| LOGGER.debug("JavaFX thread not ready while computing preview content size", e); | ||||
| } | ||||
| }); | ||||
| } | ||||
|
|
||||
| public ReadOnlyDoubleProperty contentHeightProperty() { | ||||
| return contentHeight.getReadOnlyProperty(); | ||||
| } | ||||
|
|
||||
| public ReadOnlyDoubleProperty contentWidthProperty() { | ||||
| return contentWidth.getReadOnlyProperty(); | ||||
| } | ||||
|
|
||||
| public void setLayout(PreviewLayout newLayout) { | ||||
| // Change listeners might set the layout to null while the update method is executing, therefore, we need to prevent this here | ||||
| if ((newLayout == null) || newLayout.equals(layout)) { | ||||
|
|
||||
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.
Why
MAX_VALUE?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.
We also noticed this flickering issue on hover. However, it does not come from our pull request: the same behavior is already present in the official version currently available for download.
video.mp4
Using Double.MAX_VALUE here is intentional. In a VBox, a Node expands to its
maximum width only if its maxWidth is large enough. By setting it to
Double.MAX_VALUE, we let JavaFX resize the label and the tooltip to the width
determined by the preview calculation and the layout, instead of forcing a fixed
max width. This avoids clipping long text values and ensures the tooltip adapts
to the content.
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.
How is this connected to the flickering issue? Can you clarify a bit more
Uh oh!
There was an error while loading. Please reload this page.
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's not connected i just give the answer to this question:
Why MAX_VALUE?
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.
Your answer starts with "We also noticed this flickering issue". Therefore Subhramit wondered...
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.
I cannot reproduce the situation again.
However, the popup is placed too far away when jumping:
Video.mp4