Allow CheckBoxTreeCell CellFactory

[JordanMartinez]

I was hoping to use this library in JGitFX. However, LiveDirs does not allow the option of using the CheckBoxTreeCell factory, which is essential for designing a commit or revert dialog where the user can check which files they want to stage and commit or revert.

[TomasMikula]

Hi Jordan, you are in control of creating your TreeView, so you can set your own cell factory.

[JordanMartinez]

Right, but in the documentation, it says,

this cell factory assumes that the TreeView root, and all children are instances of CheckBoxTreeItem, rather than the default TreeItem class that is used normally.

If I do set the cell factory as you mentioned, I get the desired view, but the functionality doesn't work. Checking a parent directory will not also check its subdirectories and files.

[TomasMikula]

Ah, I see, that's unfortunate.

This is another example of how most JavaFX controls are not very reusable beyond a very limited use case that someone had in mind when designing them. The implementation of the checkbox functionality must use instanceof checks at runtime, which in 99.99% of cases is a terrible idea, because it makes the types lie to you (in this case, at compile time it says any TreeItem is fine, but at runtime it says "we don't work with this TreeItem!"). When enough lies add up, it is not possible anymore to reason about your programs. Types are machine checked documentation, so let them work for you, don't work around them.

Now, I will not go down the path of duplicating code for the special case of CheckBoxTreeItems. Such design does not scale. When JavaFX comes up with yet another special kind of tree item, do we duplicate again to handle that case? I have to say no to that. We need to come up with a better solution. It will likely involve some additional type parameter somewhere.

[JordanMartinez]

This is another example of how most JavaFX controls are not very reusable beyond a very limited use case that someone had in mind when designing them.

Lol... no kidding...

It will likely involve some additional type parameter somewhere.

Have you see my PR #3?

[JordanMartinez]

Lol... I just reread my own comment about the documentation, I guess my recent work in my PR actually shouldn't be done: the root also needs to be CheckBoxTreeItem.

[JordanMartinez]

I've submitted another PR to account for the above realization (#4)

[TomasMikula]

I saw the PR and by

Now, I will not go down the path of duplicating code for the special case of CheckBoxTreeItems. Such design does not scale. When JavaFX comes up with yet another special kind of tree item, do we duplicate again to handle that case? I have to say no to that.

I mean that adding CheckBoxPathItem, which basically duplicates code from the original PathItem, is the path I don't want to go.

[JordanMartinez]

Right, just realized that. I went the route I did because I wasn't sure how to account for that, but I see how to do it now.

[JordanMartinez]

On second thought, this code doesn't work:

abstract class PathItem<GenericTreeItem extends TreeItem<Path>> extends GenericTreeItem {
}

[JordanMartinez]

I'm guessing we'll need to convert the PathItem.java classes to interfaces and use default methods? I guess accessibility won't be an issue since it'll remain package private.

[JordanMartinez]

Nah, even that doesn't work out well due to TreeItem#getChildren calls.

[TomasMikula]

Here's an idea. Currently we have

PathItem extends TreeItem<Path>

Let's keep PathItem parameterized, i.e.

PathItem<T> extends TreeItem<T>

The idea is that T will be an object from which a Path can be obtained. So it can be Path itself, or, for example, Tuple2<Path, Var<Boolean>>. The key is that PathItem and its implementations don't care what T really is, as long as you also provide a function to extract a Path from T. So PathItem constructor would take an additional argument Function<T, Path> project. DirItem will also need to be able to create T from just a Path, so it will need yet another function argument Function<Path, T> inject.

This additional type parameter will also have to be added all the way to DirectoryModel and LiveDirs. Factory methods can be added to create LiveDirs<Path>, with both inject and project mentioned above defaulting to Function.identity().

I haven't worked out all the details, but it seems promising.

Maybe another desired tweak will be to provide versions of DirectoryModel.creations() etc. that emit events containing TreeItem<T> (instead of just the path to the file that has been created), so that when a file gets created, you can traverse the tree to make changes.

[JordanMartinez]

Correct me if I'm wrong, but the only way to make this scalable isn't much different than the poorly-scalable idea that my PR uses because one cannot write:

