Re: RFR: 8267505: {List, Set, Map}PropertyBase::bind should check against identity
On Mon, 24 May 2021 11:56:35 GMT, Jose Pereda wrote: > ListPropertyBase::bind, SetPropertyBase::bind, MapPropertyBase::bind have a > check on whether a different instance of the observable is the same, and this > PR changes that to check against identity. > > Tests are included. Looks good. Verified tests failing before and succeed after the fix. One side note which might explaing, why equals was used: The super class `ReadOnlyList/Set/MapProperty` have hashcode/equals overridden, where equals accepts the corresponding collection (List/Set/Map), which is then checked against the collection the property has set. This is fine, although it looks a bit weird at first as the equals expects a collection, not a property. But it makes sense as all those classes implements the corresponding collection interface (List/Map/Set). And a quick look in the history is showing, that at first the equals was really checking for a property (and in there: bean and name), which makes `normalList.equals(listProperty)` never work. So maybe this is leftover code from the old implementaation. - Marked as reviewed by mara...@github.com (no known OpenJDK username). PR: https://git.openjdk.java.net/jfx/pull/516
Re: RFR: 8196065: ListChangeListener getRemoved() returns items that were not removed. [v6]
On Wed, 26 May 2021 09:14:35 GMT, Jeanette Winzenburg
wrote:
>> Yes, `selectedItems` is broken. I agree that having an objectively incorrect
>> test pass is fishy, but the alternative would be to not have the test at
>> all, and lose the "free" heads-up if at some point in the future the
>> underlying issue is fixed. Do you think it's better to have the correct
>> test, but ignore it?
>
> well, false greens are the worst we can have, IMO :). I think the test should
> concentrate on the isolated selectedItems behavior which __must__ propagate
> all notifications that it receives (from indices) in terms of items. If it
> fails to do so, it's a failure of its own implementation and should produce a
> failing test.
>
> Consider two scenarios:
>
> A: indices firing 2 separate changes
>
> selectedIndices.replaceAll(i -> i == 0 ? 1 : 0);
> assertEquals(2, changes.size());
> assertEquals(change(replaced(0, range("foo"), range("bar"))),
> changes.get(0));
> assertEquals(change(replaced(1, range("bar"), range("foo"))),
> changes.get(1));
>
> B: indices firing a composed change:
>
> selectedIndices._beginChange();
> selectedIndices.replaceAll(i -> i == 0 ? 1 : 0);
> selectedIndices._endChange();
> assertEquals(indicesChanges.size(), changes.size());
> assertEquals(1, changes.size());
> assertEquals(change(replaced(0, range("foo", "bar"), range("bar",
> "foo"))), changes.get(0));
>
> B passes both before and after the fix, A fails both before and after the
> fix. Whether or not that can be helped currently, is a different story. If
> can't be solved right now, I would suggest to keep it failing, file an issue
> about it and ignore the test with the issue id.
I disabled the tests until the underlying
[issue](https://bugs.openjdk.java.net/browse/JDK-8267951) is fixed.
-
PR: https://git.openjdk.java.net/jfx/pull/478
Re: RFR: 8196065: ListChangeListener getRemoved() returns items that were not removed. [v6]
On Fri, 28 May 2021 14:31:14 GMT, Jeanette Winzenburg wrote: >>> You mean `selectedIndices` not reporting truthfully? >> >> Yes, I meant `selectedIndices`. >> >> I'm not quite sure I understand what you're getting at. Are you suggesting >> to apply the changes reported by `selectedIndices` to our copy of the items >> (`itemsRefList`) while iterating the changeset? If so, I think that doesn't >> work because crucial `selectedIndices` notifications are missing. >> >> For example, `selectedIndices` sometimes doesn't report removed items. By >> simply iterating over the changeset, `itemsRefList` would soon be out of >> sync with `selectedIndices`. The only way to correctly sync `itemsRefList` >> with `selectedIndices` is to discard all items, and collect them again in a >> bulk operation (like it's done currently). >> >> That alone doesn't make `SelectedItemsReadOnlyObservableList` well-behaved: >> since its change notifications are generated while iterating over the >> `selectedIndices` changes, it will also miss some notifications. > >> I'm not quite sure I understand what you're getting at. Are you suggesting >> to apply the changes reported by `selectedIndices` to our copy of the items >> (`itemsRefList`) while iterating the changeset? If so, I think that doesn't >> work because crucial `selectedIndices` notifications are missing. >> > > exactly, that's what I meant right from the start (and obviously haven't been > clear enough ;) And now I understand what you tried to explain to me, > probably also right from the start .. thanks for spelling it out! > > hmm .. so we are stuck with a severely misbehaving selectedItems that's > trying to do its best to compensate for the misbehaviour in selectedIndices > (there are not only missing but also badly incorrect, notifications see f.i. > [JDK-8267781](https://bugs.openjdk.java.net/browse/JDK-8267781)). There > probably should be some documentation to explain that fact. Maybe open a > follow-up issue to make sure the selectedItems are revisited? Yes, this should be followed up on. [Here](https://bugs.openjdk.java.net/browse/JDK-8267951)'s the issue. - PR: https://git.openjdk.java.net/jfx/pull/478
Re: RFR: 8196065: ListChangeListener getRemoved() returns items that were not removed. [v7]
> The documentation for `ObservableListBase.nextRemove` states that a single > change always refers to the current state of the list, which likely means > that multiple disjoint removed ranges need to be applied in order, otherwise > the next change's `getFrom` doesn't refer to the correct index. > > `SelectedItemsReadOnlyObservableList` doesn't apply removals to > `itemsRefList`, which means that subsequent removals will refer to the wrong > index when retrieving the removed elements. This PR fixes the calculation of > the current index. Michael Strauß has updated the pull request incrementally with one additional commit since the last revision: Ignored tests that fail for a different reason - Changes: - all: https://git.openjdk.java.net/jfx/pull/478/files - new: https://git.openjdk.java.net/jfx/pull/478/files/fbe8aa66..950c894a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx&pr=478&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jfx&pr=478&range=05-06 Stats: 25 lines in 1 file changed: 3 ins; 16 del; 6 mod Patch: https://git.openjdk.java.net/jfx/pull/478.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/478/head:pull/478 PR: https://git.openjdk.java.net/jfx/pull/478
Re: RFR: 8266396: Save VSCMD_DEBUG output in Windows build [v3]
On Tue, 11 May 2021 01:17:32 GMT, John Neffenger wrote:
>> The Windows build calls a series of batch files to get the Visual Studio
>> paths and environment variables. The batch files are a black box: any
>> messages they print are discarded. If anything goes wrong, the only signs
>> are a vague Gradle exception and a corrupted properties file.
>>
>> This has been causing problems for years. There are at least 49 messages on
>> the mailing since 2017 that discuss the exception and ways to work around it.
>>
>> This pull request lets you enable the batch file messages, shedding light on
>> their internal workings and allowing you to catch any errors at their
>> source. Specifically, it adds the variable `VSCMD_DEBUG` to the environment
>> of `genVSproperties.bat` and documents its use.
>>
>> ### Before
>>
>> For example, if your `PATH` environment variable is missing
>> `C:/Windows/System32`, like mine was, you'll see the following errors:
>>
>>
>> $ gradle sdk
>> Dependency verification is an incubating feature.
>>
>> FAILURE: Build failed with an exception.
>>
>> * Where:
>> Script 'C:\cygwin64\home\john\src\jfx\buildSrc\win.gradle' line: 108
>>
>> * What went wrong:
>> A problem occurred evaluating script.
>>> FAIL: WINSDK_DIR not defined
>>
>> ︙
>>
>> BUILD FAILED in 4s
>>
>>
>> *Me:* What's a `WINSDK_DIR`? The Wiki says, "This means that you will have
>> to define these paths manually." I guess this is normal. 😕️
>>
>> ### After
>>
>> With this change, you can set the debug level to "3" in the `win.gradle`
>> file and get a better picture of the problem:
>>
>>
>> $ gradle sdk
>> Dependency verification is an incubating feature.
>>
>>> Configure project :
>> 'reg' is not recognized as an internal or external command,
>> operable program or batch file.
>> 'reg' is not recognized as an internal or external command,
>> operable program or batch file.
>> 'reg' is not recognized as an internal or external command,
>> operable program or batch file.
>>
>> ︙
>>
>> 'powershell.exe' is not recognized as an internal or external command,
>> operable program or batch file.
>>
>> FAILURE: Build failed with an exception.
>>
>> * Where:
>> Script 'C:\cygwin64\home\john\src\jfx\buildSrc\win.gradle' line: 108
>>
>> * What went wrong:
>> A problem occurred evaluating script.
>>> FAIL: WINSDK_DIR not defined
>>
>> ︙
>>
>> BUILD FAILED in 3s
>>
>>
>> *Me:* Oh, it's just a "command not found" error. I'll check my `PATH`. 🤓
>
> John Neffenger has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Skip sending telemetry to fix "file in use" error
Finally had time to test this.
I think this is a welcome feature, although we should document it's usage in
the
[wiki](https://wiki.openjdk.java.net/display/OpenJFX/Building+OpenJFX#BuildingOpenJFX-Windows)
and where the log file is and what to search for.
Regarding my test, I did the following:
1. Changed the following `PATH` variable: `C:\WINDOWS\system32` ->
`C:\WINDOWS\system321`
2. run `gradle`
3. Then I got, as expected:
> FAIL: WINSDK_DIR not defined
Now I struggled a bit.
I set `export VSCMD_DEBUG=3` and tried to run `gradle` again. But same error
and no logfile. But turns out the whole build folder should be deleted first as
otherwise you will get the error again immediately. That's something we should
document then as well.
After deleting the build folder and rerun `gradle`, I finally had a huge log
and after a deep look I found the following: (Note: the `command not found`
error is written in german here 😄)
[DEBUG:winsdk.bat] specified /winsdk= was not found or was incomplete
C:\dev\Projects\IdeaProjects\OpenJFX>for /F "tokens=1,2*" %i in ('reg query
"HKLM\SOFTWARE\Wow6432Node\Microsoft\VisualStudio\VSPerf" /v
"CollectionToolsDir"') DO (if "%i" == "CollectionToolsDir" (SET
"__collection_tools=%k" ) )
Der Befehl "reg" ist entweder falsch geschrieben oder
konnte nicht gefunden werden.
C:\dev\Projects\IdeaProjects\OpenJFX>if "" == "" for /F "tokens=1,2*" %i in
('reg query "HKCU\SOFTWARE\Wow6432Node\Microsoft\Microsoft SDKs\Windows\v8.1"
/v "InstallationFolder"') DO (if "%i" == "InstallationFolder" (SET
WindowsSdkDir=%k ) )
Der Befehl "reg" ist entweder falsch geschrieben oder
konnte nicht gefunden werden.
This looks good to me and is helpful. And after documenting it, I think it will
be more clear for everyone including new developers. :)
-
PR: https://git.openjdk.java.net/jfx/pull/488
Result: New OpenJFX Reviewer: Jeanette Winzenburg
Voting for Jeanette Winzenburg [1] to OpenJFX Reviewer [2] is now closed. Yes: 7 Veto: 0 Abstain: 0 According to the Bylaws definition of Three-Vote Consensus, this is sufficient to approve the nomination. -- Kevin [1] https://openjdk.java.net/census#fastegal [2] https://mail.openjdk.java.net/pipermail/openjfx-dev/2021-May/030299.html
Result: New OpenJFX Reviewer: Arun Joseph
Voting for Arun Joseph [1] to OpenJFX Reviewer [2] is now closed. Yes: 6 Veto: 0 Abstain: 0 According to the Bylaws definition of Three-Vote Consensus, this is sufficient to approve the nomination. -- Kevin [1] https://openjdk.java.net/census#ajoseph [2] https://mail.openjdk.java.net/pipermail/openjfx-dev/2021-May/030298.html
Re: RFR: 8239138: StyleManager should use a BufferedInputStream [v3]
On Thu, 27 May 2021 17:22:35 GMT, Ambarish Rapte wrote: >> `StyleManager.calculateCheckSum()` uses a raw InputStream as the input to a >> `DigestInputStream` and reads one byte at a time. This is slower in >> performance and should be changed, either to use `BufferedInputStream` or >> read byte buffer of 4096 from the stream or use both. >> >> I have tried three approaches and tested with modena.css which is ~134 KB in >> size. >> Following are the approaches and time in milliseconds taken by the method >> StyleManager.calculateCheckSum(), collected from 25 readings, >> >> 1. Use BufferedInputStream and read one byte at a time >> [commit#1](https://github.com/openjdk/jfx/commit/6cd7d44d0ce1084c6cdb06a698c7aca127a615ef) >> : >> - Maximum: 53 ms, Minimum: 27 ms, Average: 39 ms >> 2. Use BufferedInputStream and read buffer of 4096 at a time >> [commit#1+2](https://github.com/openjdk/jfx/pull/518/files/6e0c621ea62691d5402b2bca5951d1012a5b1b91) >> - Maximum: 17 ms, Minimum: 14 ms, Average: 15.58 ms >> 3. Use raw InputStream(current implementation) and read buffer of 4096 at a >> time >> [commit#1+2+3](https://github.com/openjdk/jfx/pull/518/files/215e1a183cfb902247f0d48685f7a901cb5fb003), >> which also similar to `NativeLibLoader.calculateCheckSum()` and looks >> faster than previous two. >> - Maximum: 16 ms, Minimum: 13 ms, Average: 14.25 ms >> >> >> The time taken by `StyleManager.calculateCheckSum()` with current >> implementation is, >> - Maximum: 61 ms, Minimum: 38 ms, Average: 50.58 ms >> >> >> Both approaches 2 & 3 show good improvement. I would prefer approach#3 as it >> is similar to `NativeLibLoader.calculateCheckSum()`. >> However we can choose approach#2 also. >> If we choose approach#3 then bug summary should be changed accordingly. > > Ambarish Rapte has updated the pull request incrementally with one additional > commit since the last revision: > > review update Now that PR #523 is integrated, can you merge in the latest changes from master to ensure a clean test run? - PR: https://git.openjdk.java.net/jfx/pull/518
Integrated: 8267892: Add .gitattributes to repo
On Thu, 27 May 2021 22:14:51 GMT, Kevin Rushforth wrote: > This fix adds a `.gitattributes` file to prevent git from replacing line > endings if the global `.gitconfig` file on the client system is configured > with core.autocrlf = true (which it is by default when using the native Git > for Windows). > > This can cause build or test problems with scripts or other files that expect > Unix-style line endings. For two recent examples of such problems, see > [JDK-8267694](https://bugs.openjdk.java.net/browse/JDK-8267694) and [this > comment in PR > #518](https://github.com/openjdk/jfx/pull/518#issuecomment-849966053). > > To test this, I set `core.autocrlf = true` in my `$HOME/.gitconfig` and > cloned the master branch of the jfx repo into a new local repo. The files all > had CRLF line endings. I then cloned the branch with the fix for this PR into > a new repo and verified that the line endings are correctly left alone (LF > line endings). > > As a second test, here is a failing GHA build of the patch from PR #518 that > fails as expected due to this bug: > > https://github.com/kevinrushforth/jfx/actions/runs/883603338 > > Here is a GHA build of that same branch, plus the commit to add > `.gitattributes`: > > https://github.com/kevinrushforth/jfx/actions/runs/883620592 This pull request has now been integrated. Changeset: 5e6d4429 Author:Kevin Rushforth URL: https://git.openjdk.java.net/jfx/commit/5e6d4429159e3fab9ec0aab9720393850d179710 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8267892: Add .gitattributes to repo Reviewed-by: arapte, pbansal - PR: https://git.openjdk.java.net/jfx/pull/523
Re: RFR: 8196065: ListChangeListener getRemoved() returns items that were not removed. [v6]
On Wed, 26 May 2021 15:48:08 GMT, Michael Strauß wrote: > I'm not quite sure I understand what you're getting at. Are you suggesting to > apply the changes reported by `selectedIndices` to our copy of the items > (`itemsRefList`) while iterating the changeset? If so, I think that doesn't > work because crucial `selectedIndices` notifications are missing. > exactly, that's what I meant right from the start (and obviously haven't been clear enough ;) And now I understand what you tried to explain to me, probably also right from the start .. thanks for spelling it out! hmm .. so we are stuck with a severely misbehaving selectedItems that's trying to do its best to compensate for the misbehaviour in selectedIndices (there are not only missing but also badly incorrect, notifications see f.i. [JDK-8267781](https://bugs.openjdk.java.net/browse/JDK-8267781)). There probably should be some documentation to explain that fact. Maybe open a follow-up issue to make sure the selectedItems are revisited? - PR: https://git.openjdk.java.net/jfx/pull/478
RFR: 8267094: TreeCell: cancelEvent must return correct editing location
the bug is an incorrect edit location (for tree: treeItem) in edit cancel events - expected is the location at the time the cell edit was started, actual was the location of at the time the edit was cancelled. See the report for details. Fixed by storing the edit location in startEdit and use that in cancelEdit. Added tests that failed before and passed after and tests that (accidentally :) passed before and still pass after. - Commit messages: - 8267094: TreeCell: cancelEvent must return correct editing location Changes: https://git.openjdk.java.net/jfx/pull/524/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx&pr=524&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8267094 Stats: 123 lines in 3 files changed: 98 ins; 22 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/524.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/524/head:pull/524 PR: https://git.openjdk.java.net/jfx/pull/524
Tons of gradle warnings
Hi Experts, yesterday gradle auto-updated to version 7 (?), since then I get many warnings like: Execution optimizations have been disabled for task ':graphics:copyClassFilesWin' to ensure correctness due to the following reasons: - Gradle detected a problem with the following location: 'C:\Daten\data-for-work\eclipse\gitrep-openjdk\jfx-fork\modules\javafx.graphics\build\module-classes'. Reason: Task ':graphics:copyClassFilesWin' uses this output of task ':graphics:buildModuleWin' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Looks like it is safe to ignore them, just want to be sure :) Another very minor issue: since the same time, there's an issue with terminal output in that I see the "executing" messages written out like: <===--> 58% EXECUTING [54s]> :graphics:copyClassFilesWin<===--> 58% EXECUTING [55s]<===--> 58% EXECUTING [56s]<===--> 58% EXECUTING [57s]<===--> 58% EXECUTING [58s]<===--> 58% EXECUTING [59s]<===--> 58% EXECUTING [1m 0s]<===--> 58% EXECUTING [1m 1s]<===--> and-so-on-to-the-end Can live with it, but any idea how to get rid off them? -- Jeanette
