[Refactorization & Cleanup] Controls.kt #824
Conversation
Applied the Kotlin Coding Conventions (https://kotlinlang.org/docs/reference/coding-conventions.html) Sorted and grouped functions based on functionality. Added some function variations with `ObservableValue<X>` instead of `X`. (Being X a given type) Added some parameters and default values to be consistent. Renamed some parameters to be consistent. Changed signature of some functions to be more generic, ie: `Property<String>` changed to `ObservableValue<String>`. (Should not break existing code) Removed some redundant functions. Made some `value` parameters nullable to be consistent with the existing code. Added support for more classes in `EventTarget.properties`. Changed `TextInputControl.stripNonNumeric` to accept chars instead of strings. Changed some properties keys, should not break existing code.
There was a problem hiding this comment.
I like the shorter lines.
(at first i loved the long lines, as the lambda looked like a body.)
The new way makes it look very compact.
To make it even more compact, I would like to see a break after attach.
CreateSomething().attach{
....
}
Instead of a oneliner.
I don't like the : Unit.
(i personally also dot like the return-type of other functions, but as long as the functions don't have tests, I understand why you need them)
Ps. As you change things (other than the format), you should update the changelog
Co-Authored-By: SackCastellon <[email protected]>
Co-Authored-By: SackCastellon <[email protected]>
|
This looks very good. Is it ready for merge? |
There was a problem hiding this comment.
There are still a couple of things about which I would like to know your opinion @edvin.
I added some // FIXME comments to them, the reason why I didn't change them is that it could really break some existing code.
Another option could be to ignore them by now, leave the comments, and maybe fix them in the future.
| // Button | ||
|
|
||
| fun EventTarget.button( | ||
| text: String = "", // FIXME This parameter is not nullable but in all the other existing functions it is nullable |
There was a problem hiding this comment.
Should this be nullable?
| noinline op: Label.() -> Unit = {} | ||
| ) = label().apply { | ||
| observable: ObservableValue<T>, | ||
| graphicProperty: ObservableValue<Node>? = null, // FIXME Inconsistent with all the other functions. There is no other function with ObservableValue<Node> |
There was a problem hiding this comment.
Should this be graphic: Node like the rest of functions, or should all other functions have graphicProperty: ObservableValue<Node>??
Objectives
Changes
ObservableValue<X>instead ofX. (Being X a given type)Property<String>changed toObservableValue<String>. (Should not break existing code)valueparameters nullable to be consistent with the existing code.EventTarget.properties.TextInputControl.stripNonNumericto accept chars instead of strings.