Skip to content
New issue

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

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

Already on GitHub? # 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.

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

No branches or pull requests

2 participants