Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scaling area scales VirtualFlow's content AND scrollBars #205

Closed
JordanMartinez opened this issue Nov 5, 2015 · 21 comments
Closed

Scaling area scales VirtualFlow's content AND scrollBars #205

JordanMartinez opened this issue Nov 5, 2015 · 21 comments

Comments

@JordanMartinez
Copy link
Contributor

Specifically, I need more direct control to VirtualFlow in the following ways:

  • get/set how tall the horizontal scrollbar is in the VirtualFlow
  • scale the VirtualFlowContent without the horizontal scrollbar changing its height as it does now.

Not sure if it'd be better to remove the skin or just change the way that scaling is handled within RichTextFX. However, since it's down that alley, I thought I'd bring it up. You've said before that removing the skin would be tedious. However, it didn't sound hard. What would I need to do in order to remove it?

@TomasMikula
Copy link
Member

Hi, I will reply in more length when at a computer, but I would go in the
direction of VirtualFlow optionally without scrollbars, so that one can add
whatever scrollbars they please.
On Nov 5, 2015 4:57 PM, "JordanMartinez" notifications@github.com wrote:

Specifically, I need more direct control to VirtualFlow in the following
ways:

  • get/set how tall the horizontal scrollbar is in the VirtualFlow
  • scale the VirtualFlowContent without the horizontal scrollbar
    changing its height as it does now.

Not sure if it'd be better to remove the skin or just change the way that
scaling is handled within RichTextFX. However, since it's down that alley,
I thought I'd bring it up. You've said before that removing the skin would
be tedious. However, it didn't sound hard. What would I need to do in order
to remove it?


Reply to this email directly or view it on GitHub
#205.

@JordanMartinez
Copy link
Contributor Author

That's a smart idea that would actually fix a different issue in my program relating to this! I'll wait for your longer reply.

@TomasMikula
Copy link
Member

So I prefer breaking things down into composable pieces. Hard-wiring auto-scrollbars into Flowless's VirtualFlow didn't feel quite right from the start. On the other hand, the internal architecture of VirtualFlow already very much separates the scrollbar concern: VirtualFlow basically just adds scrollbars onto VirtualFlowContent. Let's take the discussion how to best expose scrollbar-free VirtualFlow over to the Flowless project.

Yes, removing the skin shouldn't be too hard, although laborious, and would in the end simplify a bunch of other things. Let's also track this as a separate issue.

Once the above two things are done, the only remaining point will be how to create scrollbar-free StyledTextAreas.

One option is to decide at construction time and instantiate the correct VirtualFlow. This way would keep things simple in StyledTextArea code, but would double the number of constructors for each of the text areas - we would add a version of each constructor that takes an additional parameter saying whether we want scrollbars or not. A way around such combinatory explosion could be a builder pattern to simulate default parameter values.

Another way is to have a settable property that would allow to switch the presence of scrollbars after instantiation. This feels more JavaFX, but IMO would unnecessarily complicate the code, since maintaining the content position after replacing the VirtualFlow could be tricky.

@JordanMartinez
Copy link
Contributor Author

I see why a longer response was necessary ;-)

I've opened the other issues.
I like the settable property idea since, as you said, it feels more JavaFX. However, couldn't the VirtualFlow just be wrapped inside of a BorderPane and each of its scrollBars put in either the bottom/top or right/left position of the BorderPane ?

@TomasMikula
Copy link
Member

That might work for always visible scrollbars. For auto-scrollbars, you get into some issues with "dynamical layout", i.e. when only in the layout process you decide to add or remove children. Only after you find out the width of the content can you decide what scrollbars you need, if any. I figured I needed a hand-rolled solution in VirtualFlow, but feel free to experiment with BorderPane.

Anyway, for the scrollbar-free version of VirtualFlow I don't imagine scrollbars almost being there, just turned off, but really nothing scrollbar specific being there.

As a side note, I wish JavaFX controls were this way. Look e.g. at ListCell, which IMO is a really bad example of modular design. If you want to put something simple into a ListView, you need a ListCell anyway. ListCell drags in with it a graphic and a text, although a lot of times you only want one of them. font, lineSpacing, textAlignment, textFill... don't make sense if you only use graphic, but they are still there. ListCell is a Control, so it also has all of skin infrastructure. It also has editing-specific API (startEdit, commitEdit, ...), although most of the time one does not care about editing. It retains a reference back to the ListView, so it is all entangled and really anti-modular. This is not where I want to get with Flowless. So in scrollbar-free version, there really should not be any infrastructure specific to scrollbars.

@JordanMartinez
Copy link
Contributor Author

