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.