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

Make VirtualFlow's scrollBars optional #9

Closed
JordanMartinez opened this issue Nov 6, 2015 · 11 comments
Closed

Make VirtualFlow's scrollBars optional #9

JordanMartinez opened this issue Nov 6, 2015 · 11 comments

Comments

@JordanMartinez
Copy link
Contributor

Coming from FXMisc/RichTextFX#205, allow the option of not showing the horizontal and/or the vertical scrollBars.

Tomas already said in the above issue:

Hard-wiring auto-scrollbars into 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

@TomasMikula
Copy link
Member

Here's a proposal: define

interface Virtualized {
    Val<Double> totalWidthEstimateProperty();
    Val<Double> totalHeightEstimateProperty();
    Var<Double> estimatedScrollXProperty();
    Var<Double> estimatedScrollYProperty();
}

that represents a virtualized component. Make VirtualFlowContent implement that interface. Implement

class VirtualizedScrollPane<T extends Virtualized> extends Virtualized {

    VirtualizedScrollPane(T content) { /* ... */ }

    public T getContent() { /* ... */ }

    // ...
}

VirtualizedScrollPane will basically be a slimmed down version of current VirtualFlow. It just adds scrollbars to any Virtualized content.

I guess we can then rename VirtualFlowContent to VirtualFlow. We will then have factory methods that return VirtualizedScrollPane<VirtualFlow> (~VirtualFlow with scrollbars) and also factory methods that return bare VirtualFlow (without scrollbars).

@JordanMartinez
Copy link
Contributor Author

If I'm understanding you correctly:

Old Name New Name Changes (old names used)
VirtuallFlowContent VirtualFlow VirtualFlowContent without scroll bars
VirtualFlow VirtualizedScrollPane VirtualFlowContent with scroll bars

@TomasMikula
Copy link
Member

The current VirtualFlowContent already is without scrollbars.

VirtualizedScrollPane is meant to be a general component that adds scrollbars to Virtualized content. It shouldn't have anything specific to VirtualFlow(Content). I would operate on just the Virtualized interface.

@JordanMartinez
Copy link
Contributor Author

Shouldn't VirtualizedScrollPane's generic be <V extends VirtualFlow>? Otherwise, won't the code getChildren().addAll(content, hbar, vbar); throw a compiler error because Virtualized doesn't extend Node?

@TomasMikula
Copy link
Member

You are right, but then it should be <V extends Node & Virtualized>, so that we don't restrict it only to VirtualFlow (so that we can also embed a scrollbar-free StyledTextArea in a VirtualizedScrollPane after we make StyledTextArea extend Virtualized).

@JordanMartinez
Copy link
Contributor Author

Ok. That clarifies things.
Just for clarification, should Virtualized include things other than the scrolling API? For example:

public interface Virtualized<T, C extends Cell<T, ?>> {
    // scrolling API
    Val<Double> totalWidthEstimateProperty();
    Val<Double> totalHeightEstimateProperty();
    Var<Double> estimatedScrollXProperty();
    Var<Double> estimatedScrollYProperty();
    void setEstimatedScrollX(double value);
    void setEstimatedScrollY(double value);
    double getEstimatedScrollX();
    double getEstimatedScrollY();
    void scrollXToPixel(double pixel);
    void scrollYToPixel(double pixel);
    // and other scrolling-related API....

    // and API for other stuff that was in VirtualFlowContent and 
    //    proxied via VirtualFlow (old names respectively)
    C getCellFor(int itemIndex);
    Optional<C> getCellIfVisible(int itemIndex);
    Orientation getContentBias();
    VirtualFlowHit<C> hit(double x, double y);
    void show(double viewportOffset);
    // and so on...
}

@TomasMikula
Copy link
Member

No, the other API should not be there. Virtualized content doesn't have to be a flow of cells, as happens to be the case for VirtualFlow.

@JordanMartinez
Copy link
Contributor Author

Ok. Thanks for the clarification. So one would use something like the following code to do stuff:

VirtualizedScrollPane vsPane = new VirtualizedScrollPane(VirtualFlow.createHorizontal(args));
vsPane.getContent().setEstimatedScrollX(40);
vsPane.getContent().show(30);
// or more likely...
VirtualFlow content = (VirtualFlow) vsPane.getContent()
content.cellToViewport(cell, bounds)
// etc....

@TomasMikula
Copy link
Member

Yes. I don't like type casting, though. getContent() should return the type parameter V, which in this case will be a VirtualFlow.

@JordanMartinez
Copy link
Contributor Author

Right. I was thinking in terms of generics, not an instance of a class that uses generics, which would thus know what class its generic value V represents.

@JordanMartinez
Copy link
Contributor Author

Since this was fixed in 842feb6, I'm closing this issue.

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