Re: RFR: 8236689: macOS 10.15 Catalina: LCD text renders badly

2021-10-15 Thread Phil Race
On Wed, 13 Oct 2021 23:59:40 GMT, Phil Race  wrote:

> On an external (non-retina) monitor JavaFX LCD text on macOS is painful on 
> the eyes.
> Retina diminishes it rather than cures it.
> 
> The problem is a mix of a couple of things
> 1) CoreText no longer generates LCD glyphs (except perhaps if you change some 
> system settings at your own risk)
> 2) Prism's LCD shader assumes it got LCD glyphs and makes sub-pixel 
> positioning adjustments that turn greyscale
> glyphs into multi-coloured glyphs that weren't meant to be ...
> 
> The fix here is to just disable LCD by default on macOS as is already done 
> (eg) on iOS
> This ripples through to make everything use grey scale even if you asked for 
> the LCD (which you can't have)
> It also means if you REALLY want it (and perhaps are tweaking those magical 
> settings) you can have it back
> by just specifying -Dprism.lcdtext=on
> 
> Also it means the pieces of support for this on macos are still there if 
> Apple ever bring it back (unlikely).
> Not that much code would be removed anyway .. a fair amount of it is needed 
> for Windows and Linux.

Any takers for the 2nd reviewer ?
BTW another reason for making this simple and easy to revert (after lots more 
exploriing than you'd think for what ended up as a small fix) is that this 
should be backported to the LTS releases ..

-

PR: https://git.openjdk.java.net/jfx/pull/642


Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]

2021-10-15 Thread Kevin Rushforth
On Fri, 15 Oct 2021 16:23:10 GMT, Kevin Rushforth  wrote:

> > Do you want to do something about `idMap` in 
> > `com.sun.java.scene.web.WebViewHelper.WebView` that I pointed out in the PR?
> 
> I missed that comment. Where do you see this field? I can't find it anywhere.

Oh, I see it now. It's in the android sources, which aren't part of the regular 
build. @johanvos will need to comment.

-

PR: https://git.openjdk.java.net/jfx/pull/627


Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]

2021-10-15 Thread Kevin Rushforth
On Fri, 15 Oct 2021 15:04:00 GMT, Nir Lisker  wrote:

> Do you want to do something about `idMap` in 
> `com.sun.java.scene.web.WebViewHelper.WebView` that I pointed out in the PR?

I missed that comment. Where do you see this field? I can't find it anywhere.

-

PR: https://git.openjdk.java.net/jfx/pull/627


Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]

2021-10-15 Thread Kevin Rushforth
On Fri, 15 Oct 2021 14:24:15 GMT, Nir Lisker  wrote:

>> Replacements of more immutable collections.
>> 
>> One thing I found was that the field `idMap` in 
>> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it 
>> if that's indeed the case.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Removed unused imports

Marked as reviewed by kcr (Lead).

-

PR: https://git.openjdk.java.net/jfx/pull/627


Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit [v2]

2021-10-15 Thread Jeanette Winzenburg
> cell startEdit is supposed to update the editing location on its associated 
> control - was done in ListCell, not in Tree-/TableCell nor TreeCell.
> 
> Fix was to add control.edit(..). Note that ListCell was also touched to use 
> the exact same method call pattern as the fixed cell types.
> 
> Added/unignored cell tests that are failing/passing before/after the fix.

Jeanette Winzenburg has updated the pull request incrementally with one 
additional commit since the last revision:

  fixed formatting as suggested in review
  
  and removed unused (by this fix) import in same file

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/638/files
  - new: https://git.openjdk.java.net/jfx/pull/638/files/050a6caa..4f76e1fe

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=638=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=638=00-01

  Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod
  Patch: https://git.openjdk.java.net/jfx/pull/638.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/638/head:pull/638

PR: https://git.openjdk.java.net/jfx/pull/638


Re: RFR: 8187474: Tree-/TableCell, TreeCell: editingCell/Item not updated in cell.startEdit [v2]

2021-10-15 Thread Jeanette Winzenburg
On Wed, 13 Oct 2021 21:25:43 GMT, Marius Hanl  wrote:

>> Jeanette Winzenburg has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   fixed formatting as suggested in review
>>   
>>   and removed unused (by this fix) import in same file
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/TableCellTest.java
>  line 549:
> 
>> 547: int editingRow = 1;
>> 548: cell.updateIndex(editingRow);
>> 549: TablePosition editingCell = new TablePosition<>(table, 
>> editingRow, editingColumn);
> 
> Minor: There is a space missing in ``

thanks :)

-

PR: https://git.openjdk.java.net/jfx/pull/638


Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers

2021-10-15 Thread Nir Lisker
On Thu, 14 Oct 2021 23:50:10 GMT, Kevin Rushforth  wrote:

