Workaround to correct trackpad scrolling gestures under macOS

[Arcnor]

I didn't create this fix, but @StrangeNoises. It was never applied for some reason, but it should fix FXMisc/RichTextFX/issues/265.

I don't know if this is the "correct" way of fixing it or not, but it works as expected on my OSX machine.

[JordanMartinez]

Aye, it works. However, I don't know whether this is the best way to fix it (that's why I didn't merge it). Have you read through my comment in #56?

[Arcnor]

No, I didn't see that issue, just the other one I mentioned.

To be honest, child versus container handling sounds more like a philosophical question at this point, while there is a real problem of Flowless being broken in OSX at this point, and this really fixes it.

Also, internal changes can always be remade later if needed.

[JordanMartinez]

Also, internal changes can always be remade later if needed.

Can't argue with that...

child versus container handling sounds more like a philosophical question at this point

I think the larger issue of why I didn't like this approach was a (perhaps very unreasonable) fear that it might create a memory leak because I see an addEventListener without a corresponding removeEventListener. If this is an unreasonable fear, please correct my misunderstanding.

[Arcnor]

It seems an event handler is only added for non reusable cells, with the hope that they will get garbage collected, but unless the link between them is made weak, I guess they will never be collected.

I haven't tested this behavior yet, so cannot confirm if that assumption is right.

[StrangeNoises]

That was my assumption yes, as nothing holds a reference to the event handler except the cell itself, so should be garbage-collected when the cell is. ISTR there's something in the javafx javadocs to that effect. Elsewhere I've also not really seen explicit removal of event handlers of a node before its own destruction.

Bearing in mind the event handler itself is just a methodref to a method fulfilling the EventHandler functional interface. There's no bigger object involved. What exactly is there more to be garbage collected? That value is just set on the scroll event properties on the node. The node dies, so should its property values. :-)

[JordanMartinez]

Rather than just assume, could we profile this PR and the current master branch to insure no memory leak is being added in this PR? Once the assumption is proven, there's no reason not to merge this PR and a very good reason to do so. As @Arcnor said, the internal handling can always be changed later if we should so desire.

[JordanMartinez]

@Arcnor @StrangeNoises One other thought... If this is being submitted to specifically fix the corresponding issue in RichTextFX, we could always fix it there, too, using the same event-forwarding approach that @StrangeNoises created. Such a thing could be done in the createCell method that would add the listener to the cell's node and remove it when the node gets disposed.

That would fix the problem and I'd merge that immediately, giving us time to figure out how to best proceed in fixing the code here since I'm guessing profiling this PR would take more time and energy than submitting a PR such as that.

[JordanMartinez]

Using the following code (not sure whether I implemented the reusable cell correctly), it seems like there is a very small memory leak:

import javafx.application.Application;
import javafx.application.Platform;
import javafx.beans.property.SimpleBooleanProperty;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.scene.Scene;
import javafx.scene.paint.Color;
import javafx.scene.shape.Rectangle;
import javafx.stage.Stage;

import java.text.NumberFormat;
import java.util.ArrayList;
import java.util.List;
import java.util.function.Function;

public class ProfileTest extends Application {

    private class R extends Rectangle {

        R() {
            super();
            setWidth(20);
            setHeight(20);
            setFill(Color.BLACK);
        }
    }

    public static void main(String[] args) {
        launch(args);
    }

    private final SimpleBooleanProperty showTop = new SimpleBooleanProperty(false);
    private boolean shouldShowTop() { return showTop.get(); }
    private void invertView() { showTop.set(!showTop.get()); }

    @Override
    public void start(Stage primaryStage) {
        boolean useNonReusableCells = false;
        System.out.println("Using reusable cells? " + !useNonReusableCells);

        System.gc();
        System.out.println("Starting: " + memoryStatus());
        List<Integer> l = new ArrayList<>(100);
        for (int i = 0; i < 100; i++) {
            l.add(i);
        }
        ObservableList<Integer> list = FXCollections.observableList(l);
        System.gc();
        System.out.println("Finished list creation: " + memoryStatus());

        VirtualFlow<Integer, Cell<Integer, R>> flow = VirtualFlow.createHorizontal(
                list,
                useNonReusableCells ? useNonReusableCells() : useReusableCells()
        );
        primaryStage.setScene(new Scene(flow, 1960, 100));
        primaryStage.show();

        System.gc();
        System.out.println("Finished setting up stage: " + memoryStatus());

        for (int i = 0; i < 10_000; i++) {
            if (shouldShowTop()) {
                flow.showAsFirst(0);
            } else {
                flow.showAsLast(list.size() - 1);
            }
            invertView();
        }
        System.gc();
        System.out.println("Final status: " + memoryStatus());
        Platform.exit();
    }

    public Function<Integer, Cell<Integer, R>> useReusableCells() {
        return i -> new Cell<Integer, R>() {
                R h = new R();
                @Override
                public R getNode() {
                    return h;
                }

                @Override
                public boolean isReusable() {
                    return true;
                }

                @Override
                public void updateItem(Integer item) {
                    // do nothing
                }
        };
    }

    public Function<Integer, Cell<Integer, R>> useNonReusableCells() {
        return i -> Cell.wrapNode(new R());
    }

