search

FXMisc / Flowless

Efficient VirtualFlow for JavaFX
BSD 2-Clause "Simplified" License
185 stars 38 forks source link

Scroll bars now optional #11

Closed JordanMartinez closed 8 years ago

JordanMartinez commented 8 years ago

Ok. Let's try this again....

JordanMartinez commented 8 years ago

Since I asked about the VirtualizedScrollPane#dispose method in the previous problematic PR, I figured we should bring it here since most would look here not there.

I asked:

Should the VirtualizedScrollPane#dispose() method be included?

You replied:

Hmm, dispose() in the current VirtualFlow only calls content.dispose(), which stops observing the list of items. That kind of dispose doesn't make sense for VirtualizedScrollPane. However there is a number of bindings created that are not disposed—they don't need to be disposed, because currently VirtualFlow and VirtualFlowContent always come together. However, if we allow to remove the content from VirtualizedScrollPane, we should dispose those bindings. Preferably, however, they should be disposed automatically when content is removed.

My response: Ok. So, should content be held as a SimpleObjectProperty<V> content with a listener for its removal? Or should there be the method removeContent() that unbinds from the content and then returns V content ?

TomasMikula commented 8 years ago

I think the latter is simpler for us to do. There's a catch for both options, though: Node documentation says:

If a program adds a child node to a Parent (including Group, Region, etc) and that node is already a child of a different Parent or the root of a Scene, the node is automatically (and silently) removed from its former parent.

So the user will be able to remove the content from VirtualizedScrollPane even without calling removeContent()—by simply adding it to a different parent. We should be able to detect that as well and do the clean-up automatically.

JordanMartinez commented 8 years ago

True.... So, something like:

Subscription subscription = EventStreams.changesOf(content.parentProperty())
             .subscribe(change -> if (!change.newValue.equals(this)) dispose());

// where dispose is:
    private void dispose() {
        unbindScrollBar(hbar);
        unbindScrollBar(vbar);
        subscription.unsubscribe();
    }

    private void unbindScrollBar(ScrollBar bar) {
        bar.maxProperty().unbind();
        bar.unitIncrementProperty().unbind();
        bar.blockIncrementProperty().unbind();
        bar.valueProperty().unbind();
    }
TomasMikula commented 8 years ago

Mmm, who removes the content.parentProperty()'s listener then? I was thinking more like

ObjectBinding<V> content = (ObjectBinding<V>) Bindings.valueAt(getChildren(), 0);
content.addListener(changed -> dispose());

Here we don't need to call content.dispose() or content.removeListener(...), since we're only observing our own fields.

TomasMikula commented 8 years ago

Or just observe the getChildren() list for changes. Once populated (with content as the only child), the only possible change is content being removed, so when a change occurs, dispose.

TomasMikula commented 8 years ago

removeContent would then be implemented as

getChildren().clear();
return content;
JordanMartinez commented 8 years ago

What would the code for listening to the changes in children's size be? I understand that we want to avoid memory leaks by removing the listener once finished. However, ListChangeListener has always felt weird to me. Would you be against me using ReactFX's EventStreams.sizesOf(getChildren()).subscribe( () -> dispose()); and then calling the resulting Subscription's unsubscribe() method in the dispose() method? Or is there a better way to do this?

JordanMartinez commented 8 years ago

Do these need to be unbound as well?

TomasMikula commented 8 years ago

I agree they are weird. An invalidation listener on getChildren() should suffice, though.

TomasMikula commented 8 years ago

Yes.

No, these will be unbound automatically as soon as L76-L77 are unbound. That means L76-L77 need to be unbound.

JordanMartinez commented 8 years ago

Ok. Here's what I have:

class VirtualizedScrollPane /* .... */ {
    private Var<Double> hbarValue;
    private Var<Double> vbarValue;

