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

Restructure Builders.kt and Nodes.kt into Controls.kt, ItemControls.kt and Layouts.kt #57

Closed
edvin opened this issue Mar 27, 2016 · 25 comments

Comments

@edvin
Copy link
Owner

edvin commented Mar 27, 2016

For a while I've been bothered by the structure of Builders.kt vs Nodes.kt. We have stuff in Builders.kt that are not builders, but rather just extension functions that probably belong in Nodes.kt. (Example: TableView<S>.column - this does not change the hierarchy, and is not an extension of Pane).

Maybe it's time to consider organising this differently. We could create one file for List/Table-like controls, and include both builders and extensions for these controls in that file. Similarly, all text based builders and extensions like TextField and TextArea could go into one file etc.

This will make it much easier to see what's available (and what's missing) for a certain control or group of controls.

I'm still very fond of keeping everything in the tornadofx package, so you still only need a single import to work with the whole framework.

This change would not affect usage of the framework in any way.

Thoughts or other suggestions?

@edvin edvin mentioned this issue Mar 27, 2016
@thomasnield
Copy link
Collaborator

I've thought about this too. Maybe we cam organize a "Nodes" package and put a separate file for each Node type in it? That might break existing code though with package paths...

@edvin
Copy link
Owner Author

edvin commented Mar 27, 2016

Since I suspect TornadoFX will remain "lightweight" I would like to keep everything in the tornadofx package for now. We can still use separat files for either each node type or each "node group". Some files would just be too small to justify it, and Kotlin has an extra space penalty per file, so I think it's better to go with multiple classes per file.

@thomasnield
Copy link
Collaborator

Didn't know that, alright. I think your proposed approach makes sense then...

@edvin
Copy link
Owner Author

edvin commented Mar 27, 2016

An alternative would be a wider group of nodes in each file, so we don't create that many files. Controls.kt could be one, Layouts.kt could be another and maybe one for list-based controls - ListControls or something, since they have lots of methods.

@edvin
Copy link
Owner Author

edvin commented Mar 27, 2016

By the way, I've never created a framework or system with everything in one package, but for TornadoFX it just seems right, considering it's a Kotlin project (which let's you separat stuff in files, not classes) and the small size of the code base.

@thomasnield
Copy link
Collaborator

That is true, it is hard to break habits that are the result of conventions and constrains in Java. Okay, one package makes sense then.

@hastebrot
Copy link
Collaborator

Single package framework sounds good. I also think the files should be restructured a bit. Controls.kt and Layouts.kt seem plausible. There could be ItemControls instead of ListControls for all that have items (ComboBox, ListView, TableView, ...). Unfortunately TreeView doesn't fit into the item concept.

BTW: I started using asyncItems(), observable() and column(). Very useful additions. 👍

@edvin
Copy link
Owner Author

edvin commented Mar 29, 2016

Thanks @hastebrot - Hmm.. I guess we could put TreeView in ItemControls.kt anyway, after all it does operate on instances of TreeItem<T>?

@hastebrot
Copy link
Collaborator

Ahh, yes, I forgot. I only had my eyes on root. TreeView contains items, too. Right.

@edvin
Copy link
Owner Author

edvin commented Mar 29, 2016

It looks like we would need to keep ´Nodes.kt` for general Node extensions, like this stuff:

fun Node.hasClass(className: String) = styleClass.contains(className)
fun Node.addClass(className: String) = styleClass.add(className)
fun Node.removeClass(className: String) = styleClass.remove(className)
fun Node.toggleClass(className: String, predicate: Boolean) = if (predicate) addClass(className) else removeClass(className)

@edvin
Copy link
Owner Author

edvin commented Mar 29, 2016

Just spotted a potential duplication bug in toggleClass while looking at the code above, commited a fix :)

@edvin
Copy link
Owner Author

edvin commented Apr 4, 2016

Layouts will go into Layouts.kt as well :)

@edvin
Copy link
Owner Author

edvin commented Apr 8, 2016

I will hold off on this until #76 is resolved.

@edvin edvin changed the title Consider restructuring of Builders.kt and Nodes.kt Restructure Builders.kt and Nodes.kt into Controls.kt, ItemControls.kt and Layouts.kt Apr 13, 2016
@edvin edvin changed the title Restructure Builders.kt and Nodes.kt into Controls.kt, ItemControls.kt and Layouts.kt Restructure Builders.kt and Nodes.kt into Controls.kt, ItemControls.kt and Layouts.kt Apr 13, 2016
@ruckustboom ruckustboom mentioned this issue Apr 20, 2016
@edvin
Copy link
Owner Author

edvin commented May 3, 2016

There are some questions that pop up while I restructure. For example, where does SplitPane belong? It's kind of a Layout type component, but it actually extends Control. Any suggestions?

@edvin
Copy link
Owner Author

edvin commented May 3, 2016

Either all the panes should go in Layout or we need another file. Some controls work with data, other work with nodes. That should probably be the split. Do all the node-controls fit in Layout? I don't know.. What about TabPane for example?

edvin pushed a commit that referenced this issue May 3, 2016
@edvin
Copy link
Owner Author

edvin commented May 3, 2016

I've commited an initial version, but I need help to verify that stuff is in the right files. Please take a look in the new files and give feedback :)

@thomasnield
Copy link
Collaborator

I'll take a look today. Hammering through a few things at work first.

@edvin
Copy link
Owner Author

edvin commented May 3, 2016

Thank you Thomas :))

@ruckustboom
Copy link
Collaborator

I'll take a look if I get the chance, but I've got a busy day ahead of me too.

@edvin
Copy link
Owner Author

edvin commented May 4, 2016

No worries guys, have a look when you've got the time. I'll leave the ticket open until you've had a chance to look it over though.

@ruckustboom
Copy link
Collaborator

Is there a particular strategy to how the functions are ordered within a file?

@ruckustboom
Copy link
Collaborator

Also I noticed a few possible things:

fun Pane.textflow is in Controls.kt, but it is technically a Pane, though it is used more like a control. Are the files organized by use instead of technical difference?

The same goes for fun Pane.text, which is actually a Shape instead of a Control, and fun Pane.Imageview which is just a Node.

ToggleGroup isn't a Control (or even a Node for that matter) and isn't used like one in TornadoFX. Should it go in Controls.kt or somewhere else (FX.kt, Lib.kt)?

@edvin
Copy link
Owner Author

edvin commented May 4, 2016

No, there is currently no strategy to how the functions are ordered within the file, but it would be nice with some groups of stuff ordered together. I just can't seem to find a good way to group them.

The other issues you mention are exactly the ones I encountered when I did the initial restructuring. We could focus on wether the controls structure other nodes (put them in Layouts.kt) or if they structure data (put them in Controls.kt or ItemControls.kt).

ToggleGroup is used for a particular Control, so it probably makes sense to put it next to the control it is used with.

Does this make sense to you guys? I'm honestly a little lost and open to suggestions :)

@ruckustboom
Copy link
Collaborator

It makes good enough sense to me. I imaging there isn't a really good/intuitive way to separate everything. Especially as functionality grows.

@edvin
Copy link
Owner Author

edvin commented May 4, 2016

Well, actually - as functionality grows, maybe a better way to structure this will present itself. There might become a need for a more fine grained structure. For now however, these three files are quite manageable. If anyone wants to group functions or add some comments that very welcome!

I'll close this ticket as the main objective was achieved. @ronsmits will probably have some comments with regards to issue #100 :)

@edvin edvin closed this as completed May 4, 2016
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

No branches or pull requests

4 participants