>> Replacements of more immutable collections.
>> 
>> One thing I found was that the field `idMap` in 
>> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it 
>> if that's indeed the case.
>
> modules/javafx.web/src/main/java/com/sun/webkit/Utilities.java line 91:
> 
>> 89: 
>> 90: // List of packages to reject
>> 91: private static final List PACKAGES_REJECT_LIST = List.of(
> 
> After this change, I think the import of `java.util.Arrays` is unused.

Even more than that one :)

-

PR: https://git.openjdk.java.net/jfx/pull/627


Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]

2021-10-15 Thread Nir Lisker
> Replacements of more immutable collections.
> 
> One thing I found was that the field `idMap` in 
> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it 
> if that's indeed the case.

Nir Lisker has updated the pull request incrementally with one additional 
commit since the last revision:

  Removed unused imports

-

Changes:
  - all: https://git.openjdk.java.net/jfx/pull/627/files
  - new: https://git.openjdk.java.net/jfx/pull/627/files/255a314d..79606579

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jfx=627=01
 - incr: https://webrevs.openjdk.java.net/?repo=jfx=627=00-01

  Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod
  Patch: https://git.openjdk.java.net/jfx/pull/627.diff
  Fetch: git fetch https://git.openjdk.java.net/jfx pull/627/head:pull/627

PR: https://git.openjdk.java.net/jfx/pull/627


Re: RFR: 8218745: TableView: visual glitch at borders on horizontal scrolling

2021-10-15 Thread Kevin Rushforth
On Thu, 23 Sep 2021 19:01:52 GMT, Marius Hanl  wrote:

> This PR fixes an issue which is probably in JavaFX since VirtualFlow exists.
> While horizontal scrolling any VirtualFlow control the left blue border 
> sometimes disappear/gets smaller. (see also image below)
> 
> This can be fixed by **snapping** the scroll bar value (similar like e.g. a 
> **ScrollPane** does). 
> The same needs to be done on the table header as otherwise the column lines 
> might be 1 px off to the cell lines. 
> As a side effect this also fixes that the column lines sometimes get's blurry 
> when horizontal scrolling (see second image).
> 
> While testing with **-Dglass.win.uiScale** I found out that the problem is 
> not fixed for a scale like 1.25 or 1.5, while it is fixed for 1 or 2. The 
> border sometimes disappears only when the snapped value is a decimal number 
> (which obviously does not happen on a scale of 1 or 2), e.g. something like 
> 12.6 but it will never disappear when it's a normal number, so e.g. just 12.
> 
> That's why something like **Math.round(..)** or just a **cast** to an **int** 
> instead of snapping fixes this problem for all scales. I also didn't notice 
> any side effect. But not sure if this the right fix then. 
> How does JavaFX render a **node** when e.g. the x is a decimal number? And 
> does a decimal number make sense (Why we e.g. don't round the value)?
> 
> Another explanation could also be that there is an issue somewhere deep 
> inside the node layout/snapping/Clip/Group/pixel rendering and to simply 
> round/cast the value just fixes the symptom here.
> 
> In any case I'm open for any feedback, help or explaination.
> I'm also glad for anything which might help identify the root cause here, if 
> any.
> 
> ---
> 1.
> ![image](https://user-images.githubusercontent.com/66004280/134562508-bea6ab9d-d3d0-4dbb-b0ce-dc6570a94ed7.png)
> ---
> 2.
> ![image](https://user-images.githubusercontent.com/66004280/134563377-970b4e48-8528-4dad-95fb-fb93780413e8.png)

Given that this doesn't fix the problem when Hi-DPI scaling is in effect, this 
will need a different solution. See the discussion in [this 
thread](https://mail.openjdk.java.net/pipermail/openjfx-dev/2021-October/032215.html)
 on the openjfx-dev mailing list for some considerations regarding Hi-DPI 
scaling.

Since this PR isn't ready to be reviewed, I'm moving it to Draft.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualFlow.java
 line 3082:

> 3080: double snappedClipX = snapPositionX(clipX);
> 3081: setLayoutX(-snappedClipX);
> 3082: clipRect.setLayoutX(snappedClipX);

This is likely not the right place to snap the coordinates to a pixel boundary. 
This is usually done as part of layout. I'm also skeptical of the fact that you 
added it to `setClipX` but not `setClipY`.

-

Changes requested by kcr (Lead).

PR: https://git.openjdk.java.net/jfx/pull/630


Re: RFR: 8272808: Update constant collections to use the new immutable collections - leftovers [v2]

2021-10-15 Thread Nir Lisker
On Fri, 15 Oct 2021 14:24:15 GMT, Nir Lisker  wrote:

>> Replacements of more immutable collections.
>> 
>> One thing I found was that the field `idMap` in 
>> `com.sun.java.scene.web.WebViewHelper.WebView` seems unused. I can remove it 
>> if that's indeed the case.
>
> Nir Lisker has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Removed unused imports

Do you want to do something about `idMap` in 
`com.sun.java.scene.web.WebViewHelper.WebView` that I pointed out in the PR?

-

PR: https://git.openjdk.java.net/jfx/pull/627