Re: RFR: 8201568: zForce touchscreen input device fails when closed and immediately reopened [v2]
> Fixes [JDK-8201568](https://bugs.openjdk.java.net/browse/JDK-8201568). John Neffenger has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'master' into zforce-command-overrun - 8201568: zForce touchscreen input device fails when closed and immediately reopened - Changes: - all: https://git.openjdk.java.net/jfx/pull/258/files - new: https://git.openjdk.java.net/jfx/pull/258/files/54631e7f..68c05234 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=258&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=258&range=00-01 Stats: 391615 lines in 5768 files changed: 194160 ins; 135443 del; 62012 mod Patch: https://git.openjdk.java.net/jfx/pull/258.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/258/head:pull/258 PR: https://git.openjdk.java.net/jfx/pull/258
Re: RFR: 8251946: ObservableList.setAll does not conform to specification [v3]
> Hi, this PR fixes > [JDK-8251946](https://bugs.openjdk.java.net/browse/JDK-8251946) computing > whether the list was > actually modified instead of just returning `true`. The list was modified if > 1. it was not empty (modified by calling > `#clear()`), or if 2. it was modified as result of the `#addAll()` call. > > If you want any test coverage for this please let me know. > > I reported the issue a couple of days ago via web formula and waited for the > confirmation to open this PR but now I see > that @kevinrushforth (sorry for pinging) is already assigned to the JBS > issue. I'm not too familiar with how you handle > JBS issues or more specifically whether assigning is used to indicate that > the issue is in the assignee's "domain", or > that the assignee is already working on it. In the latter case, please feel > free to close this PR. (My OCA submission > is still pending anyway.) Leon Linhart has updated the pull request incrementally with one additional commit since the last revision: Reverted incorrect change and improved test coverage - Changes: - all: https://git.openjdk.java.net/jfx/pull/284/files - new: https://git.openjdk.java.net/jfx/pull/284/files/e8857913..5f260d7e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=284&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=284&range=01-02 Stats: 5 lines in 2 files changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/284.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/284/head:pull/284 PR: https://git.openjdk.java.net/jfx/pull/284
Re: RFR: 8251946: ObservableList.setAll does not conform to specification [v3]
On Fri, 25 Sep 2020 15:49:12 GMT, Kevin Rushforth wrote: >> Makes sense to me. I changed it accordingly. > > I don't think this change is correct. `setAll(Collection)` should return > true if the list is modified. As discussed in > an [earlier comment](#issuecomment-684117392) this means returning true if > either the existing Collection or the new > Collection is non-empty. I'm sorry, you're absolutely right. I didn't spend enough time thinking about the change and assumed it was fine since the tests passed. Turns out the test didn't cover it. I reverted that part of the change and improved the test. - PR: https://git.openjdk.java.net/jfx/pull/284
Re: RFR: 8251946: ObservableList.setAll does not conform to specification [v2]
On Fri, 25 Sep 2020 10:23:54 GMT, Leon Linhart wrote: >> modules/javafx.base/src/main/java/javafx/collections/ModifiableObservableListBase.java >> line 97: >> >>> 95: clear(); >>> 96: addAll(col); >>> 97: return true; >> >> I think following code would be more suitable here, >> boolean res = super.addAll(c); >> return res; >> This code is already used in two `addAll()` methods of this class. > > Makes sense to me. I changed it accordingly. I don't think this change is correct. `setAll(Collection)` should return true if the list is modified. As discussed in an [earlier comment](#issuecomment-684117392) this means returning true if either the existing Collection or the new Collection is non-empty. - PR: https://git.openjdk.java.net/jfx/pull/284
RFR: 8253634: TreeCell/Skin: misbehavior on switching skin
TreeCellSkin installs listeners to the TreeView/fixedCellSize that introduce a memory leak, NPE on replacing the treeView and incorrect update of internal state. Fixed by removing the listeners (and the internal state had been copied from treeView on change) and access of listView state when needed. Added tests that failed before and pass after the fix, plus a sanity test to guarantee same (correct) behavior before/after. Issue and fix is basically the same as for ListCellSkin [JDK-8246745](https://bugs.openjdk.java.net/browse/JDK-8246745) - Commit messages: - 8253634: TreeCell/Skin: misbehavior on switching skin Changes: https://git.openjdk.java.net/jfx/pull/309/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=309&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253634 Stats: 101 lines in 4 files changed: 60 ins; 36 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/309.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/309/head:pull/309 PR: https://git.openjdk.java.net/jfx/pull/309
Re: RFR: 8251946: ObservableList.setAll does not conform to specification [v2]
On Thu, 24 Sep 2020 18:23:54 GMT, Ambarish Rapte wrote: >> Leon Linhart has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removed unused import and addressed review comments > > modules/javafx.base/src/main/java/javafx/collections/ModifiableObservableListBase.java > line 32: > >> 30: import java.util.List; >> 31: import java.util.ListIterator; >> 32: import java.util.Objects; > > This import seems unused, please remove. Indeed. Removed it. > modules/javafx.base/src/main/java/javafx/collections/ModifiableObservableListBase.java > line 97: > >> 95: clear(); >> 96: addAll(col); >> 97: return true; > > I think following code would be more suitable here, > boolean res = super.addAll(c); > return res; > This code is already used in two `addAll()` methods of this class. Makes sense to me. I changed it accordingly. - PR: https://git.openjdk.java.net/jfx/pull/284
Re: RFR: 8251946: ObservableList.setAll does not conform to specification [v2]
> Hi, this PR fixes > [JDK-8251946](https://bugs.openjdk.java.net/browse/JDK-8251946) computing > whether the list was > actually modified instead of just returning `true`. The list was modified if > 1. it was not empty (modified by calling > `#clear()`), or if 2. it was modified as result of the `#addAll()` call. > > If you want any test coverage for this please let me know. > > I reported the issue a couple of days ago via web formula and waited for the > confirmation to open this PR but now I see > that @kevinrushforth (sorry for pinging) is already assigned to the JBS > issue. I'm not too familiar with how you handle > JBS issues or more specifically whether assigning is used to indicate that > the issue is in the assignee's "domain", or > that the assignee is already working on it. In the latter case, please feel > free to close this PR. (My OCA submission > is still pending anyway.) Leon Linhart has updated the pull request incrementally with one additional commit since the last revision: Removed unused import and addressed review comments - Changes: - all: https://git.openjdk.java.net/jfx/pull/284/files - new: https://git.openjdk.java.net/jfx/pull/284/files/5f360384..e8857913 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=284&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=284&range=00-01 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/284.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/284/head:pull/284 PR: https://git.openjdk.java.net/jfx/pull/284
Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
On Fri, 25 Sep 2020 07:47:41 GMT, Johan Vos wrote: > > > The visual representation corresponds with digits, so there can be tests that > check if the numbers are what we expect > them to be. It's good that this is not windows-only, so that it can be > tackled on linux as well. But what is not clear > to me: does this require a physical HiDPI screen, or is setting the scale > factor manually good enough to reproduce the > bug? The issue will appear consistently as long as the conditions I listed are met, regardless of the actual number of pixels the physical screen can display. For instance you'll see the problem, if you apply a 125% scaling on 1080p screen (a common configuration on 13'' laptops). Also, it will occur regardless of whether the scaling is applied at the OS level and picked up by javafx or if it is only set for a single application using the `glass.xxx.uiScale` property. - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
On Fri, 25 Sep 2020 07:47:41 GMT, Johan Vos wrote: >> Also, I don't really have an idea on how this could be tested other than >> visually, so I'm open to suggestions. > > The visual representation corresponds with digits, so there can be tests that > check if the numbers are what we expect > them to be. It's good that this is not windows-only, so that it can be > tackled on linux as well. But what is not clear > to me: does this require a physical HiDPI screen, or is setting the scale > factor manually good enough to reproduce the > bug? FYI: there is another bug [JDK-8199592](https://bugs.openjdk.java.net/browse/JDK-8199592) that *reads* somewhat similar even though the issues are very different. It is also marked as a "Windows" bug. Maybe the root of both issues is the same... - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8211294: [windows] TextArea content is blurry with 125% scaling
On Thu, 24 Sep 2020 19:09:19 GMT, Frederic Thevenet wrote: >> This PR aims to fix the blurriness to sometimes affects some controls (such >> as TextArea) in a scene when rendered with >> a scaling factor that is not an integer (typically when viewed on a HiDPI >> screen with a 125%, 150% or 175% output >> scaling). Please note that regardless of what the JBS issue (and therefore >> the title of this PR) states, this does not >> appear to be a Windows only issue as I have observed the same issue on Linux >> (Ubuntu 20.04). >> The following conditions are necessary for the blurriness to appear, but do >> not guarantee that it will: >> >> - The node, or one of its parents, must have set `Node::cacheProperty` to >> `true` >> >> - The scaling factor (as detected automatically or explicitly set via >> glass.win/gtk.uiScale) must be a non integer number >> (e.g. 1.25, 1.5, 175) >> >> - The effective layout X or Y coordinates for the cached node must be != 0; >> >> Under these conditions, the translate coordinates for the transform used >> when the cached image for a node is rendered >> to the screen may be non integer numbers, which is the cause for the >> blurriness. >> Based on these observations, this PR fixes the issue by simply rounding the >> translate coordinates (using `Math.round`) >> before the transform is applied in `renderCacheToScreen()` and as far as I >> can tell, it fixes the blurriness in all the >> previously affected applications (both trivial test cases or with complex >> scenegraphs) and does not appear to introduce >> other noticeable visual artifacts. Still, there might be a better place >> somewhere else higher up in the call chain >> where this should be addressed as it could maybe be the root cause for other >> rendering glitches, though I'm not yet >> familiar enough with the code to see if it is really the case. > > Also, I don't really have an idea on how this could be tested other than > visually, so I'm open to suggestions. The visual representation corresponds with digits, so there can be tests that check if the numbers are what we expect them to be. It's good that this is not windows-only, so that it can be tackled on linux as well. But what is not clear to me: does this require a physical HiDPI screen, or is setting the scale factor manually good enough to reproduce the bug? - PR: https://git.openjdk.java.net/jfx/pull/308
Re: RFR: 8244297: memory leak test utility [v4]
On Thu, 7 May 2020 16:08:25 GMT, Florian Kirmaier wrote:
>> It's based on the discussion of my previous PR:
>> https://github.com/openjdk/jfx/pull/71
>>
>> I Added test utility class copied from JMemoryBuddy and used it to simplify
>> 4 of the existing unit tests.
>>
>> It's a direct copy of my project
>> [JMemoryBuddy](https://github.com/Sandec/JMemoryBuddy) without any changes.
>> I'm also using it in most of the projects I'm involved with and in my
>> experience, the tests with this Library are very
>> stable. I can't remember wrong test results. Sometimes the memory behaviour
>> of some libraries itself is not stable but
>> the tests with JMemoryBuddy are basically always correct.
>
> Florian Kirmaier has updated the pull request incrementally with one
> additional commit since the last revision:
>
> JDK-8244297
> Improved the JavaDoc for JMemoryBuddy
Tests fail to compile, please correct the import.
Pointed error in `ToggleButtonTest`, please check other files also when
correcting this.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ToggleButtonTest.java
line 45:
> 43: import org.junit.Before;
> 44: import org.junit.Test;
> 45: import de.sandec.jmemorybuddy.JMemoryBuddy;
Throws compilation error: package de.sandec.jmemorybuddy does not exist
modules/javafx.controls/src/test/java/test/javafx/scene/control/ToggleButtonTest.java
line 161:
> 159:
> 160: @Test public void toggleGroupViaGroupAddAndRemoveClearsReference() {
> 161: JMemoryBuddy.memoryTest(checker -> {
Compilation error: cannot find symbol JMemoryBuddy
-
Changes requested by arapte (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/204
