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

Generalize TreeItem used in root and children of a TreeView #7

Merged
merged 25 commits into from Mar 13, 2016

Conversation

JordanMartinez
Copy link
Contributor

I named the two functions, projector and injector. However, I wonder if it might be better to name them valToPath and pathToVal. Seems to be more consistent....

…eeding to directly implement them and duplicate code.
…egular TreeItem class for its root and children.
- Now that "T" generic is used in class, update `wrap()`'s generic to "U"
- Return lambda back to method reference (this was accidentally and erroneously reverted in previous commit)
@JordanMartinez JordanMartinez changed the title Tree item general Generalize TreeItem used in root and children of a TreeView Feb 16, 2016
@TomasMikula
Copy link
Owner

Looks reasonable. I would probably try to look if there is a way to avoid these type castings:

PathItem<T> pathCh = (PathItem<T>) ch;

Seems the only reason why you cast it to PathItem is to be able to call getPath(), but you could equally well call project(getValue()), right?

Another thing is, I'm not sure whether having NormalLiveDirs extend LiveDirs is worth it, compared to just having static factory methods on LiveDirs for the "normal" case. The question is, is

NormalLiveDirs<I>

simpler enough for the user than

LiveDirs<I, Path>

?

@JordanMartinez
Copy link
Contributor Author

Seems the only reason why you cast it to PathItem is to be able to call getPath(), but you could equally well call project(getValue()), right?

That's smarter and cleaner.

Another thing is, I'm not sure whether having NormalLiveDirs extend LiveDirs is worth it, compared to just having static factory methods on LiveDirs for the "normal" case.

Would a user subclass NormalLiveDirs? That would probably be my only reason against your suggestion of using a static factory method.

Initially, I was trying to better distinguish LiveDirs basic instance from a CheckBoxLiveDirs instance. That might be worth initializing using a static factory class, but I might need to subclass that for JGitFX (not sure yet).

@TomasMikula
Copy link
Owner

LiveDirs is not meant to be subclassed by the user. Customization by inheritance rarely works well (it doesn't compose well, see e.g. CheckBoxTreeItem 😉).

@JordanMartinez
Copy link
Contributor Author

But I want it to work well! 😆 Good point!

@JordanMartinez
Copy link
Contributor Author

Seems the only reason why you cast it to PathItem is to be able to call getPath(), but you could equally well call project(getValue()), right?

I've tried out your idea but come across a couple of places where I will have to cast the TreeItem to a PathItem at some point. Either that, or I'll have to recompute stuff it already knows, e.g. Files.isDirectory(path).

@JordanMartinez
Copy link
Contributor Author

LiveDirs is not meant to be subclassed by the user. Customization by inheritance rarely works well (it doesn't compose well, see e.g. CheckBoxTreeItem 😉).

I've actually been meaning to ask you about that. Much of what you've written already implements the Directory-watching code I'd need. (I only looked here first because I wasn't familiar with that how to write the necessary code and you'd already done a good job of writing it).

However, in terms of composition, there will be a lot of work I'll have to do if I use your code (specifically, I'm thinking about #2). In some ways, I feel like my need is a bit outside the scope of this project only because TreeView's code has very special requirements with TreeItem-related classes.

So, if I wanted to use part of your code in JGitFX directly (so I wouldn't need to add a dependency somewhere), how do I copy your code into that project's files while still adhering to the BSD License, especially once I start modifying the code according to my needs? Can I even do that?

@TomasMikula
Copy link
Owner

The license requires you to only redistribute the original copyright and license terms somewhere along the source and binary distributions (alongside whatever additional copyright and license terms you wish to add). But between us, don't worry if you don't 😉

@JordanMartinez
Copy link
Contributor Author

Sweet! I'll use that as a last-resort. Thanks :-)

Edit: Besides the two issues I've opened up here, there's also the text coloring that needs to be done for each file's state

  • not added: red
  • newly added: green
  • modified: blue
  • unchanged: black

@JordanMartinez
Copy link
Contributor Author

I've updated this with my CheckBoxTreeCell implementation. It works for the most part.

However, the recursive forced check/uncheck downwards doesn't work past the 1st children level. I'm not entirely sure why, but the stateInvalidation listener isn't getting called

- Checking/Unchecking a directory wouldn't update all of its children recursively to the same state
- Checking/Unchecking a directory would sometimes make its parent's state UNDEFINED when it should be CHECKED or UNCHECKED
@JordanMartinez
Copy link
Contributor Author

Alright! That was hard, but it works!

@TomasMikula
Copy link
Owner

Well, I don't think the checkbox stuff belongs to this library. It is OK to provide a demo that shows this stuff is possible, but the core library itself is not concerned with UI stuff. It provides a model of a (changing) directory tree.

@JordanMartinez
Copy link
Contributor Author

Ok, I can move it to a demo package like the one you have in RichTextFX

@TomasMikula
Copy link
Owner

That would be nice. But in order to not include the demos in the core library jar, it would first need a little reorganization of the directory tree and the build script (as in RichTextFX).

@JordanMartinez
Copy link
Contributor Author

Lol... when I said "package", I meant "module." Might help if I used the right terminology... Yes, that's exactly what I had in mind.

@TomasMikula
Copy link
Owner

👍

@JordanMartinez
Copy link
Contributor Author

How's that?

@TomasMikula
Copy link
Owner

Looks good. So can we eliminate those type casts ((PathItem<T>) child)?

I would call getNormalInstance just getInstance.

@JordanMartinez
Copy link
Contributor Author

I would call getNormalInstance just getInstance.

Smart.

Looks good. So can we eliminate those type casts ((PathItem) child)?

Did you see my previous comment:

I've tried out your idea but come across a couple of places where I will have to cast the TreeItem to a PathItem at some point. Either that, or I'll have to recompute stuff it already knows, e.g. Files.isDirectory(path).

@JordanMartinez
Copy link
Contributor Author

With the code and examples I've used, shouldn't that be dedicated to the public domain? Otherwise, doesn't your BSD license apply to it?

@TomasMikula
Copy link
Owner

Strictly speaking, yes. Otherwise, anyone who copies the code from the demo source would be required to carry on the copyright and license of this project. At least that is my understanding of the license. But I will not sue anyone for stealing code of the demos. Feel free to add a header to the demo files saying they are public domain.

@JordanMartinez
Copy link
Contributor Author

Ok. That should suffice.

@TomasMikula
Copy link
Owner

👍

TomasMikula added a commit that referenced this pull request Mar 13, 2016
Generalize the content of TreeItems from Path to arbitrary T, given functions T -> Path and Path -> T.
@TomasMikula TomasMikula merged commit 7e1d22c into TomasMikula:master Mar 13, 2016
@JordanMartinez JordanMartinez deleted the treeItemGeneral branch March 13, 2016 04:53
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.

None yet

2 participants