class PathItem<T extends TreeItem<Path>> extends T {}

PathItem extends TreeItem<Path> and overrides it's isLeaf(). If I wanted to add CheckBoxTreeItem (or some other future TreeItem subclass), I also need to override its isLeaf() method. Since I can't modify the TreeItem subclass to extend from PathItem as opposed to TreeItem<Path>, this requires its own implementation. However, as soon as we go that route, we can no longer use generics. Each TreeItem-related class we would wish to implement would need its own PathItem-like classes that extends that base class.

This is essentially what my approach does. However, this approach could be further refined. If each TreeItem subclass needs to write its own PathItem, FileItem, DirItem, and TopLevelDirItem, then much of that code duplication could be reduced via interfaces' default methods and a static class for any TreeItem-related methods.

If Java supported traits, then this wouldn't be as big of an issue.

[JordanMartinez]

I'm trying out another way. I'm using default methods in interfaces to make things more generic (public methods are named the same but private methods are now prefixed with an underscore). To get around the problem of accessing the trees, I'm using a hack:

interface PathItem<T extends TreeItem<Path> & PathItem<T>, /* other generics */> {
    T getTree();
}

Whenever a new TreeItem subclass comes out, we can quickly implement it by following a simple code pattern (See PathItem<T, D, F>'s javadoc in my generalizeTreeItem branch).

[JordanMartinez]

I've finished the implementation of this new way. See #5.

[JordanMartinez]

Lol... Just now saw your latest comment.

[JordanMartinez]

I'm looking at your comment again. In some ways, I feel like the ideas you are proposing there would be useful in resolving #2.

[JordanMartinez]

So, you are proposing something like this:

PathItem<T> extends TreeItem<T> {
    PathItem( /* args */, Function<T, Path> project) {}
}
FileItem<T> extends PathItem<T> { /* no changes */ }
DirItem<T> extends PathItem<T> {
    DirItem( /* args */, Function<Path, T> inject) {}
}
TopLevelDirItem<T> extends DirItem<T> { /* no changes */ }

The regular TreeItem code would remain the same:

// For regular TreeItems
PathItem<Path> pathItem = new PathItem<>(/* args */, Function.identity());
TreeView<Path> view = new TreeView<>(root);

But the CheckBoxTreeItem would have some additional configuration

// For CheckBoxTreeItems
PathItem<Tuple2<Path, Var<Boolean>>> checkBoxItem = new PathItem<>(/* args */, Tuple2::get1);
TreeView<Tuple2<Path, Var<Boolean>>> view = new TreeView<>(root);
view.setCellFactory(customCheckBoxFactory);

where customCheckBoxFactory would do what CheckBoxTreeCell does but for our specific handling.

If so, yes, I do believe that would work.

[TomasMikula]

Yes, that's a pretty accurate description of what I meant.

[JordanMartinez]

I wish I would have read that and understood it before writing #5 because that got really complex really fast :smiley:

[TomasMikula]

I'm sure it was a great learning experience :)

[JordanMartinez]

Lol... It was.

[JordanMartinez]

Ran into a problem in my attempt to implement the CheckBoxTreeCell.

The reason why CheckBoxTreeCell requires CheckBoxTreeItems is because the Item class (model) is responsible for updating the parent directory and its children's selection states based on the context (e.g. if parent directory is checked (and all its children are) and then the user unchecks one of its children, that parent directory needs to be unchecked as well. The child that gets unchecked is responsible for unchecking its parent).

[TomasMikula]

TreeCell.getTreeItem() gives you the tree item, right?

[JordanMartinez]

I looked at OpenJDK and looked through how they wrote the code for the initial CheckBoxTreeCell to help guide me. I understand it a lot better now.

The issue I'm running into is how to update the parent/children's selected/intermediate properties efficiently.

[TomasMikula]

In your custom TreeCell's updateItem method, you can register listener on the selectedProperty of the item's value. This listener, as a closure, can access that cell's TreeItem. From there, you can access that item's parent and children. Am I missing something?

[TomasMikula]

Btw, if I needed to represent 3 values (selected, intermediate, unselected), I would define an enum for them and have one Property of that enum value, rather than two boolean properties.

[JordanMartinez]

Let me explain. Since every TreeCell has added a listener to its TreeItem's selectedProperty, I realized that it would be inefficient. When the first TreeCell's checkbox is updated (whether it is now checked, undefined, or unchecked), my code would start a recursive call upwards to the parent.

Using pseduo code:

// ACTUAL CODE
checkbox.selectedProperty.bindbidirectional(selectedProperty());
// where state property is 
// "Val.combine(selectedProperty, intermediateProperty, null);"
stateProperty.addListener(obs -> updateCheckBoxItems());

public void updateCheckBoxItems() {
    // no need to handle leaf/node's checkbox update 
    //  because it's bound bidirectionally...

    // update going upwards
    TreeItem parent = getTreeItem().getParent();
    if (parent != null) { updateParent(parent) }

    // now that upwards is done, do downwards update
    if (newstate == checked) { 
        force check all children and their children recursively 
    } else if (newstate == unchecked) { 
        force uncheck all children and their children recursively 
    }
    // don't do anything for intermediate state.
}

    public void updateParent(TreeItem parentItem) {
        if (parents children are all checked) { 
            make parent checked also 
        } else if (at least one child is checked) { 
            make parent undefined [indeterminate == true] 
        } else ( /* no children checked */ ) {
             make parent unchecked 
        }
        // now that parent is figured out, continue the recursion on its parent
        updateParent(parentItem.getParent())
    }

I realized that when I run the code make parent [state], it will also call it's listener (which calls the updateCheckBoxItems, which would call the code that does the downward updating.

So, I would get a crazy chain of updates that looked like this:

1. Child gets updated
2. Child updates its parent
3. parent is updated
4. parent updates its parent (go to number 3)
5. root is reached (doesn't have a parent)
6. root begins recursive call downwards (forces item to be checked/unchecked)
7. root's child gets updated
8. root's child updates it's parent (the root)
9. Go to number 5.

I get that some sort of locking mechanism needs to be involved, but I wasn't sure how that should work. I think I realized that the only way around this was to only run the listener's code if the lock wasn't locked yet. CheckBoxTreeItem has a private static boolean lock, but I can't do that on TreeItem :-)

That's about how far I got before something else needed my attention.

[TomasMikula]

You don't need to call updateParent recursively, since that's the job of the parent's listener. Same for updating children. The crazy recursion scheme would in fact terminate very quickly, because if you attempt to set a property to a value that it currently holds, it does not result in a change event.

[JordanMartinez]

You don't need to call updateParent recursively, since that's the job of the parent's listener.

I guess now that I see that my original code does work (see below), yeah you're right. If I update the parent, then allow the parent listener to continue that process, I don't need to make it recursive.

The crazy recursion scheme would in fact terminate very quickly, because if you attempt to set a property to a value that it currently holds, it does not result in a change event.

Turns out it does work. I assumed that, in the downward update, it would set its child to a new value. However, that doesn't occur when a parent's new state is an intermediate state and when it's checked or unchecked and it tries to force check/uncheck its children, those children will already be up-to-date because of the upwards update (and thus a no-op). In the one case where that isn't already true, that's the case I want it to work that way....

[JordanMartinez]

I've been thinking this issue over and I think it's not needed....

I realized that the Commit dialog or Revert dialog that would be used in JGitFX is static. The dialog that displays the TreeView that uses CheckBoxTreeItems to display the items on which a user wants some action to be applied is called via showAndWait(). Since that pauses the JavaFX Application Thread until it is closed, I don't need live updating. Even if it is called via show() and doesn't pause the thread, there could be a refresh button within the dialog that will reload the TreeView. This is how IntelliJ Idea currently deals with the problem.

Beyond that, if I implement my own TreeView, I can also implement #2 easily. Still, I think #2 would be useful for this library.

[JordanMartinez]

Another update: Although I won't be using the CheckBox-pseudo-code workaround you suggested in the manner I initially thought I would, I still think generalizing the code is better.

For example, I want to include a directory watcher in a JGitFX demo, but I want to update the text color of the file to represent the status of that file in the git repo (added, modified, untracked, etc.). With the generic-approach and the inject/project functions, I would be able to implement that.

[JordanMartinez]

Closing via #7