    VirtualizedScrollPane(V content) {
        // constructor stuff

        // scrollbar positions
        hbarValue = Var.doubleVar(hbar.valueProperty());
        vbarValue = Var.doubleVar(vbar.valueProperty());
        Bindings.bindBidirectional(
                hbarValue,
                content.estimatedScrollXProperty());
        Bindings.bindBidirectional(
                vbarValue,
                content.estimatedScrollYProperty());
        // and the rest that is normally there...

        getChildren().addListener((Observable obs) -> dispose());
    }

    /**
     * Unbinds scrolling from Content before returning Content.
     * @return - the content
     */
    V removeContent() {
        getChildren().clear();
        return content;
    }

    private void dispose() {
        hbarValue.unbindBidirectional(content.estimatedScrollXProperty());
        vbarValue.unbindBidirectional(content.estimatedScrollYProperty());
        unbindScrollBar(hbar);
        unbindScrollBar(vbar);
    }

    private void unbindScrollBar(ScrollBar bar) {
        bar.maxProperty().unbind();
        bar.unitIncrementProperty().unbind();
        bar.blockIncrementProperty().unbind();
        bar.valueProperty().unbind();
    }

Edit: changed InvaldiationListener to be a lambda

TomasMikula commented 8 years ago

This seems unnecessary:

bar.valueProperty().unbind();

OTOH, you need to add

bar.visibleProperty().unbind();
TomasMikula commented 8 years ago

Also please port Javadoc comments from the original VirtualFlow.java to the new one where applicable.

JordanMartinez commented 8 years ago

Oh, that was supposed to be visibleProperty()

Ok. I'll take care of that.

TomasMikula commented 8 years ago

It seems that cellToViewport methods also got lost in translation.

TomasMikula commented 8 years ago

A good sanity check would be to test RichTextFX with this new Flowless.

JordanMartinez commented 8 years ago

I'm checking it right now.

JordanMartinez commented 8 years ago

VirtualFlow isn't public so it can't be accessed outside of its package.... Fixing that.

JordanMartinez commented 8 years ago

Yeah... there's a lot of things that need to be made public now.

JordanMartinez commented 8 years ago

If StyledTextArea implements Virtualized, it conflicts with totalWidth/HeightEstimateProperty methods because they return DoubleProperty instead of Val<Double>

JordanMartinez commented 8 years ago

Otherwise, it seems to work fine. Once the skin gets removed, the actual properties from VirtualFlow could be returned.

JordanMartinez commented 8 years ago

Edit: Bug is in RichText's current version (0.6.10).

I found a bug. If I copy a text using the RichText demo and paste it. I get this:

Exception in thread "JavaFX Application Thread" java.lang.NullPointerException
    at org.fxmisc.richtext.demo.richtext.RichText.lambda$null$24(RichText.java:154)
    at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
    at java.util.Iterator.forEachRemaining(Iterator.java:116)
    at java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:545)
    at java.util.stream.AbstractPipeline.evaluateToArrayNode(AbstractPipeline.java:260)
    at java.util.stream.ReferencePipeline.toArray(ReferencePipeline.java:438)
    at org.fxmisc.richtext.demo.richtext.RichText.lambda$start$29(RichText.java:154)
    at org.reactfx.value.ChangeListenerWrapper.accept(Val.java:758)
    at org.reactfx.util.AbstractReducingStreamNotifications.lambda$head$275(NotificationAccumulator.java:248)
    at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
    at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
    at org.reactfx.value.ValBase.invalidate(ValBase.java:32)
    at org.reactfx.SuspendableBoolean.release(SuspendableBoolean.java:24)
    at org.reactfx.CloseableOnceGuard.close(Guard.java:49)
    at org.reactfx.MultiGuard.close(Guard.java:83)
    at org.fxmisc.richtext.StyledTextArea.replace(StyledTextArea.java:804)
    at org.fxmisc.richtext.TextEditingArea.replace(TextEditingArea.java:212)
    at org.fxmisc.richtext.EditActions.replaceSelection(EditActions.java:129)
    at org.fxmisc.richtext.ClipboardActions.paste(ClipboardActions.java:88)
    at org.fxmisc.richtext.skin.StyledTextAreaBehavior.lambda$static$70(StyledTextAreaBehavior.java:65)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate$On.lambda$null$23(EventHandlerTemplate.java:156)
    at java.util.Optional.ifPresent(Optional.java:159)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate$On.lambda$act$24(EventHandlerTemplate.java:155)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate$Builder.lambda$create$21(EventHandlerTemplate.java:117)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate.lambda$null$17(EventHandlerTemplate.java:39)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate.lambda$null$15(EventHandlerTemplate.java:26)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate.lambda$null$19(EventHandlerTemplate.java:49)
