On Tue, 15 Dec 2020 02:02:37 GMT, Kevin Rushforth <k...@openjdk.org> wrote:

>>> 
>>> 
>>> One more comment: given the quality problems that necessarily arise when 
>>> the translation of a cached image is not on an integer boundary, part of 
>>> the solution might be to snap the cached image to a pixel boundary as is 
>>> done in this PR, but we would need to ensure that this doesn't impact 
>>> smooth scrolling of a TextArea.
>> 
>> I've done some quick tests with the minimal sample below, and could not 
>> notice any performance impact with this patch compared to 15.0.1.
>> 
>>  java
>> public class TextScroll extends Application {
>> 
>>     @Override
>>     public void start(Stage stage) throws Exception {
>>         var textArea = new TextArea();
>>         var txt = new 
>> File(getClass().getResource("/Scrollpane.java").getFile());
>>         textArea.setText(Files.readString(txt.toPath(), 
>> StandardCharsets.UTF_8));
>>         var root = new ScrollPane(textArea);
>>         root.setFitToHeight(true);
>>         root.setFitToWidth(true);
>>         Scene scene;
>>         scene = new Scene(root, 800, 600);
>>         stage.setTitle("Scroll test");
>>         stage.setScene(scene);
>>         stage.show();
>>     }
>> 
>>     public static void main(String[] args) {
>>         launch(args);
>>     }
>> }
>
> Actually, when I mentioned smooth scrolling, it wasn't performance I was 
> thinking of, it was the ability to scroll by fractional pixel amounts, but 
> with the default snap-to-pixel enabled, that won't happen anyway.
> 
> I'll look into the other questions you raised tomorrow, specifically what we 
> might want to do about rendering the cache, and also your question about 
> snapToPixel.
> 
> As for the specific problem with ScrollPane, I found the cause of that one. 
> There is a (long-standing from the look of it) bug in 
> [`Region::updateSnappedInsets`](https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/java/javafx/scene/layout/Region.java#L990).
>  It attempts to account for snap-to-pixel, but does so incorrectly, using 
> `Math.ceil` without taking screen scaling into account (that is, without 
> calling the `snapXxxx` methods). The correct code should be something like:
> 
>             final boolean snap = isSnapToPixel();
>             snappedTopInset = snapSizeY(insets.getTop(), snap);
>             snappedRightInset = snapSizeX(insets.getRight(), snap);
>             snappedBottomInset = snapSizeY(insets.getBottom(), snap);
>             snappedLeftInset = snapSizeX(insets.getLeft(), snap);
> 
> This fixes the ScrollPane problem without needing to modify the cached 
> rendering code.

Thanks for that.

I am wondering; is it ok to potentially address both sub-issues discussed here 
(Scrollpane snaping vs cache rendering) under the same JBS bug? Or would it be 
better to address them under separate issues?

Speaking of which, the JBS issue title is in fact misleading, as the issue 
appears to be not specific to Windows; what's the best practice in such cases; 
rename the issue? Open a new one? Leave it as is?

-------------

PR: https://git.openjdk.java.net/jfx/pull/308

Reply via email to