Bug ID: JDK-8166565 [hidpi] a flickering scrollbar when using Caspian style sheet

JDK 9
9Fixed

http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/dade58d9d723
29-09-2016
+1
29-09-2016
Here is a token webrev with the updated javadoc comments. I left the existing documentation on the deprecated methods the same as the previous webrev as I don't want them to appear to be a convenience for anything given that they are deprecated... http://cr.openjdk.java.net/~flar/JDK-8166565/webrev.02/
29-09-2016
Sounds good to me, too.
29-09-2016
I think that sounds good, +1.
29-09-2016
So I had a different idea about the documentation over coffee this morning. These methods are now just convenience methods in lieu of sprinkling getSkinnable().snapFooXY() through the code. Perhaps I should document them as: * This is a convenience method to access the snapFooXY() method on the skinnable. * This method is equivalent to {@code getSkinnable().snapFooXY()}. ... * @see Region.snapFooXY Thoughts?
29-09-2016
+1
29-09-2016
This new webrev should have all of the suggested changes: http://cr.openjdk.java.net/~flar/JDK-8166565/webrev.01/ - added @Deprecated annotation to new methods (will file bug to add @since) - Renamed target of snapping in the new SkinBase methods (and removed extra "/**") - Fixed snapping of header in TabPaneSkin.layoutChildren (and removed a dead variable) (looked through the logic there and think it is working as intended now) - Removed comment and unnecessary extra snap() call in TitledPaneSkin.layoutChildren
29-09-2016
Jim, your suggested changes to TitledPaneSkin and TabPaneSkin sound good to me. I don't have a strong opinion on fixing the SkinBase documentation (i.e. it can stay as-is if desired), however, if you do want to change it, I would probably just say 'skinnable' rather than 'skinnable control', as this is redundant. It's kind of an odd naming nomenclature, but the term skinnable in this case means the control. I'm also fine with just saying "If the control's snapToPixel..."
29-09-2016
+1
29-09-2016
Regarding the @Deprecated , correct. What I did in a similar case is added the (since = "9") in a FIXME comment: @Deprecated // FIXME: (since = "9") but it's up to you whether you want to add that comment. Either way, please file a follow-on issue to find methods newly deprecated in 9 and add the needed (since="9") as there are others (e.g., the ones in region). I'll let you and Jonathan sort out X versus Y (and I don't have an opinion on whether it is worth changing the SkinBase docs to avoid mentioning Region, given that it isn't a new issue).
28-09-2016
To summarize: - Add @Deprecated, but not @Deprecated-@since? - (Ignore previous comment here, I see that Region's methods are already marked @Deprecated) - The existing deprecated methods already referred to region. I just noticed that after the push as well, but it isn't new - they probably cut and pasted the comments from Region just as I did (and since it didn't show up as a before/after difference I didn't think much about it. Since there is a getSkinnable() then I think "The skinnable's snapToPixel" is good, what about "The skinnable control's snapToPixel"? I think what I might do about TabPaneSkin, when Jonathan gets back to me after verifying, is to move the 3 lines inside the cascading if statements below it since the intervening lines don't affect what is done with the headerHeight and the tabStartXY and different things happen with H vs. V and leading edge (left/top) vs. trailing edge (right/bottom). It will make it all clearer. With regard to X vs Y in the TabPaneSkin, the confusing part is the cases that use resize(h, w), but if what Jonathan says is true, then that simply represents the "sideways way of looking at the layout" and so this is a simply matter of just using the X vs. Y depending on LTRB side. I'll await a conclusion on that to post a new webrev...
28-09-2016
Comments: 1) In SkinBase, the javadoc refers to 'region' in a number of places. Technically the SkinBase is not a region and the operations are being performed on the control (which is a region). It might read better to say, for example, "If the skinnable's snapToPixel...", rather than "If this region's snapToPixel..."). 2) In SkinBase just after snapSpaceY, I see two adjacent lines with /** on them (for the comment for snapSize)? 3) Your two comments (in TabPaneSkin and TitledPaneSkin): TabPaneSkin: I will need to double check TabPaneSkin, but my recollection is that we basically use the 'headerHeight' to represent the amount of space available for the tabs. In the cases where the tabs are on the LEFT or RIGHT, then it should be considered a width (even though we're taking prefHeight) and then I would probably use snapSizeX. I believe...although don't quote me on this TitledPaneSkin: You're correct here, it looks redundant and should be removed.
28-09-2016
Raising to P3 since this is indicative of other issues and not limited to this one demo.
28-09-2016
The fix itself looks good. I have two comments on the new/deprecated SkinBase methods. SkinBase.java 1. In addition to an ��@deprecated�� javadoc tag, a deprecated method should have an '@Deprecated' annotation. On a related note, once JDK-8161704 is fixed we can add the new 'since = "9"' parameter to the Deprecation annotation. Since this will fail to compile for now (see JDK-8157508), adding the @since = "9" will need to be a follow-on issue. 2. There is an extra start of javadoc comment: 526 / 527 / 528 * If this region's snapToPixel property is false, this method returns the
28-09-2016
Another interesting thing to note is that the top (major) tick mark on the slider is not visible without the fix. Running the demo with and without the fix on top of each other and switching back and forth shows a number of tiny layout differences mostly due to the fact that after the fix the controls can now layout down to the pixel resolution whereas before they were laying out only to the integer coordinate resolution which means they would only address every other pixel on retina screens...
28-09-2016
The fix is to do scaling-specific coordinate snapping in the controls package. To facilitate this solution 6 snap*() methods in Region (all new for JDK9) had to be made public (they used to be protected). An alternative could be to have the Control class override them with a protected delegate so that the methods can be called from anywhere in the controls package, but it seems that if these utility methods are going to be useful in more than one package, we should just make them public. webrev: http://cr.openjdk.java.net/~flar/JDK-8166565/webrev.00/ There will be a lot of cases of making sure I have the correct X() vs. Y() version of the snap method strewn throughout the controls package - please verify that I had no typos when I made the changes. Further note that there are 2 (I think, but don't quote me on that) cases where I wasn't sure if the code should have a snapFooX() or a snapFooY() because the values seemed to be used inconsistently after the call to snapFoo(), so I made the best call I could and then added a comment. Please provide feedback on whether I made the right call there.
28-09-2016
There seems to be a fight between the snap methods in control.SkinBase and the Snap methods in Region. The snap methods in the control package were not adjusted for the new HiDPI snapping used in the Region file. Since SkinBase inherits from Region, it could simply delegate to those new methods.
28-09-2016
Additional interesting data: - If you comment out the progress bar in the FXML file (i.e. it is no longer part of the layout, not simply making it non-visible), the tick marks and horizontal scrollbar continue to constantly repaint anyway, so the spinning progress indicator is not inducing the problem. - The repainting shows that the arrows and the rounded rect for the scrolling track are bumping up and down by slightly less than a pixel - The bottom of the rounded rect is bouncing more than the top so this appears to be a constant changing of the height of the rounded rect. I've also discovered a number of anomalies with the way that dirty regions and culling bits are managed, but since this issue is not affected by turning on and off dirty region optimizations, they can't be the cause of this issue (and I'm not 100% sure if they are actually bugs, I need to follow up with someone more familiar with the way we manage those mechanisms...)
28-09-2016
If I turn off Mac retina support with allowhidpi=false then the horizontal scrollbar is no longer considered dirty during the animation. The tick marks on the slider are still always marked dirty, though.
26-09-2016
This comes across like a dirty region optimization bug, but running with dirtyopts=false does not fix the flicker. Additionally, running with showdirty=true shows that there are 3 things that are dirty on every frame on that page: - the spinning progress indicator (obviously) - the horizontal scrollbar (no obvious reason why?) - the tick marks on the nearby vertical slider (no reason for this either, though it has no flickering)
26-09-2016
also reproducible on OS X 10.11 with Retina (JDK9 b136)
26-09-2016
I can reproduce this on my Linux system (Ubuntu 14.04) by forcing Hi-DPI with GDK_SCALE=2. It does not happen with non-Hi-DPI.
22-09-2016