Re: RFR: 8242577: Cell selection fails on iOS most of the times
On Tue, 14 Apr 2020 10:31:18 GMT, Jose Pereda wrote: > There are cases when iOS sends one or more NSTouchPhaseMoved for a given > touch event, in between NSTouchPhaseBegan and > NSTouchPhaseEnded , even if the initial event location didn't change. > By default, all these events are emulated as mouse enter/down, drag and > up/exit events. > > However, when the user taps quickly or even holds steady on a cell, if these > touch moved events are generated and sent > as mouse drag events, the cell selection fails, as the flag `latePress` used > in > [CellBehaviorBase](https://github.com/openjdk/jfx/blob/master/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/CellBehaviorBase.java#L191) > is set to false, preventing the cell selection that should happen upon touch > release event. This PR prevents emulating > mouse drag events when the touch event doesn't change its location. looks good. - Marked as reviewed by jvos (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/181
Re: RFR: 8227425: Add support for e-paper displays on i.MX6 devices
On Thu, 5 Dec 2019 00:14:34 GMT, John Neffenger wrote: >> This pull request adds support for e-paper displays with the i.MX 6 Series >> of Applications Processors and implements >> [Issue #521](https://github.com/javafxports/openjdk-jfx/issues/521) in the >> obsolete *javafxports/openjdk-jfx* >> repository. Some of the changes were made to support the new device models, >> while others are minor changes to the >> existing support in JavaFX 13. The following changes were made to support >> the new device models. >> * Work around problems on the Kobo Glo HD Model N437. >> >> Ignore the error ENOTTY (25), "Inappropriate ioctl for device," when >> setting the waveform modes. The IOCTL call is >> ignored by the driver on the Kobo Glo HD where the error occurs, anyway. >> >> Use the visible y-resolution (`yres`), not the virtual y-resolution >> (`yres_virtual`), when calculating the capacity of >> the off-screen byte buffer and the length of the frame buffer mapping. >> The virtual y-resolution reported by the frame >> buffer device driver can be an incorrect value. >> >> * Work around problems on the Kobo Clara HD Model N249. >> >> The Kobo Clara HD requires the native screen width to be set to the >> visible x-resolution (`xres`), instead than the >> normal virtual x-resolution (`xres_virtual`), when using an unrotated >> and uninverted 8-bit grayscale frame buffer. The >> work-around is provided through a new Boolean system property called >> *monocle.epd.fixWidthY8UR*. >> >> The following changes were made to the existing code that supports e-paper >> displays in JavaFX 13. >> >> * Use the correct constant for the number of bytes per pixel. >> >> Use the number of bytes per pixel (`Integer.BYTES`), not the number of >> bits per pixel (`Integer.SIZE`), when >> calculating the capacity of the 32-bit off-screen byte buffer. >> >> * Do not round the luma value to the nearest integer. >> >> Use the value of luma rounded toward zero automatically by Java when >> converting a `float` to an `int` instead of >> rounding to the nearest integer. The additional rounding operation can >> decrease performance anywhere from zero to 10 >> percent and doesn't seem worth it for a display with just 16 levels of >> gray. >> >> * Change camel case of system property *y8inverted*. >> >> Change the camel case of the system property *monocle.epd.y8inverted* to >> the form *monocle.epd.Y8Inverted* so that it >> is consistent with the other system properties containing Y1, Y4, and Y8 >> in their names. >> >> * Improve error handling when mapping the frame buffer. >> >> Log the mapping and unmapping of the frame buffer. Log any errors that >> occur on either of the system calls. Handle a >> `null` return value from `EPDFrameBuffer.getMappedBuffer`. >> >> * Replace non-ASCII characters with their ASCII equivalent. >> >> Replace non-ASCII characters in comments and log messages as follows: >> >> * "×" with "x" for display resolution and color depth, >> * "×" with "*" for multiplication, >> * "→" with "to" for transition, and >> * "°C" with "degrees Celsius" for temperature. >> >> * Add descriptions to Monocle EPD system properties. >> >> Add Javadoc comments to each constant that defines a Monocle EPD system >> property. >> >> * Rename `IntStructure.getInteger` to `IntStructure.get`. >> >> Rename the methods `getInteger` and `setInteger` in `IntStructure` to >> avoid confusion with the Java method >> `Integer.getInteger`, which gets the integer value of a system property. >> >> Below are some of the more interesting test results. >> >> * The Kobo Glo HD and Kobo Clara HD implement a true GC4 waveform mode that >> displays only pixels with the four gray >> values `0x00`, `0x55`, `0xAA`, and `0xFF`. On the older Kobo Touch models, >> the GC4 waveform mode is the same as GC16. >> >> * When the *forceMonochrome* update flag is enabled, the Kobo Clara HD uses >> a zero-percent threshold that displays black >> for pixels with the value `0x00` and white for all other values. The other >> models use a 50-percent threshold that >> displays black for values `0x7F` or less and white for values `0x80` or >> greater. >> >> * The OpenJDK 11 package in Ubuntu 18.04 LTS runs twice as fast as the >> AdoptOpenJDK build of OpenJDK 13 for some of the >> tests. The speed difference is greatest for the tests that require pixel >> conversion into an 8-bit or 16-bit frame >> buffer. I plan to investigate the cause of the performance difference. >> >> * In general, the faster processor and memory bus of the newer models does >> not fully compensate for the threefold >> increase in the number of pixels in their displays. The table below shows >> the animation speed of each model in >> milliseconds per frame and frames per second. >> >> | Product Name | Speed (ms) | Rate (fps) | >> |
Re: [Rev 01] RFR: 8227425: Add support for e-paper displays on i.MX6 devices
> This pull request adds support for e-paper displays with the i.MX 6 Series of > Applications Processors and implements > [Issue #521](https://github.com/javafxports/openjdk-jfx/issues/521) in the > obsolete *javafxports/openjdk-jfx* > repository. Some of the changes were made to support the new device models, > while others are minor changes to the > existing support in JavaFX 13. The following changes were made to support > the new device models. > * Work around problems on the Kobo Glo HD Model N437. > > Ignore the error ENOTTY (25), "Inappropriate ioctl for device," when > setting the waveform modes. The IOCTL call is > ignored by the driver on the Kobo Glo HD where the error occurs, anyway. > > Use the visible y-resolution (`yres`), not the virtual y-resolution > (`yres_virtual`), when calculating the capacity of > the off-screen byte buffer and the length of the frame buffer mapping. > The virtual y-resolution reported by the frame > buffer device driver can be an incorrect value. > > * Work around problems on the Kobo Clara HD Model N249. > > The Kobo Clara HD requires the native screen width to be set to the > visible x-resolution (`xres`), instead than the > normal virtual x-resolution (`xres_virtual`), when using an unrotated and > uninverted 8-bit grayscale frame buffer. The > work-around is provided through a new Boolean system property called > *monocle.epd.fixWidthY8UR*. > > The following changes were made to the existing code that supports e-paper > displays in JavaFX 13. > > * Use the correct constant for the number of bytes per pixel. > > Use the number of bytes per pixel (`Integer.BYTES`), not the number of > bits per pixel (`Integer.SIZE`), when > calculating the capacity of the 32-bit off-screen byte buffer. > > * Do not round the luma value to the nearest integer. > > Use the value of luma rounded toward zero automatically by Java when > converting a `float` to an `int` instead of > rounding to the nearest integer. The additional rounding operation can > decrease performance anywhere from zero to 10 > percent and doesn't seem worth it for a display with just 16 levels of > gray. > > * Change camel case of system property *y8inverted*. > > Change the camel case of the system property *monocle.epd.y8inverted* to > the form *monocle.epd.Y8Inverted* so that it > is consistent with the other system properties containing Y1, Y4, and Y8 > in their names. > > * Improve error handling when mapping the frame buffer. > > Log the mapping and unmapping of the frame buffer. Log any errors that > occur on either of the system calls. Handle a > `null` return value from `EPDFrameBuffer.getMappedBuffer`. > > * Replace non-ASCII characters with their ASCII equivalent. > > Replace non-ASCII characters in comments and log messages as follows: > > * "×" with "x" for display resolution and color depth, > * "×" with "*" for multiplication, > * "→" with "to" for transition, and > * "°C" with "degrees Celsius" for temperature. > > * Add descriptions to Monocle EPD system properties. > > Add Javadoc comments to each constant that defines a Monocle EPD system > property. > > * Rename `IntStructure.getInteger` to `IntStructure.get`. > > Rename the methods `getInteger` and `setInteger` in `IntStructure` to > avoid confusion with the Java method > `Integer.getInteger`, which gets the integer value of a system property. > > Below are some of the more interesting test results. > > * The Kobo Glo HD and Kobo Clara HD implement a true GC4 waveform mode that > displays only pixels with the four gray > values `0x00`, `0x55`, `0xAA`, and `0xFF`. On the older Kobo Touch models, > the GC4 waveform mode is the same as GC16. > > * When the *forceMonochrome* update flag is enabled, the Kobo Clara HD uses a > zero-percent threshold that displays black > for pixels with the value `0x00` and white for all other values. The other > models use a 50-percent threshold that > displays black for values `0x7F` or less and white for values `0x80` or > greater. > > * The OpenJDK 11 package in Ubuntu 18.04 LTS runs twice as fast as the > AdoptOpenJDK build of OpenJDK 13 for some of the > tests. The speed difference is greatest for the tests that require pixel > conversion into an 8-bit or 16-bit frame > buffer. I plan to investigate the cause of the performance difference. > > * In general, the faster processor and memory bus of the newer models does > not fully compensate for the threefold > increase in the number of pixels in their displays. The table below shows > the animation speed of each model in > milliseconds per frame and frames per second. > > | Product Name | Speed (ms) | Rate (fps) | > | - |:--:|:--:| > | Kobo Touch B | 125| 8.0| > | Kobo Touch C | 130| 7.7| > | Kobo Glo HD |
Re: RFR: 8241582: Infinite animation does not start from the end when started with a negative rate
On Fri, 10 Apr 2020 06:31:39 GMT, Nir Lisker wrote: > ### Cause > > `Animation#jumpTo(Duration)` does not handle `Duration.INDEFINITE` properly. > This causes > `InfiniteClipEnvelope#jumpTo(long)` to receive an erroneous value and start > the animation not from the end, depending > on the modulo calculation. ### Fix > > For infinite cycles, use the `cycleDuration` as the destination since that is > the effective end point. > > ### Tests > > * Added an `testJumpTo_IndefiniteCycles` test that fails without the patch > and passes after it. > * Added a `testJumpTo_IndefiniteCycleDuration` that passes both before and > after, just to make sure that this type of > infinite duration is correct. > * Removed a test in `SequentialTransition` that fails with this patch, but it > passed before it only because the modulo > value happened to land in the right place. Changing the duration of one of > the child animation can cause it to fail, so > the test is faulty anyway at this stage. > > ### Future work > > Playing backwards will still not work correctly, but at least now it start > from the correct value. This is the first of > a series of fixes under the umbrella issue > [JDK-8210238](https://bugs.openjdk.java.net/browse/JDK-8210238). looks good to me. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/169
Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView
On Thu, 16 Apr 2020 09:45:20 GMT, dannygonzalez wrote: >> @dannygonzalez Could you perhaps debug your application and take a look at >> how large the following array is: a random >> node -> `scene` -> `value` -> `window` -> `readOnlyProperty` -> `helper` -> >> `changeListeners`. I just tested this with >> a custom control displaying 200 cells on screen at once (each cell >> consisting of about 30 nodes itself), and I saw >> about 2 change listeners registered on this single Scene Window >> property. However, this custom control is not >> creating/destroying cells beyond the initial allocation, so there wouldn't >> be any registering and unregistering going >> on, scrolling was still smooth >30 fps. > > @hjohn I have 12136 change listeners when debugging our application as you > suggested. > > Please note that I see the issue when the TableView is having items added to > it. If you just have a static TableView I > do not see the issue. > It is only when you add items to the TableView which causes a myriad of > listeners to be deregistered and registered. > The Visual VM snapshot I attached above was taken as our application was > adding items to the TableView. I've tested this pull request locally a few times, and the performance improvement is quite significant. A test with 20.000 nested stack panes resulted in these average times: - Add all 51 ms - Remove all 25 ms Versus the unmodified code: - Add all 34 ms - Remove all 135 ms However, there are some significant functional changes as well that might impact current users: 1. The new code ensures that all listeners are notified even if the list is modified during iteration by always making a **copy** when an event is fired. The old code only did so when it was **actually** modified during iteration. This can be mitigated by making the copy in the code that modifies the list (as the original did) using the `locked` flag to check whether an iteration was in progress. 2. There is a significant increase in memory use. Where before each listener occupied an entry in an array, now each listener is wrapped by `Map.Entry` (the Integer instance used per entry can be disregarded). I estimate around 4-8x more heap will be consumed (the numbers are small however, still well below 1 MB for 20.000 entries). If this is an issue, a further level could be introduced in the listener implementation hierarchy (singleton -> array -> map). 3. Even though the current version of this pull request takes care to notify duplicate listeners the correct amount of times, it does not notify them in the correct order with respect to other listeners. If one registers listeners (a, b, a) the notification order will be (a, a, b). The last point is not easily solved and could potentially cause problems. Finally I think this solution, although it performs well is not the full solution. A doubling or quadrupling of nodes would again run into serious limitations. I think this commit https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd should not have introduced another listener for each Node on the Window class. A better solution would be to only have the Scene listen to Window and have Scene provide a new combined status property that Node could use for its purposes. Even better however would be to change the properties involved to make use of the hierarchy naturally present in Nodes, having child nodes listen to their parent, and the top level node listen to the scene. This would reduce the amount of listeners on a single property in Scene and Window immensely, instead spreading those listeners over the Node hierarchy, keeping listener lists much shorter, which should scale a lot better. - PR: https://git.openjdk.java.net/jfx/pull/108
Re: [Rev 02] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
On Thu, 16 Apr 2020 15:04:53 GMT, Jeanette Winzenburg wrote: >> Macroscopic issue is that initially, the toggle is not sync'ed to the >> selection state. Root reason is an missing else >> block when updating toggle selection state (see report for details). >> Fixed by introducing the else block and removing all follow-up errors that >> tried to amend the consequences of the >> incorrect selection state >> - removed listener to selected item >> - removed toggle selection update in showing listener >> >> The former also fixed the memory leak when replacing the selectionModel plus >> an unreported NPE when the selectionModel >> is null initially. >> Added tests that failed before the fix and passed after. As there had been >> no tests around toggle state, so added some >> to verify that the change doesn't break. Enhanced shim/skin to allow access >> to popup for testing. Removed the >> informally ignored test part for memory leak. > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > ChoiceBox/-Skin/-SelectionTest > > - added code comments to clarify test/implemenation intention Marked as reviewed by arapte (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/177
Re: [Rev 02] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
> Macroscopic issue is that initially, the toggle is not sync'ed to the > selection state. Root reason is an missing else > block when updating toggle selection state (see report for details). > Fixed by introducing the else block and removing all follow-up errors that > tried to amend the consequences of the > incorrect selection state > - removed listener to selected item > - removed toggle selection update in showing listener > > The former also fixed the memory leak when replacing the selectionModel plus > an unreported NPE when the selectionModel > is null initially. > Added tests that failed before the fix and passed after. As there had been no > tests around toggle state, so added some > to verify that the change doesn't break. Enhanced shim/skin to allow access > to popup for testing. Removed the > informally ignored test part for memory leak. Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision: ChoiceBox/-Skin/-SelectionTest - added code comments to clarify test/implemenation intention - Changes: - all: https://git.openjdk.java.net/jfx/pull/177/files - new: https://git.openjdk.java.net/jfx/pull/177/files/0d3ce123..01aa7901 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/177/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/177/webrev.01-02 Stats: 5 lines in 2 files changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/177.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/177/head:pull/177 PR: https://git.openjdk.java.net/jfx/pull/177
Re: [Rev 01] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
On Thu, 16 Apr 2020 11:23:24 GMT, Ambarish Rapte wrote: >>> 1. do nothing for a (don't feel like filing yet another bug around >>> selection ;) and b (the skin behaves correctly, I >>> think) >> >> I am good with this. Though I will file a JBS for the correction in >> ChoiceBoxSelectionModel. >> `seletPrevious()`, `selectNext()` need one more check `value instanceof >> SeparatorMenuItem`. >> and similarly `selectFirst()` and `selectLast()` should be overridden >> correctly. >> and I can't think of why `select()` was changed so may be rethink about it >> :). >> We can discuss it again whenever we start fixing it. >> >> >>> 2. fix the test to be resistant against implementation changes of >>> selectionModel >> >> Thanks for link to >> [JDK-8088261](https://bugs.openjdk.java.net/browse/JDK-8088261), As >> mentioned in this bug >> description, "Culprit is an incorrect override of select(int): it may reject >> the new index if that would select a >> separator, but it must not select an arbitrary index instead", So It is not >> sure to me what should `select()` do in >> such scenario. So I think the test can also go as is, in case we change the >> behavior then test can be changed with it. > >> should be 8088261 (spelling error, I think) - is it okay to change them to >> the right id? > > That will be good to change, but not sure if as part of this bug. It will be > unrelated to fix. At the end and following your latest comments I did nothing Except adding code comments to the separator test and the else (toggle nulling) branch. - PR: https://git.openjdk.java.net/jfx/pull/177
Re: RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
On Thu, 9 Apr 2020 09:58:27 GMT, Jesper Skov wrote: > Make the two ways of associating a ToggleButton with a ToggleGroup interact > correctly. > > This fixes https://bugs.openjdk.java.net/browse/JDK-8198402 /signed - PR: https://git.openjdk.java.net/jfx/pull/167
RFR: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
Make the two ways of associating a ToggleButton with a ToggleGroup interact correctly. This fixes https://bugs.openjdk.java.net/browse/JDK-8198402 - Commit messages: - 8198402: Fix ToggleButton leak in ToggleGroup Changes: https://git.openjdk.java.net/jfx/pull/167/files Webrev: https://webrevs.openjdk.java.net/jfx/167/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8198402 Stats: 117 lines in 3 files changed: 102 ins; 10 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/167.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/167/head:pull/167 PR: https://git.openjdk.java.net/jfx/pull/167
Re: RFR: 8193286: IntegerSpinnerFactory does not wrap value correctly
On Wed, 15 Apr 2020 23:18:46 GMT, Kevin Rushforth wrote: >> Issue : https://bugs.openjdk.java.net/browse/JDK-8193286 >> >> Root Cause : >> Incorrect implementation. >> Current implementation of int wrapValue(int,int,int) in Spinner.java works >> well if min is 0. >> Hence this implementation works with ListSpinnerValueFactory, but fails with >> IntegerSpinnerValueFactory. >> >> Fix : >> Added a method for IntegerSpinnerValueFactory with separate implementation. >> >> Testing : >> Added unit tests to test increment/decrement in multiple steps and with >> different amountToStepBy values. >> >> Also, identified a related behavioral issue >> https://bugs.openjdk.java.net/browse/JDK-8242553, which will be addressed >> separately. > > I'm not sure this is quite right, but I need to look at it more closely. It > should fix the bug in question for > well-behaved values, but might make it possible to return a value outside the > range of [min, max]. > I also want to think about it in light of > [JDK-8242553](https://bugs.openjdk.java.net/browse/JDK-8242553). While a > partial fix seems OK, we need to avoid the situation where some values cause > a change from one buggy behavior to a > different buggy behavior on the way to fixing it right. The fix works for well behaved values - and - maintains buggy behavior (to be addressed in JDK-8242553) if (amountToStepBy * no of steps) is greater than the total numbers between min and max. The Spinner.wrapValue() does return value outside min-max range if (amountToStepBy * steps) is greater than total numbers between min and max. It gets clamped to either min or max based on condition in IntegerSpinnerValueFactory valueProperty() listener. I am planning to change Spinner.wrapValue() to use modulo as part of JDK-8242553. This fix is restricted to addressing the issue of stepping through of valid (amountToStepBy * no of steps) which was reported as bug. - PR: https://git.openjdk.java.net/jfx/pull/174
Re: [Rev 01] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
On Thu, 16 Apr 2020 11:18:55 GMT, Ambarish Rapte wrote:
>> btw: just noticed that there are methods in ChoiceBoxSkin testing the fix
>> for next/prev
>>
>> @Test public void test_jdk_8988261_selectNext() {
>> @Test public void test_jdk_8988261_selectPrevious() {
>>
>> the name look like they want to point to the corresponding issue .. but the
>> id is incorrect: that id doesn't exist,
>> should be 8088261 (spelling error, I think) - is it okay to change them to
>> the right id?
>
>> 1. do nothing for a (don't feel like filing yet another bug around
>> selection ;) and b (the skin behaves correctly, I
>> think)
>
> I am good with this. Though I will file a JBS for the correction in
> ChoiceBoxSelectionModel.
> `seletPrevious()`, `selectNext()` need one more check `value instanceof
> SeparatorMenuItem`.
> and similarly `selectFirst()` and `selectLast()` should be overridden
> correctly.
> and I can't think of why `select()` was changed so may be rethink about it :).
> We can discuss it again whenever we start fixing it.
>
>
>> 2. fix the test to be resistant against implementation changes of
>> selectionModel
>
> Thanks for link to
> [JDK-8088261](https://bugs.openjdk.java.net/browse/JDK-8088261), As mentioned
> in this bug
> description, "Culprit is an incorrect override of select(int): it may reject
> the new index if that would select a
> separator, but it must not select an arbitrary index instead", So It is not
> sure to me what should `select()` do in
> such scenario. So I think the test can also go as is, in case we change the
> behavior then test can be changed with it.
> should be 8088261 (spelling error, I think) - is it okay to change them to
> the right id?
That will be good to change, but not sure if as part of this bug. It will be
unrelated to fix.
-
PR: https://git.openjdk.java.net/jfx/pull/177
Re: [Rev 01] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
On Thu, 16 Apr 2020 09:17:19 GMT, Jeanette Winzenburg
wrote:
>> yeah, you are right:
>>
>> a) the implementation of ChoiceBoxSelectionModel is broken when it comes to
>> handling of unselectable items (such as
>> Separator): next/previous try to move on, the others simply select. The
>> implementation changed in fix of
>> [JDK-8088261](https://bugs.openjdk.java.net/browse/JDK-8088261) - before
>> select(index) tried to handle it, after this
>> was moved into next/previous. Arguably, the model can do what it wants
>> without specification ;) b) the skin is
>> responsible to sync the selection state of its toggles with the state of
>> model: if the selectedIndex/Item does not have
>> a corresponding toggle (f.i. if it's a separator), all toggles must be
>> unselected. c) my test related to the
>> Separator is broken - as you correctly noted, it will fail if a future
>> implementation decides to select a really
>> selectable item :) My plan:
>> 1. do nothing for a (don't feel like filing yet another bug around selection
>> ;) and b (the skin behaves correctly, I
>> think) 2. fix the test to be resistant against implementation changes of
>> selectionModel
>>
>> Thanks for the extensive review, very much appreciated :)
>
> btw: just noticed that there are methods in ChoiceBoxSkin testing the fix for
> next/prev
>
> @Test public void test_jdk_8988261_selectNext() {
> @Test public void test_jdk_8988261_selectPrevious() {
>
> the name look like they want to point to the corresponding issue .. but the
> id is incorrect: that id doesn't exist,
> should be 8088261 (spelling error, I think) - is it okay to change them to
> the right id?
> 1. do nothing for a (don't feel like filing yet another bug around
> selection ;) and b (the skin behaves correctly, I
> think)
I am good with this. Though I will file a JBS for the correction in
ChoiceBoxSelectionModel.
`seletPrevious()`, `selectNext()` need one more check `value instanceof
SeparatorMenuItem`.
and similarly `selectFirst()` and `selectLast()` should be overridden correctly.
and I can't think of why `select()` was changed so may be rethink about it :).
We can discuss it again whenever we start fixing it.
> 2. fix the test to be resistant against implementation changes of
> selectionModel
Thanks for link to
[JDK-8088261](https://bugs.openjdk.java.net/browse/JDK-8088261), As mentioned
in this bug
description, "Culprit is an incorrect override of select(int): it may reject
the new index if that would select a
separator, but it must not select an arbitrary index instead", So It is not
sure to me what should `select()` do in
such scenario. So I think the test can also go as is, in case we change the
behavior then test can be changed with it.
-
PR: https://git.openjdk.java.net/jfx/pull/177
Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView
On Thu, 16 Apr 2020 08:46:26 GMT, John Hendrikx wrote: >> If it is of any help, I have attached a VisualVM snapshot (v1.4.4) where the >> ExpressionHelper.removeListener is using >> 61% of the JavaFX thread whilst running our application. >> [snapshot-1587024308245.nps.zip](https://github.com/openjdk/jfx/files/4485728/snapshot-1587024308245.nps.zip) >> >> If you show only the JavaFX Application thread, press the "HotSpot" and >> "Reverse Calls" button you can take a look to >> see which classes are calling the removeListener function. >>  > > @dannygonzalez Could you perhaps debug your application and take a look at > how large the following array is: a random > node -> `scene` -> `value` -> `window` -> `readOnlyProperty` -> `helper` -> > `changeListeners`. I just tested this with > a custom control displaying 200 cells on screen at once (each cell consisting > of about 30 nodes itself), and I saw > about 2 change listeners registered on this single Scene Window property. > However, this custom control is not > creating/destroying cells beyond the initial allocation, so there wouldn't be > any registering and unregistering going > on, scrolling was still smooth >30 fps. @hjohn I have 12136 change listeners when debugging our application as you suggested. Please note that I see the issue when the TableView is having items added to it. If you just have a static TableView I do not see the issue. It is only when you add items to the TableView which causes a myriad of listeners to be deregistered and registered. The Visual VM snapshot I attached above was taken as our application was adding items to the TableView. - PR: https://git.openjdk.java.net/jfx/pull/108
Re: [Rev 01] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
On Thu, 16 Apr 2020 09:05:12 GMT, Jeanette Winzenburg
wrote:
>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ChoiceBoxSkin.java
>> line 416:
>>
>>> 415: } else {
>>> 416: toggleGroup.selectToggle(null);
>>> 417: }
>>
>> The `else` part here means that user programmatically has selected a
>> `Separator` or `SeparatorMenuItem`. The behavior
>> in such scenario is not defined in doc but the methods, `select()`,
>> `selectNext()`, `selectPrevious()` of
>> `ChoiceBox.ChoiceBoxSelectionModel` imply that if index points to a
>> `Separator` then a valid item should be selected.
>> However these method do handle this correctly. If these methods are
>> implemented such that no Separator is allowed to
>> be selected then this `else` part would not be needed and we might be able
>> to remove the `instanceof` checks. The fix
>> in this PR looks good to me. But we should also decide the behavior in above
>> scenario and may be file a JBS. If we
>> decide that when a `Separator` is chosen for selection then the current
>> selection should not be changed or a valid item
>> should be selected, then the test related to Separator selection need to be
>> changed. Or all of it can be handled in a
>> follow on issue.
>
> yeah, you are right:
>
> a) the implementation of ChoiceBoxSelectionModel is broken when it comes to
> handling of unselectable items (such as
> Separator): next/previous try to move on, the others simply select. The
> implementation changed in fix of
> [JDK-8088261](https://bugs.openjdk.java.net/browse/JDK-8088261) - before
> select(index) tried to handle it, after this
> was moved into next/previous. Arguably, the model can do what it wants
> without specification ;) b) the skin is
> responsible to sync the selection state of its toggles with the state of
> model: if the selectedIndex/Item does not have
> a corresponding toggle (f.i. if it's a separator), all toggles must be
> unselected. c) my test related to the
> Separator is broken - as you correctly noted, it will fail if a future
> implementation decides to select a really
> selectable item :) My plan:
> 1. do nothing for a (don't feel like filing yet another bug around selection
> ;) and b (the skin behaves correctly, I
> think) 2. fix the test to be resistant against implementation changes of
> selectionModel
>
> Thanks for the extensive review, very much appreciated :)
btw: just noticed that there are methods in ChoiceBoxSkin testing the fix for
next/prev
@Test public void test_jdk_8988261_selectNext() {
@Test public void test_jdk_8988261_selectPrevious() {
the name look like they want to point to the corresponding issue .. but the id
is incorrect: that id doesn't exist,
should be 8088261 (spelling error, I think) - is it okay to change them to the
right id?
-
PR: https://git.openjdk.java.net/jfx/pull/177
Re: [Rev 01] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
On Wed, 15 Apr 2020 15:52:13 GMT, Ambarish Rapte wrote:
>> Jeanette Winzenburg has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> ChoiceBox: added FIXME with reference to issue
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ChoiceBoxSkin.java
> line 347:
>
>> 346: // Test only purpose
>> 347: ContextMenu getChoiceBoxPopup() {
>> 348: return popup;
>
> I would recommend to prefix the method name with `test_.` It is not followed
> across, only `TabPaneSkin` has `test_`
> prefixed method.
well, as TabPaneSkin is a singularity in controls (in graphics such a pattern
seems to be wider-spread), I would prefer
not to do it here - also because ChoiceBoxSkin has another test-only accessor
(for label text) which doesn't. Might be
a candidate for a general cleanup task - if we decide to really introduce such
a naming convention (which I personally
wouldn't like :)
-
PR: https://git.openjdk.java.net/jfx/pull/177
Re: [Rev 01] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
On Wed, 15 Apr 2020 15:46:16 GMT, Ambarish Rapte wrote:
>> Jeanette Winzenburg has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> ChoiceBox: added FIXME with reference to issue
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ChoiceBoxSkin.java
> line 416:
>
>> 415: } else {
>> 416: toggleGroup.selectToggle(null);
>> 417: }
>
> The `else` part here means that user programmatically has selected a
> `Separator` or `SeparatorMenuItem`. The behavior
> in such scenario is not defined in doc but the methods, `select()`,
> `selectNext()`, `selectPrevious()` of
> `ChoiceBox.ChoiceBoxSelectionModel` imply that if index points to a
> `Separator` then a valid item should be selected.
> However these method do handle this correctly. If these methods are
> implemented such that no Separator is allowed to
> be selected then this `else` part would not be needed and we might be able to
> remove the `instanceof` checks. The fix
> in this PR looks good to me. But we should also decide the behavior in above
> scenario and may be file a JBS. If we
> decide that when a `Separator` is chosen for selection then the current
> selection should not be changed or a valid item
> should be selected, then the test related to Separator selection need to be
> changed. Or all of it can be handled in a
> follow on issue.
yeah, you are right:
a) the implementation of ChoiceBoxSelectionModel is broken when it comes to
handling of unselectable items (such as
Separator): next/previous try to move on, the others simply select. The
implementation changed in fix of
[JDK-8088261](https://bugs.openjdk.java.net/browse/JDK-8088261) - before
select(index) tried to handle it, after this
was moved into next/previous. Arguably, the model can do what it wants without
specification ;)
b) the skin is responsible to sync the selection state of its toggles with the
state of model: if the
selectedIndex/Item does not have a corresponding toggle (f.i. if it's a
separator), all toggles must be unselected.
c) my test related to the Separator is broken - as you correctly noted, it will
fail if a future implementation decides
to select a really selectable item :)
My plan:
1. do nothing for a (don't feel like filing yet another bug around selection ;)
and b (the skin behaves correctly, I
think) 2. fix the test to be resistant against implementation changes of
selectionModel
Thanks for the extensive review, very much appreciated :)
-
PR: https://git.openjdk.java.net/jfx/pull/177
Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView
On Thu, 16 Apr 2020 08:24:16 GMT, dannygonzalez wrote: >> The listeners added by `Node` are apparently internally required for >> internal properties `TreeShowing` and >> `TreeVisible`, and are used to take various decisions like whether to >> play/pause animations. There is also a couple of >> listeners registering on these properties in turn (in `PopupWindow`, >> `SwingNode`, `WebView` and `MediaView`). A lot of >> the checks for visibility / showing could easily be done by using the >> `Scene` property and checking visibility / >> showing status from there. No need for so many listeners. The other >> classes mentioned might register their own >> listener, instead of having `Node` do it for them (and thus impacting >> *every* node). Alternatively, `Node` may lazily >> register the listeners for Scene.Window and Window.Showing only when needed >> (which from what I can see is for pretty >> specific classes only, not classes that you'd see a lot in a TableView...) > > If it is of any help, I have attached a VisualVM snapshot (v1.4.4) where the > ExpressionHelper.removeListener is using > 61% of the JavaFX thread whilst running our application. > [snapshot-1587024308245.nps.zip](https://github.com/openjdk/jfx/files/4485728/snapshot-1587024308245.nps.zip) > > If you show only the JavaFX Application thread, press the "HotSpot" and > "Reverse Calls" button you can take a look to > see which classes are calling the removeListener function. >  @dannygonzalez Could you perhaps debug your application and take a look at how large the following array is: a random node -> `scene` -> `value` -> `window` -> `readOnlyProperty` -> `helper` -> `changeListeners`. I just tested this with a custom control displaying 200 cells on screen at once (each cell consisting of about 30 nodes itself), and I saw about 2 change listeners registered on this single Scene Window property. However, this custom control is not creating/destroying cells beyond the initial allocation, so there wouldn't be any registering and unregistering going on, scrolling was still smooth >30 fps. - PR: https://git.openjdk.java.net/jfx/pull/108
Re: RFR: 8242530: [macos] Some audio files miss spectrum data when another audio file plays first
On Wed, 15 Apr 2020 23:17:56 GMT, Alexander Matveev wrote: > https://bugs.openjdk.java.net/browse/JDK-8242530 > > - GstElementClass which is base class for all elements has same instance > between all spectrum elements (not sure if it is > same for all elements) and thus post_message was sending events to > AVFoundation callback from GStreamer platform. This > issue is reproducible if we load OSXPlatfrom first and then play media > files using GStreamer. Fixed by introducing > separate callback to send messages to AVFoundation. looks good to me... - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/184
Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView
On Thu, 16 Apr 2020 07:34:40 GMT, John Hendrikx wrote: >> Looking at the commit >> https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd >> it seems that the long listener lists are actually part of the `Scene`'s >> `Window` property and the `Window`'s `Showing` >> property. Each `Node` registers itself on those and so the listener lists >> for those properties would scale with the >> number of nodes. >> >> A test case showing this problem would really be great as then the patch can >> also be verified to solve the problem, but >> I suppose it could be reproduced simply by having a large number of Nodes in >> a scene. @dannygonzalez could you give us >> an idea how many Nodes we're talking about? 1000? 10.000? It also means >> there might be other options, do Nodes really >> need to add these listeners and for which functionality and are there >> alternatives? It would also be possible to >> target only these specific properties with an optimized listener list to >> reduce the impact of this change. > > The listeners added by `Node` are apparently internally required for internal > properties `TreeShowing` and > `TreeVisible`, and are used to take various decisions like whether to > play/pause animations. There is also a couple of > listeners registering on these properties in turn (in `PopupWindow`, > `SwingNode`, `WebView` and `MediaView`). A lot of > the checks for visibility / showing could easily be done by using the `Scene` > property and checking visibility / > showing status from there. No need for so many listeners. The other classes > mentioned might register their own > listener, instead of having `Node` do it for them (and thus impacting *every* > node). Alternatively, `Node` may lazily > register the listeners for Scene.Window and Window.Showing only when needed > (which from what I can see is for pretty > specific classes only, not classes that you'd see a lot in a TableView...) If it is of any help, I have attached a VisualVM snapshot (v1.4.4) where the ExpressionHelper.removeListener is using 61% of the JavaFX thread whilst running our application. [snapshot-1587024308245.nps.zip](https://github.com/openjdk/jfx/files/4485728/snapshot-1587024308245.nps.zip) If you show only the JavaFX Application thread, press the "HotSpot" and "Reverse Calls" button you can take a look to see which classes are calling the removeListener function.  - PR: https://git.openjdk.java.net/jfx/pull/108
Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView
On Thu, 16 Apr 2020 07:11:58 GMT, John Hendrikx wrote: >> The patch proposed here does not share the case where the listener deletion >> performance becomes a bottleneck. >> >> I think that it is necessary to reproduce it by testing first, but >> >> If you just focus on improving listener removal performance, >> >> If the lifespan of a large number of registered listeners is biased, >> It seems like the next simple change can improve delete performance without >> changing the data structure. >> >> * Change the search from the front of the list to the search from the back. >> >> This will reduce the number of long-life listeners matching. > > Looking at the commit > https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd > it seems that the long listener lists are actually part of the `Scene`'s > `Window` property and the `Window`'s `Showing` > property. Each `Node` registers itself on those and so the listener lists > for those properties would scale with the > number of nodes. > > A test case showing this problem would really be great as then the patch can > also be verified to solve the problem, but > I suppose it could be reproduced simply by having a large number of Nodes in > a scene. @dannygonzalez could you give us > an idea how many Nodes we're talking about? 1000? 10.000? It also means > there might be other options, do Nodes really > need to add these listeners and for which functionality and are there > alternatives? It would also be possible to > target only these specific properties with an optimized listener list to > reduce the impact of this change. The listeners added by `Node` are apparently internally required for internal properties `TreeShowing` and `TreeVisible`, and are used to take various decisions like whether to play/pause animations. There is also a couple of listeners registering on these properties in turn (in `PopupWindow`, `SwingNode`, `WebView` and `MediaView`). A lot of the checks for visibility / showing could easily be done by using the `Scene` property and checking visibility / showing status from there. No need for so many listeners. The other classes mentioned might register their own listener, instead of having `Node` do it for them (and thus impacting *every* node). Alternatively, `Node` may lazily register the listeners for Scene.Window and Window.Showing only when needed (which from what I can see is for pretty specific classes only, not classes that you'd see a lot in a TableView...) - PR: https://git.openjdk.java.net/jfx/pull/108
Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView
On Thu, 16 Apr 2020 06:34:39 GMT, yosbits wrote: >> @hjohn >> I haven't quantified exactly where all the listeners are coming from but the >> Node class for example has various >> listeners on it. >> The following changeset (which added an extra listener on the Node class) >> impacted TableView performance significantly >> (although it was still bad before this change in my previous tests): >>> commit e21606d3a1b73cd4b44383babc750a4b4721edfd >>> Author: Florian Kirmaier mailto:fk at sandec.de>> >>> Date: Tue Jan 22 09:08:36 2019 -0800 >>> >>> 8216377: Fix memoryleak for initial nodes of Window >>> 8207837: Indeterminate ProgressBar does not animate if content is added >>> after scene is set on window >>> >>> Add or remove the windowShowingChangedListener when the scene changes >> >> As you can imagine, a TableView with many columns and rows can be made up of >> many Node classes. The impact of having >> multiple listeners deregistering on the Node when new rows are being added >> to a TableView can be a significant >> performance hit on the JavaFX thread. I don't have the time or knowledge >> to investigate why these listeners are all >> needed especially around the Node class. I went directly to the source of >> the problem which was the linear search to >> deregister each listener. I have been running my proposed fix in our JavaFX >> application for the past few months and it >> has saved it from being unusable due to the JavaFx thread being swamped. > > The patch proposed here does not share the case where the listener deletion > performance becomes a bottleneck. > > I think that it is necessary to reproduce it by testing first, but > > If you just focus on improving listener removal performance, > > If the lifespan of a large number of registered listeners is biased, > It seems like the next simple change can improve delete performance without > changing the data structure. > > * Change the search from the front of the list to the search from the back. > > This will reduce the number of long-life listeners matching. Looking at the commit https://github.com/openjdk/jfx/commit/e21606d3a1b73cd4b44383babc750a4b4721edfd it seems that the long listener lists are actually part of the `Scene`'s `Window` property and the `Window`'s `Showing` property. Each `Node` registers itself on those and so the listener lists for those properties would scale with the number of nodes. A test case showing this problem would really be great as then the patch can also be verified to solve the problem, but I suppose it could be reproduced simply by having a large number of Nodes in a scene. @dannygonzalez could you give us an idea how many Nodes we're talking about? 1000? 10.000? It also means there might be other options, do Nodes really need to add these listeners and for which functionality and are there alternatives? It would also be possible to target only these specific properties with an optimized listener list to reduce the impact of this change. - PR: https://git.openjdk.java.net/jfx/pull/108
Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView
On Wed, 15 Apr 2020 07:11:27 GMT, dannygonzalez wrote: >> @dannygonzalez You mentioned "There are multiple change listeners on a Node >> for example, so you can imagine with the >> number of nodes in a TableView this is going to be a problem if the >> unregistering is slow.". >> Your changes in this PR seem to be focused on improving performance of >> unregistering listeners when there are thousands >> of listeners listening on changes or invalidations of the **same** property. >> Is this actually the case? Is there a >> single property in TableView or TreeTableView where such a high amount of >> listeners are present? If so, I think the >> problem should be solved there. As it is, I donot think this PR will help >> unregistration performance of listeners when >> the listeners are spread out over dozens of properties, and although high >> in total number, the total listeners per >> property would be low and their listener lists would be short. Performance >> of short lists (<50 entries) is almost >> unbeatable, so it is relevant for this PR to know if there are any >> properties with long listener lists. > > @hjohn > I haven't quantified exactly where all the listeners are coming from but the > Node class for example has various > listeners on it. > The following changeset (which added an extra listener on the Node class) > impacted TableView performance significantly > (although it was still bad before this change in my previous tests): >> commit e21606d3a1b73cd4b44383babc750a4b4721edfd >> Author: Florian Kirmaier mailto:fk at sandec.de>> >> Date: Tue Jan 22 09:08:36 2019 -0800 >> >> 8216377: Fix memoryleak for initial nodes of Window >> 8207837: Indeterminate ProgressBar does not animate if content is added >> after scene is set on window >> >> Add or remove the windowShowingChangedListener when the scene changes > > As you can imagine, a TableView with many columns and rows can be made up of > many Node classes. The impact of having > multiple listeners deregistering on the Node when new rows are being added to > a TableView can be a significant > performance hit on the JavaFX thread. I don't have the time or knowledge to > investigate why these listeners are all > needed especially around the Node class. I went directly to the source of the > problem which was the linear search to > deregister each listener. I have been running my proposed fix in our JavaFX > application for the past few months and it > has saved it from being unusable due to the JavaFx thread being swamped. If the lifespan of a large number of registered listeners is biased, It seems like the next simple change can improve delete performance without changing the data structure. * Change the search from the front of the list to the search from the back. This will reduce the number of long-life listeners matching. - PR: https://git.openjdk.java.net/jfx/pull/108