Ok. Am I correct in understanding you to be saying:

  • VirtualFlow should not get anti-modular like ListCell, which has back references to various components.
  • Therefore, VirtualFlow should have two versions: one that has scroll bars, which appear when needed, and another without scroll bars at all.
  • Being dependent upon VirtualFlow, there are a few ways that StyledTextArea could deal with this scroll bar issue:
    • Idea 1: The version of VirtualFlow that either has or does not have scroll bars is decided at construction time: There will be two types of StyledTextAreas: one that uses the version of VirtualFlow that has scrollbars (thus, it will display scroll bars when needed), and another one that uses the version without scroll bars (thus, it can never display scroll bars because they don't exist in the object).
    • Idea 2: Scroll bar-free version of VirtualFlow is used. Scroll bar infrastructure and whether to display scroll bars is handled somehow within RichTextFX: There is only one type of StyledTextArea, albeit now it has an additional property. When said property is "turned on," StyledTextArea will create scroll bars and figure out whether said scroll bars should be displayed.

Pros & Cons of Idea 1

Pros Cons Workarounds
code stays simple (easy/fast to implement) doubles # of constructors builder pattern could be used to make more convenient
inflexible

Pros & Cons of Idea 2

Pros Cons
feels like JavaFX unnecessarily complicates the code (could be tricky to maintain the content position after replacing the VirtualFlow
flexible

Besides that, there are a few things you said that I didn't understand. You wrote:

maintaining the content position after replacing the VirtualFlow could be tricky

What do you mean by "content position? Are you referring to how far down the document that is displayed via VirtualFlow is scrolled? Also, why and when would the VirtualFlow be replaced?

@TomasMikula
Copy link
Member

Therefore, VirtualFlow should have two versions: one that has scroll bars, which appear when needed, and another without scroll bars at all.

Yes. The one with scrollbars would be a wrapper around the one without scrollbars.

Idea 2: Scroll bar-free version of VirtualFlow is used. Scroll bar infrastructure and whether to display scroll bars is handled somehow within RichTextFX: There is only one type of StyledTextArea, albeit now it has an additional property. When said property is "turned on," StyledTextArea will create scroll bars and figure out whether said scroll bars should be displayed.

This is an option, too. I meant something else: when the property is turned on, the scrollbar-less VirtualFlow would be replaced by a scrollbar-ful VirtualFlow. This also answers your question about when would VirtualFlow be replaced.

To elaborate more on your idea

StyledTextArea will create scroll bars and figure out whether said scroll bars should be displayed.

We would probably like to reuse the same code that VirtualFlow with scrollbars already uses

@JordanMartinez
Copy link
Contributor Author

Ok, that clarifies a lot!

So the two ideas are better summarized as:

  1. The version of VirtualFlow that either has or does not have scroll bars is decided at StyledTextArea's construction time.
    1. If the no-scrollbars option is chosen, scroll bars could be "added" on top of StyledTextArea by the developer in a similar way that they are already added in VirtualFlow (version: 0.4.6) right now.
  2. Scroll bar-free version of VirtualFlow is used. Scroll bar infrastructure and whether to display scroll bars is handled somehow within RichTextFX. One way to implement this approach:
    1. The version of VirtualFlow used to display control is based on the value of a property. When property is "turned on", scroll-bars-included VirtualFlow is used. When Property is "turned off," scroll-bar-free VirtualFlow is used. Default value is "on." When a change is registered, the current skin is replaced with the opposite one.

In regards to the second option, what would happen if the program, for whatever reason, called getEstimatedScrollX() when the scroll-bar-free VirtualFlow is the current skin? StyledTextArea would need to have that property in order to support the scroll-bars-included VirtuaFlow skin, but it wouldn't actually return a value, much less if something attempted to bind to its estimatedScrollXProperty().

What if there was an additional wrapper around StyledTextArea that uses the first approach but also includes scroll bars? Then, transformations to the content could be readily applied without losing the display-when-needed scroll bars.

@TomasMikula
Copy link
Member

You can scroll the content even if there are no scrollbars. Scrollbar-free VirtualFlow will have those methods.

I presented 2. as an option, but not that I really like it.

I'm now starting to think that to achieve the best modularity, StyledTextArea should never include scrollbars, but it should be embeddable in a VirtualizedScrollPane that I proposed in FXMisc/Flowless#9

@JordanMartinez
Copy link
Contributor Author

True. I just think that scroll bars appearing better fits the end-user's expectations. Seeing an area that scrolls but doesn't have scroll bars feels a bit weird IMO.

I'm also thinking Option 2 isn't the best and that Option 1 would be.

@JordanMartinez
Copy link
Contributor Author

Embedding it would make more sense.

@TomasMikula
Copy link
Member

We might provide factory methods that return VirtualizedScrollPane<StyledTextArea<S, PS>> as a convenient way to get a text area wrapped in scrollbars.

@JordanMartinez
Copy link
Contributor Author

I love convenience! Definitely!

@JordanMartinez
Copy link
Contributor Author

Tasks remaining:

  • VirtualFlow has no scroll bars by default; to get them, embed it in a VirtualizedScrollPane
  • Remove the skin
  • add convenient static method to wrap a text area in a VirtualizedScrollPane

@TomasMikula
Copy link
Member

Do you have all you need to achieve this now?

@JordanMartinez
Copy link
Contributor Author

I believe so. Now I can scale the VirtualFlow without the scrollbars getting in the way. At the same time, I can figure out how tall the scroll bar's height is by

virtualizedScrollPane.getHeight() - vspane.getViewportHeight();

@JordanMartinez
Copy link
Contributor Author

Looks like VirtualizedScrollPane's layoutChildren doesn't take transformations into account..
With the code (using Groovy, but it's still clear enough):

 // text content (shown below) forces both scroll bars to appear.
String text = "...";
VirtualizedScrollPane<InlineCssTextArea> vsPane = AreaFactory
    .embeddedInlineCssTextArea(text);
Scene scene = new Scene(new Pane(vsPane), 1200, 600);
InlineCssTextArea area = vsPane.getContent();
vsPane.setMinWidth(600);
vsPane.setMinHeight(400);
vsPane.addEventFilter(ScrollEvent.ANY, {
    area.scaleY *= 1.5
});

I get the following picture :-D

gfx_008

@JordanMartinez
Copy link
Contributor Author

So, the workaround I came up with was to copy VirtualizedScrollPane exactly and add some code in its layoutChildren() that would display the area correctly. I had to copy the file exactly because hbar and vbar are private, not protected. Otherwise, I could have just extended the object and made these minor modifications.

@TomasMikula
Copy link
Member

Not sure if this is not the expected behavior of scale. Maybe VirtualizedScrollPane should have a clip.

@JordanMartinez
Copy link
Contributor Author

Adding a clip (as seen below) did fix it. My version doesn't always work correctly, but I'm sure it could be better implemented.

private final Rectangle clipRect = new Rectangle();

// in the constructor, add
setClip(clipRect);

// within layoutChildren(), rewrite

content.resize(w, h);
clipRect.width = layoutWidth;
clipRect.height = layoutHeight;

zooming in (scaling up) made the lower right corner between scrollbars (as both are visible in my example) White.
zooming out (scaling down) left a Gray background that should otherwise be White in VirtualizedScrollPane

@JordanMartinez
Copy link
Contributor Author

Taken from FXMisc/Flowless#14, to scale a StyledTextArea within its VirtualizedScrollPane, one needs to create a wrapper class that handles such scaling. The final result would then look like:

StyledTextArea<?, ?> area = // creation code
ScaledVirtualized wrapper = new ScaledVirtualized(area);
VirtualizedScrollPane<ScaledVirtualized> vsPane = new VirtualizedScrollPane(wrapper);

The following class (written in Groovy) is an example of such a wrapper:

class ScaledVirtualized<V extends Node & Virtualized> extends Region implements Virtualized {
    final V content
    Scale scale = new Scale(1, 1, 0, 0)
    Val<Double> estHeight
    Val<Double> estWidth
    Var<Double> estScrollY
    Var<Double> estScrollX

    ScaledVirtualized(V content) {
        super()
        this.content = content
        getChildren().add(content)
        getTransforms().add(scale)
        estHeight = Val.combine(
                content.totalHeightEstimateProperty(),
                scale.yProperty(),
                { estHeight, scaleFactor -> estHeight * scaleFactor }
        )
        estWidth = Val.combine(
                content.totalWidthEstimateProperty(),
                scale.xProperty(),
                { estWidth, scaleFactor -> estWidth * scaleFactor }
        )
        estScrollX = Var.mapBidirectional(
                content.estimatedScrollXProperty(),
                (Function) { double scrollX -> scrollX * scale.x },
                (Function) { double scrollX -> scrollX / scale.x }
        )
        estScrollY = Var.mapBidirectional(
                content.estimatedScrollYProperty(),
                (Function) { double scrollY -> scrollY * scale.y },
                (Function) { double scrollY -> scrollY / scale.y }
        )
        addEventFilter(ScrollEvent.SCROLL, { ScrollEvent event ->
            if (event.controlDown) {
                scale.with {
                    double factor = 0.9
                    if (event.deltaY > 0) {
                        y *= factor
                        x *= factor
                    } else {
                        y /= factor
                        x /= factor
                    }
                }
                layoutChildren()
                event.consume()
            }
        })
    }

    @Override
    protected void layoutChildren() {
        double width = layoutBounds.width
        double height = layoutBounds.height
        content.resize(width / scale.x, height/ scale.y)
    }
    @Override
    Var<Double> estimatedScrollXProperty() {
        estScrollX
    }
    @Override
    Var<Double> estimatedScrollYProperty() {
        estScrollY
    }
    @Override
    Val<Double> totalHeightEstimateProperty() {
        estHeight
    }
    @Override
    Val<Double> totalWidthEstimateProperty() {
        estWidth
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants