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
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. Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/184
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. @arapte Can you also review this? - PR: https://git.openjdk.java.net/jfx/pull/184
RFR: 8242530: [macos] Some audio files miss spectrum data when another audio file plays first
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. - Commit messages: - 8242530: [macos] Some audio files miss spectrum data when another audio file plays first Changes: https://git.openjdk.java.net/jfx/pull/184/files Webrev: https://webrevs.openjdk.java.net/jfx/184/webrev.00 Issue: https://bugs.openjdk.java.net/browse/JDK-8242530 Stats: 23 lines in 3 files changed: 19 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/184.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/184/head:pull/184 PR: https://git.openjdk.java.net/jfx/pull/184
Re: RFR: 8193286: IntegerSpinnerFactory does not wrap value correctly
On Mon, 13 Apr 2020 06:59:08 GMT, Ajit Ghaisas 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. - PR: https://git.openjdk.java.net/jfx/pull/174
Re: Proposed IntegerSpinner buggy behavior correction - JDK-8242553
I just took another look at the SpinnerValueFactory API docs. The use of the term "circular" heavily implies modulo arithmetic as the expected behavior if wrapAround is true. That the usual meaning of "wrap" versus "clamp" when you have a range bounded on both ends. Maybe the confusion comes from the fact that it isn't stated as clearly as it might be, coupled with a single example of a step by one going from max to min (if incrementing). I note that example doesn't say what the amountToStepBy is, but it wasn't trying to be prescriptive. Since the current behavior of "wrap around unless you happen to wrap a bit too much and then we'll clamp" doesn't make sense in any universe that I can think of, it is fine to treat this as a bug. I'm not worried about "bug compatibility" here. Clarifying the spec at the same time seems like a good idea to me. A related issue is that the default value of wrapAround is not specified (the default is `false`, but you wouldn't know that from reading the docs). This should also be addressed at the same time. You mentioned that this is specific to IntegerValueFactory, but it looks like DoubleValueFactory (and maybe ListValueFactory) have the same bug. Or am I missing something? On an unrelated note, I just noticed that the SpinnerValueFactory constructor has no documentation. This is because it is an implicitly added constructor, which is an anti-pattern for public API. I'll file a separate issue for that. -- Kevin On 4/15/2020 2:23 AM, Jeanette Winzenburg wrote: Hi Ajit, yes, I read the doc, probably a bit differently - could well be my misunderstanding and misunderstandable wording :) Trying again: - I read your suggestion (in https://bugs.openjdk.java.net/browse/JDK-8242553) to imply f.i. that being at value and incrementing a full-cycle (that is max -min +1), I will land on value again - for me the doc seemed to imply that in such a case I would land on min. Though, given the "circular" as you pointed out correctly, was my misunderstanding - the current implementation is buggy (https://bugs.openjdk.java.net/browse/JDK-8193286) in calculating the remainder (which is what the first bullet amounts to) incorrectly for min != 0 Where do I err? -- Jeanette Zitat von Ajit Ghaisas : Hi Jeanette, The doc never assumes amountPerStep = 1. I am quoting it here - “The wrapAround property is used to specify whether the value factory should be circular. For example, should an integer-based value model increment from the maximum value back to the minimum value (and vice versa).” The word “circular” clarifies that once we exceed maximum value, we should start from minimum. I think, the doc is OK in it’s current form, but implementation needs to be corrected. Regards, Ajit On 14-Apr-2020, at 8:01 PM, Jeanette Winzenburg wrote: Hi Ajit, thought the doc was simply bad (in specifying the behavior for amountPerStep = 1 and not thinking of larger amounts) - my expection is a calculated wrap, that is the target as you suggest via modulo the difference from current value. Don't know if anybody took the doc literally .. -- Jeanette Zitat von Ajit Ghaisas : Hi, Once I fix JDK-8193286, I would like to take up JDK-8242553 (IntegerSpinner does not wrap around values correctly if amountToStepBy is larger than total numbers between Max and Min) The current implementation is not as per what is documented. Refer : https://openjfx.io/javadoc/14/javafx.controls/javafx/scene/control/SpinnerValueFactory.html#wrapAroundProperty I propose to fix the current buggy behavior of IntegerSpinner. Although it is a corner case, I would like to know if anybody relies on this buggy behavior? Regards, Ajit
Re: [Rev 03] RFR: 8238954: Improve performance of tiled snapshot rendering
On Wed, 15 Apr 2020 17:20:55 GMT, Frederic Thevenet wrote: >> At first glance, the NPE in >> [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082) occurs in >> the Prism layer, >> which is one level _below_ Quantum where the tiling is currently >> implemented, so I'm not sure tit is reachable from >> there; if we want the code to be shared, it looks like it would need to be >> moved even further down (maybe in the >> `ResourceFactory`?) Also, while reusing code is generally the way to go, in >> such lower layers, very closely >> intertwined with the actual rendering, I'm afraid that insisting on having a >> "one-size-fits-all" implementation might >> get in the way of necessary case-by-case optimizations, so I'd like to have >> someone with a deeper knowledge of the >> code base to weight in before starting work in that direction. Maybe >> @kevinrushforth could advise? > > Hi everyone, > > This PR hasn't seen much activity in a while, so I though I would give it a > gentle kick to hopefully get it moving > again ;) As explained above, I feel a little stalled at the moment, as we > need to decide whether or not it is a good > idea to try and address all or part of JDK-8189082 within the scope of this > PR, and I don't feel like I can settle > that on my own. Thanks. Sorry for the delay. This is on my review queue, which has been growing of late. I'll take a look at it soon. To answer one of your questions: > we need to decide whether or not it is a good idea to try and address all or > part of JDK-8189082 within the scope of > this PR I think your idea of addressing the other similar cases (Canvas and SubScene in particular) in a follow-up PR seems fine. If, at that time, you find you can refactor this to share some of the implementation, that would be good. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Wed, 15 Apr 2020 20:59:29 GMT, Kevin Rushforth wrote: >> I think @arapte has a similar MacBookPro model to mine. >> >> I think @prrace might be able to test it (I'll sync with him offline). > > Here are the results on Phil's machine, which is a Mac Book Pro with a > graphics accelerator (Nvidia, I think). > > Without the patch: > 2000 quads average 8.805 fps > > With the patch: > 2000 quads average 4.719 fps > > Almost a 2x performance hit. Conclusion: The new shaders that support attenuation don't seem to have much of a performance impact on machines with an Intel HD, but on systems with a graphics accelerator, it is a significant slowdown. So we are left with the two choices of doubling the number of shaders (that is, a set of shaders with attenuation and a set without) or living with the performance hit (which will only be a problem on machines with a dedicated graphics accelerator for highly fill-limited scenes). The only way we can justify a 2x drop in performance is if we are fairly certain that this is a corner case, and thus unlikely to hit real applications. If we do end up deciding to replicate the shaders, I don't think it is all that much work. I'm more worried about how well it would scale to subsequent improvements, although we could easily decide that for, say, spotlights attenuation is so common that you wouldn't create a version that doesn't do that. In the D3D HLSL shaders, ifdefs are used, so the work would be to restore the original code and add the new code under an ifdef. Then double the number of lines of gradle (at that point, I'd do it in a for-each loop), then modify the logic that loads the shaders to pick the right one. For GLSL, the different parts of the shader are in different files, so it's a matter of creating new versions of each of the three lighting shaders that handle attenuation and choosing the right one at runtime. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Tue, 7 Apr 2020 23:03:21 GMT, Kevin Rushforth wrote: >> @arapte Can you please test the performance changes too? > > I think @arapte has a similar MacBookPro model to mine. > > I think @prrace might be able to test it (I'll sync with him offline). Here are the results on Phil's machine, which is a Mac Book Pro with a graphics accelerator (Nvidia, I think). Without the patch: 2000 quads average 8.805 fps With the patch: 2000 quads average 4.719 fps Almost a 2x performance hit. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 02] RFR: 8223298: SVG patterns are drawn wrong
On Wed, 15 Apr 2020 15:14:49 GMT, Arun Joseph wrote: >> Issue: Assuming the pixelScale is 2, the tile image size is doubled at the >> native side which is propagated to the java >> side as well. But, as transform initialization takes place after scaling, >> the transform is reset to default value. >> Fix: Override scale() method in WCBufferedContext class to call init() >> before scaling. > > Arun Joseph has updated the pull request incrementally with one additional > commit since the last revision: > > Added SVGTest Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/164
Re: [Rev 03] RFR: 8238954: Improve performance of tiled snapshot rendering
On Tue, 17 Mar 2020 15:31:20 GMT, Frederic Thevenet wrote: >>> >>> >>> Now that the tiling is done in the `QuantumRenderer` level, I'll bring back >>> [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082). Can't this >>> tiling be used to fix that? >> >> It won't help with things in their current state, as the `RenderToImage` >> method that is modified in this PR is >> currently called only by the snapshot feature in `Scene` But of course I >> suspect the same technique could also be used >> to solve [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082) and >> now is indeed a good time to investigate >> it further. I'll have a look to see if it is possible to factorize the >> tiling implementation for both classes of >> issues, in which case it would make sense to do prior to merging this PR. If >> it is different enough that the >> implementation cannot be shared but only the general idea, then I suggest we >> address it in a different PR. What do you >> all think ? > > At first glance, the NPE in > [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082) occurs in the > Prism layer, > which is one level _below_ Quantum where the tiling is currently implemented, > so I'm not sure tit is reachable from > there; if we want the code to be shared, it looks like it would need to be > moved even further down (maybe in the > `ResourceFactory`?) Also, while reusing code is generally the way to go, in > such lower layers, very closely > intertwined with the actual rendering, I'm afraid that insisting on having a > "one-size-fits-all" implementation might > get in the way of necessary case-by-case optimizations, so I'd like to have > someone with a deeper knowledge of the > code base to weight in before starting work in that direction. Maybe > @kevinrushforth could advise? Hi everyone, This PR hasn't seen much activity in a while, so I though I would give it a gentle kick to hopefully get it moving again ;) As explained above, I feel a little stalled at the moment, as we need to decide whether or not it is a good idea to try and address all or part of JDK-8189082 within the scope of this PR, and I don't feel like I can settle that on my own. Thanks. - PR: https://git.openjdk.java.net/jfx/pull/112
Re: [Rev 01] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
On Wed, 15 Apr 2020 10:02:47 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: added FIXME with reference to issue
The change looks good to me, however I have minor concern about selecting
Separator, which should be taken in a follow
on issue. It may need a change in `Separator` selection test here.
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.
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.
-
PR: https://git.openjdk.java.net/jfx/pull/177
Re: [Rev 02] RFR: 8223298: SVG patterns are drawn wrong
> Issue: Assuming the pixelScale is 2, the tile image size is doubled at the > native side which is propagated to the java > side as well. But, as transform initialization takes place after scaling, the > transform is reset to default value. > Fix: Override scale() method in WCBufferedContext class to call init() before > scaling. Arun Joseph has updated the pull request incrementally with one additional commit since the last revision: Added SVGTest - Changes: - all: https://git.openjdk.java.net/jfx/pull/164/files - new: https://git.openjdk.java.net/jfx/pull/164/files/fb1c3934..86674653 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/164/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/164/webrev.01-02 Stats: 132 lines in 1 file changed: 132 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/164.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/164/head:pull/164 PR: https://git.openjdk.java.net/jfx/pull/164
Re: [Rev 01] RFR: 8223298: SVG patterns are drawn wrong
On Fri, 10 Apr 2020 16:29:57 GMT, Guru Hb wrote: >> Arun Joseph has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update copyright year > > Looks good to me. Tested on Windows and mac OS X. > If feasible can we have Unit test for this. As discussed offline, please add a test for this. - PR: https://git.openjdk.java.net/jfx/pull/164
Re: No tags for releases at github openjdk/jfx?
You can find all tags for each release at https://github.com/openjdk/jfx/tags For OpenJFX versions 11, 12 and 13, you can find them in the previous repository: https://github.com/javafxports/openjdk-jfx/tags Greetings, Joeri On Wed, Apr 15, 2020 at 1:18 PM Florian Kirmaier wrote: > Hi everyone, > > I can't see any tags in the GitHub project for the various releases of > JavaFX. > I would like to know which commit is the base for various releases. > Specifically, I'm interested in the version 14 and 15-ea3. > Is there a place where I can look it up? > > Thanks, > Florian Kirmaier > -- Joeri Sykora *Gluon*E: joeri.syk...@gluonhq.com
Re: [Rev 01] RFR: 8241737: TabPaneSkin memory leak on replacing selectionModel
On Wed, 15 Apr 2020 10:17:41 GMT, Ambarish Rapte wrote:
>> Looked again, and actually seeing two different issues ;)
>>
>> A) your test - that is with firing the pulse: fails for both not/removing
>> the listener
>> B) basically same test, but not firing the pulse - it fails without removing
>> and passes with removing the listener
>>
>> So I think we should include B as it is directly related to this fix (and
>> verifies the need to remove the listener). As
>> to A: we should keep it somewhere to not forget, but where?
>> Test code B:
>>
>> @Test
>> public void testNPEOnSwitchSkinAndSelect() {
>> TabPane testPane = new TabPane(new Tab("tab0"), new Tab("tab1"));
>> Group root = new Group(testPane);
>> Scene scene = new Scene(root);
>> Stage stage = new Stage();
>> stage.setScene(scene);
>> stage.show();
>>
>> testPane.setSkin(new TabPaneSkin1(testPane));
>> testPane.getSelectionModel().select(1);
>> }
>>
>>
>> The exception (seen on the test stack when rewiring the uncaughthandler as
>> usual, else on sysout):
>>
>> Exception in thread "main" java.lang.NullPointerException
>> at
>> javafx.controls/javafx.scene.control.skin.TabPaneSkin.lambda$0(TabPaneSkin.java:447)
>> at
>> javafx.base/javafx.beans.WeakInvalidationListener.invalidated(WeakInvalidationListener.java:83)
>> at
>> javafx.base/com.sun.javafx.binding.ExpressionHelper$Generic.fireValueChangedEvent(ExpressionHelper.java:348)
>> at
>> javafx.base/com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:80)
>> at
>> javafx.base/javafx.beans.property.ReadOnlyObjectPropertyBase.fireValueChangedEvent(ReadOnlyObjectPropertyBase.java:74)
>> at
>> javafx.base/javafx.beans.property.ReadOnlyObjectWrapper.fireValueChangedEvent(ReadOnlyObjectWrapper.java:102)
>> at
>> javafx.base/javafx.beans.property.ObjectPropertyBase.markInvalid(ObjectPropertyBase.java:113)
>> at
>> javafx.base/javafx.beans.property.ObjectPropertyBase.set(ObjectPropertyBase.java:147)
>> at
>> javafx.controls/javafx.scene.control.SelectionModel.setSelectedItem(SelectionModel.java:105)
>> at
>> javafx.controls/javafx.scene.control.TabPane$TabPaneSelectionModel.select(TabPane.java:736)
>> at
>> javafx.controls/test.javafx.scene.control.SelectionFocusModelMemoryTest.testNPEOnSwitchSkinAndRemoveTabPaneFirePulse(SelectionFocusModelMemoryTest.java:261)
>
>> B) basically same test, but not firing the pulse - it fails without removing
>> and passes with removing the listener
>
> Seems like this test randomly crashes (not always) any other test from
> `TabPaneTest`. It might be crashing the test
> executed just after this one OR the next test which does `tk.firePulse()`.
> Also not including `tk.firePulse()` would
> make the test incomplete/ unreliable.
>> Personally, I would tend to keep and ignore that test with doc:
>
> That is better to include the test now, else very doubtful of adding it in
> future. Including the test in next commit
> with @Ignore("JDK-8242621").
can't reproduce the random crashing (but then, it's random :) And don't quite
understand why you think the test being
incomplete/unreliable without a firePulse (there are tons of tests without) -
as long as we don't want to test the
result of a layout pass, we should be fine.
anyway, that's a different story, to me your fix and tests look fine
-
PR: https://git.openjdk.java.net/jfx/pull/175
Re: [Rev 01] RFR: 8241737: TabPaneSkin memory leak on replacing selectionModel
On Wed, 15 Apr 2020 10:52:26 GMT, Ambarish Rapte wrote: >> `TabPaneSkin` adds a listener to `SelectionModel.selectedItemProperty()` >> which holds the `SelectionModel` from being >> GCed. Fix is to add and remove the listener when a `SelectionModel` is >> changed. >> If the fix looks good, We can change all the >> `getSkinnable().getSelectionModel()` calls to use the new class member >> `selectionModel`. >> The fix seems safe to cause any regression. Enabled an ignored test that was >> added as part of fix for >> [JDK-8241455](https://bugs.openjdk.java.net/browse/JDK-8241455). > > Ambarish Rapte has updated the pull request incrementally with one additional > commit since the last revision: > > Add tests Marked as reviewed by fastegal (Author). - PR: https://git.openjdk.java.net/jfx/pull/175
No tags for releases at github openjdk/jfx?
Hi everyone, I can't see any tags in the GitHub project for the various releases of JavaFX. I would like to know which commit is the base for various releases. Specifically, I'm interested in the version 14 and 15-ea3. Is there a place where I can look it up? Thanks, Florian Kirmaier
Re: [Rev 01] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
On Wed, 15 Apr 2020 10:02:47 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: added FIXME with reference to issue Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/177
Re: [Rev 01] RFR: 8241737: TabPaneSkin memory leak on replacing selectionModel
> `TabPaneSkin` adds a listener to `SelectionModel.selectedItemProperty()` > which holds the `SelectionModel` from being > GCed. Fix is to add and remove the listener when a `SelectionModel` is > changed. > If the fix looks good, We can change all the > `getSkinnable().getSelectionModel()` calls to use the new class member > `selectionModel`. > The fix seems safe to cause any regression. Enabled an ignored test that was > added as part of fix for > [JDK-8241455](https://bugs.openjdk.java.net/browse/JDK-8241455). Ambarish Rapte has updated the pull request incrementally with one additional commit since the last revision: Add tests - Changes: - all: https://git.openjdk.java.net/jfx/pull/175/files - new: https://git.openjdk.java.net/jfx/pull/175/files/c3e2cbf9..e4293c75 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/175/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/175/webrev.00-01 Stats: 44 lines in 1 file changed: 39 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/175.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/175/head:pull/175 PR: https://git.openjdk.java.net/jfx/pull/175
Re: RFR: 8241737: TabPaneSkin memory leak on replacing selectionModel
On Tue, 14 Apr 2020 13:26:51 GMT, Jeanette Winzenburg
wrote:
> B) basically same test, but not firing the pulse - it fails without removing
> and passes with removing the listener
Seems like this test randomly crashes (not always) any other test from
`TabPaneTest`. It might be crashing the test
executed just after this one OR the next test which does `tk.firePulse()`. Also
not including `tk.firePulse()` would
make the test incomplete/ unreliable.
> Personally, I would tend to keep and ignore that test with doc:
That is better to include the test now, else very doubtful of adding it in
future. Including the test in next commit
with @Ignore("JDK-8242621").
-
PR: https://git.openjdk.java.net/jfx/pull/175
Re: [Rev 01] RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
On Tue, 14 Apr 2020 14:09:13 GMT, Ajit Ghaisas 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/ChoiceBox.java
> line 185:
>
>> 184:
>> sm.selectedItemProperty().addListener(selectedItemListener);
>> 185: // unfixed part of JDK-8090015 - why exclude null?
>> 186: if (sm.getSelectedItem() != null && !
>> valueProperty().isBound()) {
>
> Add a TODO: or FIXME: if you intend to work on it. Also, it will be better to
> create a JBS issue.
>
> If not - please remove the comment.
Actually forgot that comment, already filed and took the issue JDK-8242001
thanks, eagle eye :)
-
PR: https://git.openjdk.java.net/jfx/pull/177
Re: [Rev 01] 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: added FIXME with reference to issue - Changes: - all: https://git.openjdk.java.net/jfx/pull/177/files - new: https://git.openjdk.java.net/jfx/pull/177/files/017be222..0d3ce123 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/177/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/177/webrev.00-01 Stats: 1 line in 1 file changed: 0 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: RFR: 8242489: ChoiceBox: initially toggle not sync'ed to selection
On Mon, 13 Apr 2020 10:36:51 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.
Code changes and test look OK to me.
I have a minor comment that I have listed separately.
modules/javafx.controls/src/main/java/javafx/scene/control/ChoiceBox.java line
185:
> 184:
> sm.selectedItemProperty().addListener(selectedItemListener);
> 185: // unfixed part of JDK-8090015 - why exclude null?
> 186: if (sm.getSelectedItem() != null && !
> valueProperty().isBound()) {
Add a TODO: or FIXME: if you intend to work on it. Also, it will be better to
create a JBS issue.
If not - please remove the comment.
-
PR: https://git.openjdk.java.net/jfx/pull/177
Re: Proposed IntegerSpinner buggy behavior correction - JDK-8242553
Hi Ajit, yes, I read the doc, probably a bit differently - could well be my misunderstanding and misunderstandable wording :) Trying again: - I read your suggestion (in https://bugs.openjdk.java.net/browse/JDK-8242553) to imply f.i. that being at value and incrementing a full-cycle (that is max -min +1), I will land on value again - for me the doc seemed to imply that in such a case I would land on min. Though, given the "circular" as you pointed out correctly, was my misunderstanding - the current implementation is buggy (https://bugs.openjdk.java.net/browse/JDK-8193286) in calculating the remainder (which is what the first bullet amounts to) incorrectly for min != 0 Where do I err? -- Jeanette Zitat von Ajit Ghaisas : Hi Jeanette, The doc never assumes amountPerStep = 1. I am quoting it here - “The wrapAround property is used to specify whether the value factory should be circular. For example, should an integer-based value model increment from the maximum value back to the minimum value (and vice versa).” The word “circular” clarifies that once we exceed maximum value, we should start from minimum. I think, the doc is OK in it’s current form, but implementation needs to be corrected. Regards, Ajit On 14-Apr-2020, at 8:01 PM, Jeanette Winzenburg wrote: Hi Ajit, thought the doc was simply bad (in specifying the behavior for amountPerStep = 1 and not thinking of larger amounts) - my expection is a calculated wrap, that is the target as you suggest via modulo the difference from current value. Don't know if anybody took the doc literally .. -- Jeanette Zitat von Ajit Ghaisas : Hi, Once I fix JDK-8193286, I would like to take up JDK-8242553 (IntegerSpinner does not wrap around values correctly if amountToStepBy is larger than total numbers between Max and Min) The current implementation is not as per what is documented. Refer : https://openjfx.io/javadoc/14/javafx.controls/javafx/scene/control/SpinnerValueFactory.html#wrapAroundProperty I propose to fix the current buggy behavior of IntegerSpinner. Although it is a corner case, I would like to know if anybody relies on this buggy behavior? Regards, Ajit
Re: [Rev 03] RFR: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.
> Extract keystate and add to the existing modifier mask, to support eg > multi-select > > https://bugs.openjdk.java.net/browse/JDK-8202296 Tom Schindl has updated the pull request incrementally with one additional commit since the last revision: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events. Fix whitespace errors - Changes: - all: https://git.openjdk.java.net/jfx/pull/170/files - new: https://git.openjdk.java.net/jfx/pull/170/files/64a802ca..8fe96a90 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/170/webrev.03 - incr: https://webrevs.openjdk.java.net/jfx/170/webrev.02-03 Stats: 11 lines in 1 file changed: 0 ins; 0 del; 11 mod Patch: https://git.openjdk.java.net/jfx/pull/170.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/170/head:pull/170 PR: https://git.openjdk.java.net/jfx/pull/170
Re: [Rev 02] RFR: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.
> Extract keystate and add to the existing modifier mask, to support eg > multi-select > > https://bugs.openjdk.java.net/browse/JDK-8202296 Tom Schindl has updated the pull request incrementally with two additional commits since the last revision: - 8202296: Monocle MouseInput doesn't send keyboard modifiers in events. Fix whitespace errors - 8202296: Monocle MouseInput doesn't send keyboard modifiers in events. Fix whitespace errors - Changes: - all: https://git.openjdk.java.net/jfx/pull/170/files - new: https://git.openjdk.java.net/jfx/pull/170/files/2ca1c88a..64a802ca Webrevs: - full: https://webrevs.openjdk.java.net/jfx/170/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/170/webrev.01-02 Stats: 9 lines in 1 file changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jfx/pull/170.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/170/head:pull/170 PR: https://git.openjdk.java.net/jfx/pull/170
Re: [Rev 01] RFR: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events.
> Extract keystate and add to the existing modifier mask, to support eg > multi-select > > https://bugs.openjdk.java.net/browse/JDK-8202296 Tom Schindl has updated the pull request incrementally with one additional commit since the last revision: 8202296: Monocle MouseInput doesn't send keyboard modifiers in events. Added Test for keyboard modifiers - Changes: - all: https://git.openjdk.java.net/jfx/pull/170/files - new: https://git.openjdk.java.net/jfx/pull/170/files/79fe0fd7..2ca1c88a Webrevs: - full: https://webrevs.openjdk.java.net/jfx/170/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/170/webrev.00-01 Stats: 54 lines in 1 file changed: 54 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/170.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/170/head:pull/170 PR: https://git.openjdk.java.net/jfx/pull/170
Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView
On Tue, 14 Apr 2020 12:20:10 GMT, John Hendrikx wrote: >> In a minimal test I wrote (not a microbenchmark that removes listeners), I >> tried this PR code, but did not reproduce >> the performance improvement. I have attached a test program in my PR(#125) > > @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. - PR: https://git.openjdk.java.net/jfx/pull/108