    public String memoryStatus() {
        Runtime runtime = Runtime.getRuntime();
        long maxMemory = runtime.maxMemory();
        long allocatedMemory = runtime.totalMemory();
        long freeMemory = runtime.freeMemory();

        NumberFormat format = NumberFormat.getInstance();
        StringBuilder sb = new StringBuilder();
        sb.append("Used ");
        sb.append(format.format((runtime.totalMemory() - runtime.freeMemory())));
        sb.append(" B from ");
        sb.append(format.format((freeMemory + (maxMemory - allocatedMemory))));
        return sb.toString();
    }
}

which outputs (formatted for clarity):

=== Without Change
Using reusable cells? false
- Starting:                       Used 2,906,608 B from 1,835,897,256
- Finished list creation:         Used 2,296,176 B from 1,835,857,552
- Finished setting up stage:      Used 4,274,280 B from 1,833,879,448
- Final status:                   Used 3,607,048 B from 1,834,546,680

Using reusable cells? true
- Starting:                       Used 2,906,744 B from 1,835,897,120
- Finished list creation:         Used 2,296,352 B from 1,835,857,376
- Finished setting up stage:      Used 4,283,880 B from 1,833,869,848
- Final status:                   Used 3,617,896 B from 1,834,535,832

=== With Change

Using reusable cells? false
- Starting:                       Used 2,906,704 B from 1,835,897,160
- Finished list creation:         Used 2,296,360 B from 1,835,857,368
- Finished setting up stage:      Used 4,319,952 B from 1,833,833,776
- Final status:                   Used 3,653,096 B from 1,834,500,632

Using reusable cells? true
- Starting:                       Used 2,906,672 B from 1,835,897,192
- Finished list creation:         Used 2,296,280 B from 1,835,857,448
- Finished setting up stage:      Used 4,366,952 B from 1,833,786,776
- Final status:                   Used 3,699,568 B from 1,834,454,160

[Arcnor]

Hi, sorry for the delay, I haven't been working on the project that uses this.

To answer your question above: No, I don't want to use RichText but Flowless directly (RichText is not flexible enough for my needs).

I've also run your test, but with 1M rows instead of just 100, and 1M iterations on the change loop, also ran the test multiple times. Here are my results, but I don't see any leak here?

Without change:

Using reusable cells? true
Starting:                  Used 7,152,584 B from 7,631,272,704
Finished list creation:    Used 6,424,736 B from 7,629,305,696
Finished setting up stage: Used 7,970,528 B from 7,627,759,904
Final status:              Used 7,234,880 B from 7,628,495,552

Using reusable cells? false
Starting:                  Used 7,152,520 B from 7,631,272,768
Finished list creation:    Used 6,424,592 B from 7,629,305,840
Finished setting up stage: Used 7,970,336 B from 7,627,760,096
Final status:              Used 7,232,968 B from 7,628,497,464

With change:

Using reusable cells? true
Starting:                  Used 7,152,552 B from 7,631,272,736
Finished list creation:    Used 6,424,552 B from 7,629,305,880
Finished setting up stage: Used 7,970,064 B from 7,627,760,368
Final status:              Used 7,234,304 B from 7,628,496,128

Using reusable cells? false
Starting:                  Used 7,153,280 B from 7,631,272,008
Finished list creation:    Used 6,425,280 B from 7,629,305,152
Finished setting up stage: Used 7,970,856 B from 7,627,759,576
Final status:              Used 7,233,872 B from 7,628,496,560

[JordanMartinez]

Ok, thanks for running the test. I think that proves @StrangeNoises assumption true.

[Arcnor]

Awesome, thanks.

Unfortunately, this doesn't seem to fix the issue completely. I'm getting the same problem randomly, but I can't figure out why (haven't debugged it, though)

Anyway, this is better than having it not work at any point.

[JordanMartinez]

Hmm.... Is this an issue in JavaFX for OSX then? (I'm still wondering why this doesn't occur on the other two major platforms)

[JordanMartinez]

After I merged this, I received a report about the build failing: https://travis-ci.org/FXMisc/Flowless/jobs/363634546#L2550

[Arcnor]

Well, I haven't tried any other platform, not sure if it's Mac specific. However, there is no "smooth" scrolling (as in, scrolling with "weight", whatever that's called) in Windows at least? So you won't see this problem happening, I think.

Also, I think I saw the explanation of why this was the way it is on OSX in one of the liked issues, but I might be mistaken.

[JordanMartinez]

StrangeNoises had a comment in the issue in RichTextFX that probably mentioned something in that regard. It's been a while since I read through it.

[Arcnor]

About the build, that's very strange. As far as I know, the PR was built before being merged as well (because Travis won't let you merge when the build is red, unless I'm mistaken about that), and it was ok?

[Arcnor]

Yeah, it was: https://travis-ci.org/FXMisc/Flowless/builds/353589200

[JordanMartinez]

Yeah, it was. That's why I'm a bit confused... The test might need to be rewritten slightly. I'll have to look into it more next week.

[JordanMartinez]

Latest build #66 worked fine. Weird.

[Arcnor]

Flaky test, then, probably.

I still need to go through this and check why is not working in some situations, but I think it's good that we have this in place anyway.

Thanks again!

[JordanMartinez]

Just a thought, but would there be any reason to change the internal way of adding/setting the scroll-event-forwarding event handlers to this way?

Hope you get it worked out!

[Arcnor]

I haven't tried your change, but it looks similar to what there was before. I'm guessing now you're adding the event handler even for reusable cells, but you're guaranteed to add them only when the cell is added to the stage? (so you do it only once)

Maybe @StrangeNoises can comment a bit more on it.