TomasMikula commented 8 years ago

Yeah, don't bother StyledTextArea implementing Virtualized before we get rid of skin. Whether or not the rendering is virtualized is now completely up to the skin.

Yeah, there is a bug. But AFAIK, the bug is not present in the released 0.6.10 version, only in the current master. This is due to half-baked incomplete TomasMikula/RichTextFX#6. I should have tested it better before merging.

JordanMartinez commented 8 years ago

I see you've fixed the copy/paste with paragraph styles.

TomasMikula commented 8 years ago

Yes. If you are happy with this PR and there aren't any troubles porting RichTextFX to this version of Flowless, I'm ready to merge this.

JordanMartinez commented 8 years ago

This might be a stupid question, but is this supposed to happen? When I write the following code in StyledTextAreaView's constructor:

VirtualFlow virtuaFlow = // its constructor...

VirtualizedScrollPane<VirtualFlow> vsPane = new VirtualizedScrollPane(virtualFlow);
getChildren().add(vsPane);

Any text is no longer displayed, even when area is initialized with text.

TomasMikula commented 8 years ago

Well, no, that's not supposed to happen :D

JordanMartinez commented 8 years ago

I think I found another issue....

Edit: I encountered another issue that happened when I copied/pasted a text that had a background color (though I think it's technically the highlight color). It only occurred when using this branch of Flowless.

Edit 2: Ok, it's no longer showing up anymore. I'm not sure why it isn't happening anymore, but this is what I did to get it:

  1. Start the RichText demo
  2. Type some text
  3. Select some text and change it's background color (last color picker on the lower right)
  4. Copy the text
  5. Paste the text.

I got this exception:

Exception in thread "JavaFX Application Thread" java.lang.NullPointerException
    at org.fxmisc.richtext.demo.richtext.RichText.lambda$null$18(RichText.java:155)
    at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
    at java.util.Iterator.forEachRemaining(Iterator.java:116)
    at java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
    at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
    at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
    at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:545)
    at java.util.stream.AbstractPipeline.evaluateToArrayNode(AbstractPipeline.java:260)
    at java.util.stream.ReferencePipeline.toArray(ReferencePipeline.java:438)
    at org.fxmisc.richtext.demo.richtext.RichText.lambda$start$23(RichText.java:157)
    at org.reactfx.value.ChangeListenerWrapper.accept(Val.java:758)
    at org.reactfx.util.AbstractReducingStreamNotifications.lambda$head$275(NotificationAccumulator.java:248)
    at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:68)
    at org.reactfx.ObservableBase.notifyObservers(ObservableBase.java:57)
    at org.reactfx.value.ValBase.invalidate(ValBase.java:32)
    at org.reactfx.SuspendableBoolean.release(SuspendableBoolean.java:24)
    at org.reactfx.CloseableOnceGuard.close(Guard.java:49)
    at org.reactfx.MultiGuard.close(Guard.java:83)
    at org.fxmisc.richtext.StyledTextArea.replace(StyledTextArea.java:804)
    at org.fxmisc.richtext.TextEditingArea.replace(TextEditingArea.java:212)
    at org.fxmisc.richtext.EditActions.replaceSelection(EditActions.java:129)
    at org.fxmisc.richtext.ClipboardActions.paste(ClipboardActions.java:88)
    at org.fxmisc.richtext.skin.StyledTextAreaBehavior.lambda$static$15(StyledTextAreaBehavior.java:65)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate$On.lambda$null$23(EventHandlerTemplate.java:156)
    at java.util.Optional.ifPresent(Optional.java:159)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate$On.lambda$act$24(EventHandlerTemplate.java:155)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate$Builder.lambda$create$21(EventHandlerTemplate.java:117)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate.lambda$null$17(EventHandlerTemplate.java:39)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate.lambda$null$15(EventHandlerTemplate.java:26)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate.lambda$null$19(EventHandlerTemplate.java:49)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate.lambda$null$15(EventHandlerTemplate.java:26)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate.lambda$null$15(EventHandlerTemplate.java:26)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate.lambda$null$15(EventHandlerTemplate.java:26)
    at org.fxmisc.wellbehaved.event.EventHandlerTemplate.lambda$bind$14(EventHandlerTemplate.java:18)
    at com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:86)
    at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:238)
    at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:191)
    at com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
    at com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
    at javafx.event.Event.fireEvent(Event.java:198)
    at javafx.scene.Scene$KeyHandler.process(Scene.java:3964)
    at javafx.scene.Scene$KeyHandler.access$1800(Scene.java:3910)
    at javafx.scene.Scene.impl_processKeyEvent(Scene.java:2040)
    at javafx.scene.Scene$ScenePeerListener.keyEvent(Scene.java:2501)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler$KeyEventNotification.run(GlassViewEventHandler.java:197)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler$KeyEventNotification.run(GlassViewEventHandler.java:147)
    at java.security.AccessController.doPrivileged(Native Method)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler.lambda$handleKeyEvent$353(GlassViewEventHandler.java:228)
    at com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:389)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler.handleKeyEvent(GlassViewEventHandler.java:227)
    at com.sun.glass.ui.View.handleKeyEvent(View.java:546)
    at com.sun.glass.ui.View.notifyKey(View.java:966)
    at com.sun.glass.ui.gtk.GtkApplication._runLoop(Native Method)
    at com.sun.glass.ui.gtk.GtkApplication.lambda$null$49(GtkApplication.java:139)
    at java.lang.Thread.run(Thread.java:745)
