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

Scroll bars now optional #11

Merged
merged 12 commits into from
Nov 12, 2015

Conversation

JordanMartinez
Copy link
Contributor

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

@JordanMartinez
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

removeContent would then be implemented as

getChildren().clear();
return content;

@JordanMartinez
Copy link
Contributor Author

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
Copy link
Contributor Author

Do these need to be unbound as well?

@TomasMikula
Copy link
Member

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

@TomasMikula
Copy link
Member

Yes.

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

@JordanMartinez
Copy link
Contributor Author

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
Copy link
Member

This seems unnecessary:

bar.valueProperty().unbind();

OTOH, you need to add

bar.visibleProperty().unbind();

@TomasMikula
Copy link
Member

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

@JordanMartinez
Copy link
Contributor Author

Oh, that was supposed to be visibleProperty()

Ok. I'll take care of that.

@TomasMikula
Copy link
Member

It seems that cellToViewport methods also got lost in translation.

@TomasMikula
Copy link
Member

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

@JordanMartinez
Copy link
Contributor Author

I'm checking it right now.

@JordanMartinez
Copy link
Contributor Author

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

@JordanMartinez
Copy link
Contributor Author

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

@JordanMartinez
Copy link
Contributor Author

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

@JordanMartinez
Copy link
Contributor Author

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

@JordanMartinez
Copy link
Contributor Author

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
Copy link
Member

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 FXMisc/RichTextFX#6. I should have tested it better before merging.

@JordanMartinez
Copy link
Contributor Author

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

@TomasMikula
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@JordanMartinez
Copy link
Contributor Author

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)

hbar.visibleProperty().addListener(obs -> Platform.runLater(() -> requestLayout()));
vbar.visibleProperty().addListener(obs -> Platform.runLater(() -> requestLayout()));

getChildren().addListener((Observable obs) -> dispose());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This listener needs to be added after the child list has been populated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Text is still not appearing even after changing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... I think I get what's happening now.
If you look at the mouse, the scroll bar only barely appears. So I guess it is technically working.
rich text demo_003

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If using Flowless 0.4.7, that small scrollbar arrow doesn't appear.

@JordanMartinez
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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

@JordanMartinez
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

K. I'm satisfied with the PR.

TomasMikula added a commit that referenced this pull request Nov 12, 2015
VirtualFlow now without scroll bars, embeddable in VirtualizedScrollPane.
@TomasMikula TomasMikula merged commit 6f93817 into FXMisc:master Nov 12, 2015
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

Successfully merging this pull request may close these issues.

2 participants