Re: RFR: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly [v5]
On Fri, 24 Feb 2023 06:26:21 GMT, Karthik P K wrote: > > I think this class may benefit from a few tests that test with a very wide > > caret, to see if positioning is what you'd expect in those cases as well. I > > get the impression a lot of the code assumes a narrow caret (1 or 2 pixels) > > and this is why we see constants like `0` and `1` in the code, and even > > places where the caret width should be subtracted but isn't because it is > > assumed to be `0`. > > Since caret width is hard coded and there is no direct way of changing the > width, I believe we can proceed without adding tests with wide caret. Yeah, that's okay; it seems to make a lot of assumptions about the caret, even going as far as ignoring a caret with exactly 4 path elements (as apparently there is no way to ask if it is a split caret when RTL/LTR mixed text is encountered). I guess those will need to tackled if we ever change the caret shape. - PR: https://git.openjdk.org/jfx/pull/980
Re: RFR: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly [v5]
On Thu, 23 Feb 2023 16:29:29 GMT, Andy Goryachev wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
>> line 805:
>>
>>> 803: // appear at the left of the centered prompt.
>>> 804: newX = midPoint -
>>> promptNode.getLayoutBounds().getWidth() / 2;
>>> 805: if (newX > 0) {
>>
>> Let's say `caretWidth / 2` is 5, and that would be position we would show
>> the prompt text if it doesn't fit.
>>
>> If the `newX` calculation is less than 5, but greater than 0, the prompt
>> text would be further to the left than you'd expect.
>>
>> So I think this line should be:
>>
>> Suggestion:
>>
>> if (newX > caretWidth / 2) {
>>
>>
>> Probably best to put that in a local variable.
>
> this is undocumented, I think, but the caret path can be one of the three
> things:
> 1. a single point (MoveTo, I think)
> 2. a single line - MoveTo followed by a LineTo
> 3. two separate lines (split caret) - MoveTo, LineTo, MoveTo, LineTo (split
> caret is explicitly ignored on line 252)
>
> One would think that the caret width can be changed by CSS, but it is not
> easy. The caretPath on TextInputControl:184 has no CSS class name set, so
> the width cannot be changed trivially via a stylesheet. I suppose the
> application code can dig the internals looking for paths and setting widths
> manually, but this would be a highly unreliable move.
>
> So I think this caretWidth can be other than 1.0 (line 233) for the purposes
> of this PR.
> Thanks for these details @andy-goryachev-oracle . I was trying to figure out
> how I can change the caret width. Just to check the behavior of text
> alignment, I changed the hard coded value at line no 233.
>
> > Let's say `caretWidth / 2` is 5, and that would be position we would show
> > the prompt text if it doesn't fit.
> > If the `newX` calculation is less than 5, but greater than 0, the prompt
> > text would be further to the left than you'd expect.
> > So I think this line should be:
> > Probably best to put that in a local variable.
>
> The newX is calculated symmetrically from midpoint using the text length. If
> we consider the condition `newX > careWidth/2`, we will be aligning the text
> at the position of `caretWidth/2` even when there is space for the whole
> text. Which means that the text will be aligned to left even when there is
> space for whole text with CENTER alignment. So in my opinion, current
> conditions and values gives better alignment behavior.
Alright, thanks for checking this.
-
PR: https://git.openjdk.org/jfx/pull/980
Re: RFR: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly
On Thu, 23 Feb 2023 17:40:05 GMT, Andy Goryachev wrote: > It might behave differently when the caret is at the rightmost position next > to the control edge, in which case the behavior is correct. Perhaps there > ought to be some conditional logic implemented, but the way it behaves now > when the caret is close to the left edge feels unnatural. > Yes this behavior looks unnatural. I see this issue in the mainline as well. Basically this behavior is observed because of the logic that, when we start typing in text field with RIGHT alignment, left side of the text is scrolled. So from whichever position we start inserting new characters, left side of the text gets scrolled. Because of the same behavior the newly inserted text scrolls to the left when we start typing from the first visible character. I will analyze a bit more on this behavior. - PR: https://git.openjdk.org/jfx/pull/980
Re: RFR: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly [v5]
On Thu, 23 Feb 2023 09:20:26 GMT, John Hendrikx wrote: > I think this class may benefit from a few tests that test with a very wide > caret, to see if positioning is what you'd expect in those cases as well. I > get the impression a lot of the code assumes a narrow caret (1 or 2 pixels) > and this is why we see constants like `0` and `1` in the code, and even > places where the caret width should be subtracted but isn't because it is > assumed to be `0`. Since caret width is hard coded and there is no direct way of changing the width, I believe we can proceed without adding tests with wide caret. - PR: https://git.openjdk.org/jfx/pull/980
Re: RFR: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly [v5]
On Thu, 23 Feb 2023 09:11:56 GMT, John Hendrikx wrote: >> Karthik P K has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix text and prompt alignment issue > > modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java > line 831: > >> 829: // Align to left when prompt text length is more >> than text field width >> 830: promptNode.setLayoutX(caretWidth / 2); >> 831: } > > Similar comment, I don't think its correct. It's just odd that promptNewX > may be smaller than caretWidth / 2 but is accepted, but when the text is too > wide the "minimum" value suddently is caretWidth / 2. > > Also, I don't see how `promptOldX` has any relevance when it comes to > positioning the prompt. I think this is only relevant for the actual text > (because it can scroll depending on where the cursor is), not for the prompt > -- unless the prompt can be scrolled as well. If that's the case then the > `CENTER` case may need to be updated to take `promptOldX` into account as > well. As it stands currently, they have behaviors that seem to conflict. Yes `promptOldX` is not required in case of prompt text. Code is updated. Same comment as the first one for the condition while comparing newly calculated value with `careWidth/2`. - PR: https://git.openjdk.org/jfx/pull/980
Re: RFR: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly [v5]
On Thu, 23 Feb 2023 16:34:11 GMT, Andy Goryachev wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
>> line 818:
>>
>>> 816: } else if (newX < 0 && oldX > 1) {
>>> 817: textTranslateX.set(caretWidth / 2);
>>> 818: }
>>
>> I get the impression this code also needs to use `caretWidth / 2` instead of
>> `0` and `1`.
>>
>> ie:
>>
>> Suggestion:
>>
>> // Update if there is space on the right
>> if (newX + textNodeWidth <= textRight.get() - caretWidth /
>> 2) {
>> textTranslateX.set(newX);
>> } else if (newX < caretWidth / 2 && oldX > caretWidth / 2) {
>> textTranslateX.set(caretWidth / 2);
>> }
>>
>>
>> It would only work for very narrow caret's otherwise, and for wide caret's
>> it would have some unexpected positioning in the edge case where text almost
>> fits.
>
> I would also recommend testing the code on Windows with the screen scale set
> to 225%, as it might show issues related to fractional scale.
> I get the impression this code also needs to use `caretWidth / 2` instead of
> `0` and `1`.
>
Here also same comment as above for the `newX < caretWidth/2` condition. But
while comparing the `oldX`, `caretWidth/2` should be considered instead of 1
and also `caretWidth / 2` is subtracted from `textRight` value in the previous
if condition as you suggested. These updates have been done in the code.
> I would also recommend testing the code on Windows with the screen scale set
> to 225%, as it might show issues related to fractional scale.
I will test this latest code updates.
-
PR: https://git.openjdk.org/jfx/pull/980
Re: RFR: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly [v5]
On Thu, 23 Feb 2023 16:29:29 GMT, Andy Goryachev wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
>> line 805:
>>
>>> 803: // appear at the left of the centered prompt.
>>> 804: newX = midPoint -
>>> promptNode.getLayoutBounds().getWidth() / 2;
>>> 805: if (newX > 0) {
>>
>> Let's say `caretWidth / 2` is 5, and that would be position we would show
>> the prompt text if it doesn't fit.
>>
>> If the `newX` calculation is less than 5, but greater than 0, the prompt
>> text would be further to the left than you'd expect.
>>
>> So I think this line should be:
>>
>> Suggestion:
>>
>> if (newX > caretWidth / 2) {
>>
>>
>> Probably best to put that in a local variable.
>
> this is undocumented, I think, but the caret path can be one of the three
> things:
> 1. a single point (MoveTo, I think)
> 2. a single line - MoveTo followed by a LineTo
> 3. two separate lines (split caret) - MoveTo, LineTo, MoveTo, LineTo (split
> caret is explicitly ignored on line 252)
>
> One would think that the caret width can be changed by CSS, but it is not
> easy. The caretPath on TextInputControl:184 has no CSS class name set, so
> the width cannot be changed trivially via a stylesheet. I suppose the
> application code can dig the internals looking for paths and setting widths
> manually, but this would be a highly unreliable move.
>
> So I think this caretWidth can be other than 1.0 (line 233) for the purposes
> of this PR.
Thanks for these details @andy-goryachev-oracle . I was trying to figure out
how I can change the caret width. Just to check the behavior of text alignment,
I changed the hard coded value at line no 233.
> Let's say `caretWidth / 2` is 5, and that would be position we would show the
> prompt text if it doesn't fit.
>
> If the `newX` calculation is less than 5, but greater than 0, the prompt text
> would be further to the left than you'd expect.
>
> So I think this line should be:
>
> Probably best to put that in a local variable.
The newX is calculated symmetrically from midpoint using the text length. If we
consider the condition `newX > careWidth/2`, we will be aligning the text at
the position of `caretWidth/2` even when there is space for the whole text.
Which means that the text will be aligned to left even when there is space for
whole text with CENTER alignment. So in my opinion, current conditions and
values gives better alignment behavior.
-
PR: https://git.openjdk.org/jfx/pull/980
Re: RFR: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly [v6]
> When Text width was more than TextField width, the logic to update > `textTranslateX` in `updateCaretOff` method was causing the issue of > unexpected behavior for Right and Center alignment. > > Made changes to update `textTranslateX` in `updateCaretOff` method only when > text width is less than text field width i.e `delta` is positive. > For both right and center alignments, the `textTranslateX` value calculated > in `updateTextPos` method will be updated without any condition so that > expected behavior is achieved for all scenarios of text width relative to > text field width. > > Added unit tests to validate LEFT, CENTER and RIGHT alignments. RIGHT and > CENTER alignment tests are expected to fail without the fix provided in this > PR. Karthik P K has updated the pull request incrementally with one additional commit since the last revision: Updating the code according to review comments - Changes: - all: https://git.openjdk.org/jfx/pull/980/files - new: https://git.openjdk.org/jfx/pull/980/files/8cf9fd71..edc1bd79 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=980=05 - incr: https://webrevs.openjdk.org/?repo=jfx=980=04-05 Stats: 7 lines in 1 file changed: 0 ins; 1 del; 6 mod Patch: https://git.openjdk.org/jfx/pull/980.diff Fetch: git fetch https://git.openjdk.org/jfx pull/980/head:pull/980 PR: https://git.openjdk.org/jfx/pull/980
Re: RFR: 8090123: Items are no longer visible when collection is changed [v6]
On Thu, 23 Feb 2023 17:48:39 GMT, Andy Goryachev wrote: > Noticed a minor behavior issue, on Mac with multiple monitors. The secondary > monitor (scale=1) is positioned above the primary retina screen (scale=2). > When showing a popup in the case of 100 elements, the down arrow at the > bottom cannot be seen. > I couldn't reproduce this issue, but created bug [JDK-8303148](https://bugs.openjdk.org/browse/JDK-8303148) since you are seeing this issue. - PR: https://git.openjdk.org/jfx/pull/1039
Re: RFR: 8299595: Remove terminally deprecated JavaFX GTK 2 library [v7]
On Thu, 23 Feb 2023 22:50:17 GMT, Kevin Rushforth wrote:
>> Thiago Milczarek Sayao has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Improve exception
>
> modules/javafx.graphics/src/main/native-glass/gtk/GlassApplication.cpp line
> 118:
>
>> 116: // Major version is checked before loading
>> 117: if (version == 3) {
>> 118: if(gtk_check_version(version, GTK_3_MIN_MINOR_VERSION,
>> GTK_3_MIN_MICRO_VERSION)) {
>
> Minor: add space after `if`
Done.
> modules/javafx.graphics/src/main/native-glass/gtk/GlassApplication.cpp line
> 120:
>
>> 118: if(gtk_check_version(version, GTK_3_MIN_MINOR_VERSION,
>> GTK_3_MIN_MICRO_VERSION)) {
>> 119: char message[100];
>> 120: std::sprintf(message, "Minimum GTK version required is
>> %d.%d.%d. System has %d.%d.%d.",
>
> Please change this to `snprintf`, which take the length of the array. We
> should not be using `sprintf` directly as that can lead to buffer overflow.
Done.
-
PR: https://git.openjdk.org/jfx/pull/999
Re: RFR: 8299595: Remove terminally deprecated JavaFX GTK 2 library [v8]
> Simple PR to remove gtk2 library compilation and loading. Thiago Milczarek Sayao has updated the pull request incrementally with one additional commit since the last revision: Review changes - Changes: - all: https://git.openjdk.org/jfx/pull/999/files - new: https://git.openjdk.org/jfx/pull/999/files/d315bc52..8624dd27 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=999=07 - incr: https://webrevs.openjdk.org/?repo=jfx=999=06-07 Stats: 13 lines in 1 file changed: 0 ins; 1 del; 12 mod Patch: https://git.openjdk.org/jfx/pull/999.diff Fetch: git fetch https://git.openjdk.org/jfx pull/999/head:pull/999 PR: https://git.openjdk.org/jfx/pull/999
Re: RFR: 8303038: Glass gtk3 sends scroll events with delta(x, y) = 0 [v3]
On Thu, 23 Feb 2023 11:28:56 GMT, Thiago Milczarek Sayao wrote: >> Simple fix to get the scroll deltas from GDK_SCROLL_SMOOTH. If we ignore >> this scroll event type, deltas are sent to java with the value equal to zero. >> >> Here's whats happening: >> >> We include all event masks, so when using gtk3 (>= 3.4.0) it includes >> `GDK_SMOOTH_SCROLL_MASK` meaning we receive duplicated events, one with >> `direction = GDK_SMOOTH_SCROLL_MASK` and other with "legacy" direction >> (UP/DOWN). >> >> When receiving the event corresponding to `GDK_SMOOTH_SCROLL_MASK` we >> ignored it causing it to send deltas (x,y) = 0. >> >> The fix now checks if `GDK_SMOOTH_SCROLL_MASK` is supported and uses it, >> also adding smooth scroll functionality. > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Fix direction I left a print statement for testing. I will remove once confirmed to work. - PR: https://git.openjdk.org/jfx/pull/1044
Re: RFR: 8303038: Glass gtk3 sends scroll events with delta(x, y) = 0 [v4]
> Simple fix to get the scroll deltas from GDK_SCROLL_SMOOTH. If we ignore this > scroll event type, deltas are sent to java with the value equal to zero. > > Here's whats happening: > > We include all event masks, so when using gtk3 (>= 3.4.0) it includes > `GDK_SMOOTH_SCROLL_MASK` meaning we receive duplicated events, one with > `direction = GDK_SMOOTH_SCROLL_MASK` and other with "legacy" direction > (UP/DOWN). > > When receiving the event corresponding to `GDK_SMOOTH_SCROLL_MASK` we ignored > it causing it to send deltas (x,y) = 0. > > The fix now checks if `GDK_SMOOTH_SCROLL_MASK` is supported and uses it, also > adding smooth scroll functionality. Thiago Milczarek Sayao has updated the pull request incrementally with one additional commit since the last revision: Rename var - Changes: - all: https://git.openjdk.org/jfx/pull/1044/files - new: https://git.openjdk.org/jfx/pull/1044/files/0af8cf6f..88620461 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1044=03 - incr: https://webrevs.openjdk.org/?repo=jfx=1044=02-03 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1044.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1044/head:pull/1044 PR: https://git.openjdk.org/jfx/pull/1044
Re: RFR: 8260528: Clean glass-gtk sizing and positioning code [v50]
On Sun, 19 Feb 2023 18:29:16 GMT, Thiago Milczarek Sayao wrote: >> This cleans size and positioning code, reducing special cases, code >> complexity and size. >> >> Changes: >> >> - cached extents: 28, 1, 1, 1 are old defaults - modern gnome uses different >> sizes. It does not assume any size because it varies - it does cache because >> it's unlikely to vary on the same system - but if it does occur, it will >> only waste a resize event. >> - window geometry, min/max size are centralized in >> `update_window_constraints`; >> - Frame extents (the window decoration size used for "total window size"): >> - frame extents are received in `process_property_notify`; >> - removed quirks in java code; >> - When received, call `set_bounds` again to adjust the size (to account >> decorations later received); >> - Removed `activate_window` because it's the same as focusing the window. >> `gtk_window_present` will deiconify and focus it. >> - `ensure_window_size` was a quirk - removed; >> - `requested_bounds` removed - not used anymore; >> - `window_configure` incorporated in `set_bounds` with `gtk_window_move` and >> `gtk_window_resize`; >> - `process_net_wm_property` is a work-around for Unity only (added a check >> if Unity - but it can probably be removed at some point) >> - `restack` split in `to_front()` and `to_back()` to conform to managed code; >> - Set `gtk_window_set_focus_on_map` to FALSE because if TRUE the Window >> Manager changes the window ordering in the "focus stealing" mechanism - this >> makes possible to remove the quirk on `request_focus()`; >> - Note: `geometry_get_*` and `geometry_set_*` moved location but unchanged. > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Re-fix initial size I can confirm that the black header area is fixed on 20.04. I'll do some final testing and code review soon. - PR: https://git.openjdk.org/jfx/pull/915
Re: RFR: 8299595: Remove terminally deprecated JavaFX GTK 2 library [v7]
On Tue, 21 Feb 2023 00:29:13 GMT, Thiago Milczarek Sayao
wrote:
>> Simple PR to remove gtk2 library compilation and loading.
>
> Thiago Milczarek Sayao has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Improve exception
The updated fix and changes to tests looks fine.
I think the check you added for minimum GTK version of 3.8 is also fine, but I
noted one thing (the call to sprintf) that needs to be changed.
modules/javafx.graphics/src/main/native-glass/gtk/GlassApplication.cpp line 118:
> 116: // Major version is checked before loading
> 117: if (version == 3) {
> 118: if(gtk_check_version(version, GTK_3_MIN_MINOR_VERSION,
> GTK_3_MIN_MICRO_VERSION)) {
Minor: add space after `if`
modules/javafx.graphics/src/main/native-glass/gtk/GlassApplication.cpp line 120:
> 118: if(gtk_check_version(version, GTK_3_MIN_MINOR_VERSION,
> GTK_3_MIN_MICRO_VERSION)) {
> 119: char message[100];
> 120: std::sprintf(message, "Minimum GTK version required is
> %d.%d.%d. System has %d.%d.%d.",
Please change this to `snprintf`, which take the length of the array. We should
not be using `sprintf` directly as that can lead to buffer overflow.
-
PR: https://git.openjdk.org/jfx/pull/999
Re: RFR: JDK-8223373: Remove IntelliJ IDEA specific files from the source code repository [v3]
On Thu, 23 Feb 2023 00:41:28 GMT, Thiago Milczarek Sayao wrote: >> This PR does: >> >> - Remove specific Idea files and let it be imported from gradle; >> - Adds checkstyle (to use with checkstyle plugin - it will let you know >> style mistakes); >> - Configures auto-format to sun style (with the changes mentioned in [Code >> Style Rules](https://wiki.openjdk.org/display/OpenJFX/Code+Style+Rules)); >> - Automatically sets Copyright notice (updates year too); >> - Run configurations for samples/toys and builds. > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Use java 17 by default Looks good to me. Tested with the latest IntelliJ version, everything works as expected. The addition to the `.gitignore` is also very helpful and I can confirm I get no unrelated IntelliJ/Sonarlint files anymore. Gradle changes also looks fine as far as I can tell. But someone else should probably have a look as well. - Marked as reviewed by mhanl (Committer). PR: https://git.openjdk.org/jfx/pull/1009
RFR: 8290866: Apple Color Emoji turns gray after JavaFX version 18
This fix properly supports colour rendering of Emoji on macOS On other platforms the Emoji will be rendered as ordinary greyscale glyphs - if there is font support for the requested code point. A simple manual test is provided which uses a Text node, Label control and editable TextField control. Some highlights of the code - To determine if it is a color emoji glyph probe the 'sbix' font table which is what is used by Apple - Text runs now break at an Emoji glyph - The Emoji is retrieved as an BGRA image - ie 4 channel including alpha - It was necessary to retrieve the Emoji glyph bounds via a different CoreText API since the bounds that were being retrieved were wrong for the Emoji image - causing clipping - It was necessary to retrieve the Emoji code point advance via a CoreText API since the HMTX metrics were very wrong - causing overlapping glyphs - drawString checks if it is an Emoji run and redirects to a new drawColorGlyph method which draws the image as a texture - All 3 rendering pipelines have this support and have been verified on all 3 desktop platforms - Commit messages: - 8290866 - 8290866 Changes: https://git.openjdk.org/jfx/pull/1047/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1047=00 Issue: https://bugs.openjdk.org/browse/JDK-8290866 Stats: 511 lines in 15 files changed: 488 ins; 9 del; 14 mod Patch: https://git.openjdk.org/jfx/pull/1047.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1047/head:pull/1047 PR: https://git.openjdk.org/jfx/pull/1047
[jfx20] RFR: 8303019: cssref.html incorrect internal link in Path
documentation change **targeting jfx20 branch** - fixed all incorrect references in "Also has all properties of ..." - added a link to Shape where it was missing - fixed chart -> Chart - Commit messages: - 8303019: cssref.html incorrect internal link in Path Changes: https://git.openjdk.org/jfx/pull/1046/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1046=00 Issue: https://bugs.openjdk.org/browse/JDK-8303019 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jfx/pull/1046.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1046/head:pull/1046 PR: https://git.openjdk.org/jfx/pull/1046
Re: RFR: 8090123: Items are no longer visible when collection is changed [v6]
On Thu, 23 Feb 2023 05:21:37 GMT, Karthik P K wrote: >> When a large number of items were scrolled in the `ChoiceBox`, the scrolled >> offset was carried forward when the list is replaced with small number of >> items. Hence the scroll up arrow was displayed with empty popup. >> >> Changed code to scroll to top before popup display when content height of >> `ChoiceBox` is smaller than the scrolled offset. >> >> Added system test to validate the fix. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comments The issue described in the ticket appears to be fixed. Noticed a minor behavior issue, on Mac with multiple monitors. The secondary monitor (scale=1) is positioned above the primary retina screen (scale=2). When showing a popup in the case of 100 elements, the down arrow at the bottom cannot be seen. In the attached screenshot, one can see the popup clipped at the secondary monitor bottom edge, the gray bar is the top of the primary screen:  Perhaps the wrong Screen is used in computation, or not accounting for getVisualBounds()? Since this control is designed to work with a few items, I'd suggest addressing this issue in a separate PR. Karthik, would you please create a new bug? - Marked as reviewed by angorya (Committer). PR: https://git.openjdk.org/jfx/pull/1039
Re: RFR: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly [v5]
On Thu, 23 Feb 2023 08:52:19 GMT, John Hendrikx wrote:
>> Karthik P K has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix text and prompt alignment issue
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
> line 805:
>
>> 803: // appear at the left of the centered prompt.
>> 804: newX = midPoint -
>> promptNode.getLayoutBounds().getWidth() / 2;
>> 805: if (newX > 0) {
>
> Let's say `caretWidth / 2` is 5, and that would be position we would show the
> prompt text if it doesn't fit.
>
> If the `newX` calculation is less than 5, but greater than 0, the prompt text
> would be further to the left than you'd expect.
>
> So I think this line should be:
>
> Suggestion:
>
> if (newX > caretWidth / 2) {
>
>
> Probably best to put that in a local variable.
this is undocumented, I think, but the caret path can be one of the three
things:
1. a single point (MoveTo, I think)
2. a single line - MoveTo followed by a LineTo
3. two separate lines (split caret) - MoveTo, LineTo, MoveTo, LineTo (split
caret is explicitly ignored on line 252)
One would think that the caret width can be changed by CSS, but it is not
easy. The caretPath on TextInputControl:184 has no CSS class name set, so the
width cannot be changed trivially via a stylesheet. I suppose the application
code can dig the internals looking for paths and setting widths manually, but
this would be a highly unreliable move.
So I think this caretWidth can be other than 1.0 (line 233) for the purposes of
this PR.
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
> line 818:
>
>> 816: } else if (newX < 0 && oldX > 1) {
>> 817: textTranslateX.set(caretWidth / 2);
>> 818: }
>
> I get the impression this code also needs to use `caretWidth / 2` instead of
> `0` and `1`.
>
> ie:
>
> Suggestion:
>
> // Update if there is space on the right
> if (newX + textNodeWidth <= textRight.get() - caretWidth / 2)
> {
> textTranslateX.set(newX);
> } else if (newX < caretWidth / 2 && oldX > caretWidth / 2) {
> textTranslateX.set(caretWidth / 2);
> }
>
>
> It would only work for very narrow caret's otherwise, and for wide caret's it
> would have some unexpected positioning in the edge case where text almost
> fits.
I would also recommend testing the code on Windows with the screen scale set to
225%, as it might show issues related to fractional scale.
-
PR: https://git.openjdk.org/jfx/pull/980
Re: RFR: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly
On Thu, 22 Dec 2022 10:33:15 GMT, Ajit Ghaisas wrote: >> When Text width was more than TextField width, the logic to update >> `textTranslateX` in `updateCaretOff` method was causing the issue of >> unexpected behavior for Right and Center alignment. >> >> Made changes to update `textTranslateX` in `updateCaretOff` method only when >> text width is less than text field width i.e `delta` is positive. >> For both right and center alignments, the `textTranslateX` value calculated >> in `updateTextPos` method will be updated without any condition so that >> expected behavior is achieved for all scenarios of text width relative to >> text field width. >> >> Added unit tests to validate LEFT, CENTER and RIGHT alignments. RIGHT and >> CENTER alignment tests are expected to fail without the fix provided in this >> PR. > > The proposed fix fixes the reported issue. > > **Regarding the question of changing current behavior of RIGHT alignment of > the TextField:** > If the string is larger than the TextField width - > - LEFT align should result in - Start of the text visible and end of the text > hidden. > - RIGHT align should result in - End of the text visible at the right edge of > the TextField and start of the text hidden. > > Right now, only the LEFT align behaves correctly. > RIGHT align behaves correctly only if the string is smaller than the > TextField width. The moment string length is larger than TextField width, it > starts behaving opposite (start of the text visible and end of the text > hidden.) > I don't know whether there is some rationale for this implementation or it is > a bug which went unnoticed till now. Noticed an issue similar to what @aghaisas reported earlier. - set TOP_RIGHT alignment (any _RIGHT in fact) - resize the field such that the text is wider than TextField can display - click on the first visible character and start typing Expected: the typed chars should appear in the view, the caret moves right, the remaining text scrolls right Observed: the typed characters appear to the left of the caret which does not move. It might behave differently when the caret is at the rightmost position next to the control edge, in which case the behavior is correct. Perhaps there ought to be some conditional logic implemented, but the way it behaves now when the caret is close to the left edge feels unnatural.  - PR: https://git.openjdk.org/jfx/pull/980
Re: RFR: 8178368: Right alignment of text fields and alignment of prompt text works incorrectly [v5]
On Thu, 23 Feb 2023 07:36:43 GMT, Karthik P K wrote: >> When Text width was more than TextField width, the logic to update >> `textTranslateX` in `updateCaretOff` method was causing the issue of >> unexpected behavior for Right and Center alignment. >> >> Made changes to update `textTranslateX` in `updateCaretOff` method only when >> text width is less than text field width i.e `delta` is positive. >> For both right and center alignments, the `textTranslateX` value calculated >> in `updateTextPos` method will be updated without any condition so that >> expected behavior is achieved for all scenarios of text width relative to >> text field width. >> >> Added unit tests to validate LEFT, CENTER and RIGHT alignments. RIGHT and >> CENTER alignment tests are expected to fail without the fix provided in this >> PR. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Fix text and prompt alignment issue almost there, except for one scenario with _RIGHT alignment which feels weird. - PR: https://git.openjdk.org/jfx/pull/980
Integrated: 8301022: Video distortion is observed while playing youtube video
On Wed, 22 Feb 2023 07:46:58 GMT, Jay Bhaskar wrote: > Issue: current update is breaking the rendering of media controls on youtube > video playback >For the Webkit Gtk platform, the layout class name is returned as > AdwaitaLayoutTraits >which is incompatible with the JAVA platform. > Solution: Use mediaControlsAdwaitaJavaScript and > mediaControlsAdwaitaUserAgentStyleSheet > which is compatible with RenderThemeJava. > Test: After the fix, open youtube.com and play any video, mouse hover on the > video area, control should come up. > Without the fix, open youtube.com and play any video, mouse hover on > the video area, and the control > should come up overlapping with each other. This pull request has now been integrated. Changeset: 14883a29 Author:Jay Bhaskar Committer: Kevin Rushforth URL: https://git.openjdk.org/jfx/commit/14883a296ac08a91fc24570a7479a6c8c2117643 Stats: 1527 lines in 4 files changed: 1527 ins; 0 del; 0 mod 8301022: Video distortion is observed while playing youtube video Reviewed-by: kcr, hmeda - PR: https://git.openjdk.org/jfx/pull/1045
Re: RFR: 8303038: Glass gtk3 sends scroll events with delta(x, y) = 0 [v3]
On Thu, 23 Feb 2023 11:28:56 GMT, Thiago Milczarek Sayao wrote: >> Simple fix to get the scroll deltas from GDK_SCROLL_SMOOTH. If we ignore >> this scroll event type, deltas are sent to java with the value equal to zero. >> >> Here's whats happening: >> >> We include all event masks, so when using gtk3 (>= 3.4.0) it includes >> `GDK_SMOOTH_SCROLL_MASK` meaning we receive duplicated events, one with >> `direction = GDK_SMOOTH_SCROLL_MASK` and other with "legacy" direction >> (UP/DOWN). >> >> When receiving the event corresponding to `GDK_SMOOTH_SCROLL_MASK` we >> ignored it causing it to send deltas (x,y) = 0. >> >> The fix now checks if `GDK_SMOOTH_SCROLL_MASK` is supported and uses it, >> also adding smooth scroll functionality. > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Fix direction It works with me now. - Marked as reviewed by gl...@github.com (no known OpenJDK username). PR: https://git.openjdk.org/jfx/pull/1044
Re: RFR: 8303038: Glass gtk3 sends scroll events with delta(x, y) = 0 [v3]
> Simple fix to get the scroll deltas from GDK_SCROLL_SMOOTH. If we ignore this > scroll event type, deltas are sent to java with the value equal to zero. Thiago Milczarek Sayao has updated the pull request incrementally with one additional commit since the last revision: Fix direction - Changes: - all: https://git.openjdk.org/jfx/pull/1044/files - new: https://git.openjdk.org/jfx/pull/1044/files/135f410a..0af8cf6f Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1044=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1044=01-02 Stats: 13 lines in 1 file changed: 9 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jfx/pull/1044.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1044/head:pull/1044 PR: https://git.openjdk.org/jfx/pull/1044
Re: RFR: 8303038: Glass gtk3 sends scroll events with delta(x, y) = 0 [v2]
On Thu, 23 Feb 2023 02:35:20 GMT, Thiago Milczarek Sayao wrote: >> Simple fix to get the scroll deltas from GDK_SCROLL_SMOOTH. If we ignore >> this scroll event type, deltas are sent to java with the value equal to zero. > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Fix for Ubuntu 20.04 Yeah, I was going to check this, I noticed it was probably reversed. - PR: https://git.openjdk.org/jfx/pull/1044
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak
On Thu, 23 Feb 2023 09:26:29 GMT, Marius Hanl wrote:
>> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
>> line 285:
>>
>>> 283: }
>>> 284:
>>> 285: private static void removeAcceleratorsFromScene(List>> MenuItem> items, Scene scene) {
>>
>> May I suggest a change name: removeAcceleratorsFromScenePrivate() to avoid
>> confusion?
>
> I'm not in favor of using `Private` in a method name. That is clear from the
> method signature and overloading methods is a valid choice In my opinion,
> this is fine as is.
> But we could also think about naming it: `removeAcceleratorsFromSceneImpl()`
I've been working on changes for a possible follow-up PR which address more
bugs. For adding accelerators, there are 3 public methods, for removing there
are currently 4.
I've looked at it, and it can/should be made to have 3 public remove methods
that exactly mirror the public add methods (if you use a specific one to add
then use the corresponding one to remove).
Further, for every private addAcceleratorsFromScene method and
doAcceleratorInstall method I believe there should be a private corresponding
method which reverses it. I had to rename a couple private
removeAcceleratorsFromScene to doAcceleratorUninstall.
So yes, this is a very confusing class. Pairing up the add/remove methods make
sense to me. Once that's done, we might want to rename some of the private ones
just so it's easier to understand what each one is doing, but the public ones
are fine I think.
-
PR: https://git.openjdk.org/jfx/pull/1037
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak
On Wed, 22 Feb 2023 19:24:16 GMT, Andy Goryachev wrote:
>> Each time a menu would change scenes, a new set of ListChangeListeners would
>> be added to the items in the menu. The bigger problem however is that these
>> list change listeners have a strong reference to the scene which is
>> potentially a much bigger leak.
>>
>> The first commit was more straightforward, but there are 2 things that might
>> be of concern:
>>
>> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but
>> it'll remove all the ListChangeListeners attached to it, regardless of which
>> scene was passed in. Something similar happens with changeListenerMap
>> already, so I think it's fine.
>> 2. I made the remove method public so that external calls from skins to
>> remove the accelerators would remove the ListChangeListener and also because
>> all the remove methods are public.
>>
>> After I wrote more tests I realised using the ObservableLists as keys in the
>> WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would
>> be simple. The fix is in the second commit.
>>
>> There are still more issues that stem from the fact that for each anchor
>> there could be multiple menus and the current code doesn't account for that.
>> For example, swapping context menus on a tab doesn't register change
>> listeners on the new context menu because the TabPane itself had a scene
>> change listener already. These other issues could relate to JDK-8268374
>> https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449
>> https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to
>> the fact that there's no logic to remove listeners when Tab/TableColumn's
>> are removed from their associated control (TabPane, TableView,
>> TreeTableView).
>>
>> I'm looking at these issues, but I think they're dependent on this fix.
>> Either I can add to this PR or I can wait to see what comes out of this and
>> fix them subsequently.
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
> line 285:
>
>> 283: }
>> 284:
>> 285: private static void removeAcceleratorsFromScene(List> MenuItem> items, Scene scene) {
>
> May I suggest a change name: removeAcceleratorsFromScenePrivate() to avoid
> confusion?
I'm not in favor of using `Private` in a method name. That is clear from the
method signature and overloading methods is a valid choice In my opinion, this
is fine as is.
But we could also think about naming it: `removeAcceleratorsFromSceneImpl()`
-
PR: https://git.openjdk.org/jfx/pull/1037
Re: RFR: 8178368: Right and Center alignment of text field works incorrectly [v5]
On Thu, 23 Feb 2023 07:36:43 GMT, Karthik P K wrote: >> When Text width was more than TextField width, the logic to update >> `textTranslateX` in `updateCaretOff` method was causing the issue of >> unexpected behavior for Right and Center alignment. >> >> Made changes to update `textTranslateX` in `updateCaretOff` method only when >> text width is less than text field width i.e `delta` is positive. >> For both right and center alignments, the `textTranslateX` value calculated >> in `updateTextPos` method will be updated without any condition so that >> expected behavior is achieved for all scenarios of text width relative to >> text field width. >> >> Added unit tests to validate LEFT, CENTER and RIGHT alignments. RIGHT and >> CENTER alignment tests are expected to fail without the fix provided in this >> PR. > > Karthik P K has updated the pull request incrementally with one additional > commit since the last revision: > > Fix text and prompt alignment issue I think this class may benefit from a few tests that test with a very wide caret, to see if positioning is what you'd expect in those cases as well. I get the impression a lot of the code assumes a narrow caret (1 or 2 pixels) and this is why we see constants like `0` and `1` in the code, and even places where the caret width should be subtracted but isn't because it is assumed to be `0`. - PR: https://git.openjdk.org/jfx/pull/980
Re: RFR: 8178368: Right and Center alignment of text field works incorrectly [v5]
On Thu, 23 Feb 2023 07:36:43 GMT, Karthik P K wrote:
>> When Text width was more than TextField width, the logic to update
>> `textTranslateX` in `updateCaretOff` method was causing the issue of
>> unexpected behavior for Right and Center alignment.
>>
>> Made changes to update `textTranslateX` in `updateCaretOff` method only when
>> text width is less than text field width i.e `delta` is positive.
>> For both right and center alignments, the `textTranslateX` value calculated
>> in `updateTextPos` method will be updated without any condition so that
>> expected behavior is achieved for all scenarios of text width relative to
>> text field width.
>>
>> Added unit tests to validate LEFT, CENTER and RIGHT alignments. RIGHT and
>> CENTER alignment tests are expected to fail without the fix provided in this
>> PR.
>
> Karthik P K has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix text and prompt alignment issue
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
line 805:
> 803: // appear at the left of the centered prompt.
> 804: newX = midPoint -
> promptNode.getLayoutBounds().getWidth() / 2;
> 805: if (newX > 0) {
Let's say `caretWidth / 2` is 5, and that would be position we would show the
prompt text if it doesn't fit.
If the `newX` calculation is less than 5, but greater than 0, the prompt text
would be further to the left than you'd expect.
So I think this line should be:
Suggestion:
if (newX > caretWidth / 2) {
Probably best to put that in a local variable.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
line 818:
> 816: } else if (newX < 0 && oldX > 1) {
> 817: textTranslateX.set(caretWidth / 2);
> 818: }
I get the impression this code also needs to use `caretWidth / 2` instead of
`0` and `1`.
ie:
Suggestion:
// Update if there is space on the right
if (newX + textNodeWidth <= textRight.get() - caretWidth / 2) {
textTranslateX.set(newX);
} else if (newX < caretWidth / 2 && oldX > caretWidth / 2) {
textTranslateX.set(caretWidth / 2);
}
It would only work for very narrow caret's otherwise, and for wide caret's it
would have some unexpected positioning in the edge case where text almost fits.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
line 831:
> 829: // Align to left when prompt text length is more
> than text field width
> 830: promptNode.setLayoutX(caretWidth / 2);
> 831: }
Similar comment, I don't think its correct. It's just odd that promptNewX may
be smaller than caretWidth / 2 but is accepted, but when the text is too wide
the "minimum" value suddently is caretWidth / 2.
Also, I don't see how `promptOldX` has any relevance when it comes to
positioning the prompt. I think this is only relevant for the actual text
(because it can scroll depending on where the cursor is), not for the prompt --
unless the prompt can be scrolled as well. If that's the case then the
`CENTER` case may need to be updated to take `promptOldX` into account as well.
As it stands currently, they have behaviors that seem to conflict.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
line 839:
> 837: } else if (newX < 0 && oldX > 1) {
> 838: textTranslateX.set(caretWidth / 2);
> 839: }
Again, I don't think its correct. It's just odd that `newX` may be smaller
than `caretWidth / 2` but is accepted, but when the text is too wide the
"minimum" value suddenly is `caretWidth / 2`.
-
PR: https://git.openjdk.org/jfx/pull/980
ChangeListener documentation clarification
Hi list, I've been busy trying to ensure that ChangeListeners received sensible old/new values at all times. This is important as users may be relying on these values to be correct when doing calculations or creating nested bindings (where an old listener is removed based on the old value received, and a new listener is registered based on the new value received). As you may know, currently the values received by ChangeListeners are not what you'd expect when the value is modified within an ongoing notification (a nested change). This affects ChangeListeners registered *after* the listener that triggered the nested change. These listeners receive incorrect old values, and duplicate new values. It gets even more complicated when multiple listeners do this or when listeners are added/removed during the notification process (all of which is allowed). I would like to update the ChangeListener javadoc to document what the user can expect when they register their listener. The current documentation implies that: - The listener is only called when a change occurs (implying new value received changed from last time) - The above further implies old and new values are distinct as it must have changed - The fact that it is called the "old value" implies that it will hold the value of the previously received new value and not some other value that is distinct from the received new value If we can agree on the above, we may want to specify this contract a bit more explicitly. Also I'd like to explicitly state that the received new value need not be the same as the value obtained from `ObservableValue#getValue` when another notification is still pending. A further rule we should discuss is whether all ChangeListeners should receive notifications about **all** changes to a property. The current implementation will notify all listeners X times when there were X changes, but, when there are nested changes, will not actually send all the changes as a "new value" (it skips some of the values, and sends some twice). I see a few options here: 1. All ChangeListeners receive notifications about all changes exactly as they occurred, regardless of nested changes made by listeners earlier in the chain of listeners. 2. ChangeListeners registered later in the chain of listeners will not see changes that were changed (vetoed) by earlier listeners. This implies that if there were X changes, later listeners may see X - Y changes, where Y is the number of nested changes made. This also implies there is a strict order in which listeners are registered and called. I would be inclined to go with option 1 here, but I would like to hear what others think. The current implementation in my PR (https://github.com/openjdk/jfx/pull/837) changes `ExpressionHelper` to follow the above contract, and uses option 1, where all change listeners receive all notifications exactly as they occurred. The current implementations in JavaFX breaks the above contract in several ways: 1) Old value and new value can be the same instance. The collection properties (ListProperty) do this to avoid having to make a copy of the collection for a proper old value. I think this okay, but they need to document they are deviating here from the ChangeListener contract. Effectively, it is no longer a change listener, it only provides the current values. 2) New value received is the same new value as the previous call. This happens when an earlier listener makes a change, triggering a nested notification. When the nested notification completes, the parent notification continues by sending the same new value again to the later listeners. 3) Old value received is not the same as the new value of the previous call. This happens in the same situation as 2) I'm hoping we can get this situation fixed as they may trigger subtle bugs in calculations involving old/new values. When manually tracking bindings for example it can result in listeners that are never removed (for incorrect old values) or listeners being registered multiple times (for duplicate new values). Thanks for reading! --John