Exception in thread "JavaFX Application Thread" java.lang.NullPointerException
    at org.fxmisc.richtext.InlineStyleTextArea.lambda$new$9(InlineStyleTextArea.java:24)
    at org.fxmisc.richtext.skin.ParagraphBox.<init>(ParagraphBox.java:76)
    at org.fxmisc.richtext.skin.StyledTextAreaView.createCell(StyledTextAreaView.java:386)
    at org.fxmisc.richtext.skin.StyledTextAreaView.lambda$new$69(StyledTextAreaView.java:114)
    at org.fxmisc.flowless.CellPool.getCell(CellPool.java:20)
    at org.fxmisc.flowless.CellListManager.cellForItem(CellListManager.java:75)
    at org.reactfx.collection.MappedList.get(MappedList.java:27)
    at org.reactfx.collection.MemoizationListImpl.get(MemoizationList.java:99)
    at org.fxmisc.flowless.CellListManager.getCell(CellListManager.java:64)
    at org.fxmisc.flowless.CellPositioner.getSizedCell(CellPositioner.java:129)
    at org.fxmisc.flowless.VirtualFlow.getCell(VirtualFlow.java:143)
    at org.fxmisc.richtext.skin.StyledTextAreaView.followCaret(StyledTextAreaView.java:277)
    at org.fxmisc.richtext.skin.StyledTextAreaView.layoutChildren(StyledTextAreaView.java:212)
    at javafx.scene.Parent.layout(Parent.java:1079)
    at javafx.scene.Parent.layout(Parent.java:1085)
    at javafx.scene.Parent.layout(Parent.java:1085)
    at javafx.scene.Scene.doLayoutPass(Scene.java:552)
    at javafx.scene.Scene$ScenePulseListener.pulse(Scene.java:2397)
    at com.sun.javafx.tk.Toolkit.lambda$runPulse$30(Toolkit.java:355)
    at java.security.AccessController.doPrivileged(Native Method)
    at com.sun.javafx.tk.Toolkit.runPulse(Toolkit.java:354)
    at com.sun.javafx.tk.Toolkit.firePulse(Toolkit.java:381)
    at com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:510)
    at com.sun.javafx.tk.quantum.QuantumToolkit.pulse(QuantumToolkit.java:490)
    at com.sun.javafx.tk.quantum.QuantumToolkit.lambda$runToolkit$404(QuantumToolkit.java:319)
    at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95)
    at com.sun.glass.ui.gtk.GtkApplication._runLoop(Native Method)
    at com.sun.glass.ui.gtk.GtkApplication.lambda$null$49(GtkApplication.java:139)
    at java.lang.Thread.run(Thread.java:745)
JordanMartinez commented 8 years ago

I added these checks to see if the content was being disposed of before actually seeing it, but no text was printed:

getChildren().addListener((Observable obs) -> System.out.println("Children has changed: Called before disposing"));
getChildren().addListener((Observable obs) -> dispose());
getChildren().addListener((Observable obs) -> System.out.println("Children has changed: Called after disposing"));
JordanMartinez commented 8 years ago

I added a check in layoutChildren():

double w = layoutWidth - vbarWidth;
double h = layoutHeight - hbarHeight;

System.out.println("Content is being resized to " + w + ", " + h);
content.resize(w, h);

It kept printing: Content is being resized to -17.0, -17.0

JordanMartinez commented 8 years ago

Adding an additional check in layoutChildren() showed that LayoutBounds width/height was 0/0:

double layoutWidth = getLayoutBounds().getWidth();
double layoutHeight = getLayoutBounds().getHeight();
System.out.println("Layout Bounds is: Width [" + layoutWidth + "] | Height [" + layoutHeight + "]");

Prints: Layout Bounds is: Width [0.0] | Height [0.0]

JordanMartinez commented 8 years ago

I'm pretty sure it's related to the following code. The original VirtualFlow contains this code which is not in the current VirtualizedScrollPane:

getChildren().add(navigator);
clipProperty().bind(Val.map(
    layoutBoundsProperty(),
    b -> new Rectangle(b.getWidth(), b.getHeight())));

There's also no way for VirtualizedScrollPane to know about Navigator because Virtualized doesn't have such a method.

Edit: The code shows up in the current VirtualFlow so I don't know if that would be an issue.

TomasMikula commented 8 years ago

That code was in VirtualFlowContent and as far as I can see it is now in VirtualFlow.

JordanMartinez commented 8 years ago

Any ideas then? Even if I write double layoutWidth = content.getLayoutBounds().getWidth(), and the same for layoutHeight, it still doesn't display the text.

Edit 1: Nevermind! Using content.getLayoutBounds() will display the text. I forgot to remove some other code. Edit 2: Using this approach makes the scroll bar flicker rapidly once text spans outside of view.

JordanMartinez commented 8 years ago

If I add only VirtualFlow to children and layoutChildren() only has the following code, the text still doesn't display:

double layoutWidth = getLayoutBounds().getWidth();
double layoutHeight = getLayoutBounds().getHeight();
content.resize(layoutWidth, layoutHeight);
JordanMartinez commented 8 years ago

Figured out the issue. It has nothing to do with VirtualFlow. Sorry for the false alarm.

StyledTextAreaView's layoutChildren() method wasn't updated as well:

@Override
protected void layoutChildren() {
    virtualFlow.resize(getWidth(), getHeight()); // this line needed to be changed to..
    vsPane.resize(getWidth(), getHeight());  // this line. Once changed, the issue is resolved.

    // rest of the method
}
JordanMartinez commented 8 years ago

K. I'm satisfied with the PR.