Refactorization and cleanup #815
Conversation
…g/docs/reference/coding-conventions.html) Added return type to all public functions to ensure API stability. Sorted and grouped functions based on functionality. Added missing function `TreeTableView<S>.column(String, KClass<T>, TreeTableColumn<S, T>.() -> Unit)` to be consistent with existing `column` functions. Replaced some lambdas with function references. Added missing `op` parameter to multiple `column` functions to be consistent. Removed unnecessary @JvmName("columnForObservableProperty") annotation. Removed `inline` and `noinline` modifiers from multiple functions where it was not necessary. Removed unnecessary cast from `nestedColumn` functions.
Added return type to all public functions to ensure API stability. Sorted and grouped functions based on functionality. Simplified some if statements. Added `inline` modifier and specified type variance in some collections utility functions. `insets(Number)` now calls the _proper_ constructor and its argument is nullable, so when omitted it always returns `Insets.EMPTY`. Changed `TextInputControl.stripNonNumeric` signature so `allowedChars` param is a list of Chars instead of a list of Strings.
Sorted and grouped functions based on functionality.
Added return type to all public functions to ensure API stability. Replaced `put` function call with overloaded operator.
Removed explicit type from `property` functions that caused errors. Reassignment of singleAssign now throws a RuntimeException. Removed redundant `text()` function. Fixed bug in `label()` function where graphic property was to correctly bound.
…ctorization-and-cleanup
Added return type to all public functions to ensure API stability. Renamed test class to match file name.
Added return type to all public functions to ensure API stability. Added changes to changelog. Refactored `Latch.release()` to be more idiomatic.
Added return type to all public functions to ensure API stability. Sorted and grouped functions based on functionality. Improved some tests.
Added return type to all public functions to ensure API stability. Sorted and grouped functions based on functionality. Improved some tests. Added languaje injection comments to some json code.
Added return type to ensure API stability. Sorted and grouped functions.
…ctorization-and-cleanup
For some SimpleObjectProperty replaced the function with a property. Cleanup.
Renamed function `handleFilterChanged_` to `handleFilterChanged`. Renamed property `autoCompleteFilter_` to `autoCompleteFilter`. Changed some parameter names.
Sorted and grouped functions. Changed default value of `viewTitle` in `UIComponent` constructor tu `null`. Removed redundant `apply` wrapper in `Component.runAsync()` functions. Fixed default value in `ConfigProperties.boolean()` was `false` instead of `null`.
…re()` Added return types. Sorted functioons.
Added return type to all public functions to ensure API stability. Sorted and grouped functions based on functionality. Refactored properties so its type is more abstract. Renamed `AbstractField.labelProperty` to `AbstractField.textProperty`. Fixed some `ReplaceWith` expressions. Added optional `op` parameter to `Menu.separator()` function. Fixed `StackPane.creatable` getter where `savable` was returned instead. Added private ReadOnlyBooleanWrapper for `valid` property in Validation.kt, to avoid casting BooleanExpression to BooleanProperty. Removed redundant checks in `ViewModel.bind()` method.
…nd-cleanup Also added return types and sorted functions.
There was a problem hiding this comment.
IMPORTANT: I refactored this repo a lot, but I am not the one that makes decisions about them.
I have somebody to battle against about the formatting ;-)
I did look untill controls.
First, great work!
I haven't really commented on things I agree with.
About the things I didn't agree with:
I don't like to add the returntype everywhere, because:
- at some places it is not neccesary. this information are asking for attention, without saying anything:
fun toString() : String = "" //I think everyone knows that toString returns a String.
fan a() = TheClass().apply{ //returns TheClass. - at some places it is not the base function, so changing the returntype of the base-function should change the methods of functions calling that function:
fun a1(a : PAR) : String = a() - breaks clearify the code, nesting decreases. (this is on multiple occasions.
And some more things.
Oh, and for the next time, change this to multiple PRs.
Now it's a bit an all or nothing PR.
(this is what I found frustrating when i did the PRs as well, as you see a lot of things, but can only change one kind of refactoring at a time).
src/main/java/tornadofx/Animation.kt
Outdated
|
|
||
| fun sequentialTransition(play: Boolean = true, op: (SequentialTransition.() -> Unit)) = SequentialTransition().apply { | ||
| op(this) | ||
| fun sequentialTransition(play: Boolean = true, op: SequentialTransition.() -> Unit): SequentialTransition = SequentialTransition().apply { |
There was a problem hiding this comment.
I don't like this a lot, as it doesn't add anything and makes the header harder to read, due to it's length. That's the reason why I placed the apply and also in the header.
There was a problem hiding this comment.
How about something like:
fun sequentialTransition(play: Boolean = true, op: SequentialTransition.() -> Unit): SequentialTransition =
SequentialTransition().apply(op).also { if (play) it.play() }| reversed: Boolean = false, | ||
| play: Boolean = true, | ||
| op: RotateTransition.() -> Unit = {} | ||
| ): RotateTransition = root.rotate(time, angle, easing, reversed, play, op) |
There was a problem hiding this comment.
Don't pro or against, so just a general remark: With specifying the return-type, the API can become clearer, but it also means thhat you can't change the implementation by changing the base function.
| @@ -350,16 +363,17 @@ fun Iterable<Animation>.playParallel( | |||
| * @param op Modify the animation after it is created | |||
| * @return A SequentialTransition | |||
| */ | |||
| fun Animation.then(vararg animation: Animation, op: SequentialTransition.() -> Unit = {}) = when { | |||
| fun Animation.then(vararg animation: Animation, op: SequentialTransition.() -> Unit = {}): SequentialTransition = when { | |||
There was a problem hiding this comment.
I'm not sure if it adds something. In intellij the return type is automatically added in the popup and when you look at the functionimplementation, I think it's immediately clear what it returns
src/main/java/tornadofx/Animation.kt
Outdated
| keyframe(duration) { | ||
| keyvalue(writableValue, endValue, interpolator) | ||
| } | ||
| keyframe(duration) { keyvalue(writableValue, endValue, interpolator) } |
There was a problem hiding this comment.
I personnally like the multiline-way more.
Do note that with this change a crash at line 501 can point to two things.
There was a problem hiding this comment.
That makes sense, it can make debugging easier
| @@ -579,7 +620,7 @@ abstract class ViewTransition { | |||
| * @param replacement The node that will be in the scenegraph after the transition | |||
| * @return The [StackPane] that will be in the scenegraph during the transition | |||
| */ | |||
| open fun stack(current: Node, replacement: Node) = StackPane(replacement, current) | |||
| open fun stack(current: Node, replacement: Node): StackPane = StackPane(replacement, current) | |||
There was a problem hiding this comment.
I think StackPane overhere adds nothing, it only makes the implementation longer.
| operator fun <T : Enum<T>> Number.plus(value: Dimension<T>) = Dimension(this.toDouble() + value.value, value.units) | ||
| operator fun <T : Enum<T>> Number.minus(value: Dimension<T>) = Dimension(this.toDouble() - value.value, value.units) | ||
| operator fun <T : Enum<T>> Number.times(value: Dimension<T>) = Dimension(this.toDouble() * value.value, value.units) | ||
| operator fun <T : Enum<T>> Number.plus(value: Dimension<T>): Dimension<T> = value + this |
| fun <T> MutableList<T>.move(item: T, newIndex: Int) { | ||
| check(newIndex in 0 until size) | ||
| check(newIndex in indices) { "Invalid index $newIndex for MutableList of size $size" } |
There was a problem hiding this comment.
I think we should create a function for this Collection<T>.checkValidIndex()
| internal val targetRef: WeakReference<MutableList<TargetType>> = WeakReference(targetList) | ||
| internal val sourceToTarget = HashMap<SourceType, TargetType>() | ||
| internal val sourceToTarget = HashMap<Map.Entry<SourceTypeKey, SourceTypeValue>, TargetType>() |
There was a problem hiding this comment.
should be changed to kotlin-inline call
| where InjectableType : Component, InjectableType : ScopedInstance = task { invoke(find(scope)) }.apply { ui(doOnUi) } | ||
| inline fun <reified InjectableType, reified ReturnType> KFunction1<InjectableType, ReturnType>.runAsync( | ||
| noinline doOnUi: (ReturnType) -> Unit = {} | ||
| ): Task<ReturnType> where InjectableType : Component, |
There was a problem hiding this comment.
agreed that it should be broken up. I want to see another brake before or after the =
src/main/java/tornadofx/Component.kt
Outdated
| fun disableClose() { | ||
| properties["tornadofx.closeable"] = SimpleBooleanProperty(false) | ||
| } | ||
| val accelerators: MutableMap<KeyCombination, () -> Unit> = HashMap() |
There was a problem hiding this comment.
should be replaced with mutableMapOf
|
One of the main reasons why I think it is necessary to specify the return type is to prevent 2. About 1. I also thought it could be omitted but, just tried to follow de convention for libraries, maybe @edvin has some comment on this. Finally, about splitting it in multiple PRs, I considered it but I thought it may end up being a mess because there were no rules to follow. At least If this PR doesn't get merged, I hope, we'll get some clear guidelines to follow when refactoring the code or when creating future PRs. |
All collections are generalized to its read-only/mutable interface. Removed some explicit type arguments. Property `Fieldset.labelPositionProperty` is now final. Fixed unused parameter in deprecated function `Node.replaceWith()`.
|
About 1. You are correct!
The reason is that if the content of a function changes it's content, the body has to be treated differently, without good reason: You don't (or actually you do ;-) )return anything. I made last year a start to this guide: |
|
Another issue, wich edvin should give acceptence of course, is that we can use subpackages in the project, but add @file:Suppress("PackageDirectoryMismatch")
package tornadofxto the top of the file |
|
I don't think we should introduce packages for now. Many extension functions will become less discoverable if they are not in the already imported. That's why the plugin specifically imports |
|
It would be true if we don't use @file:Suppress("PackageDirectoryMismatch")
package tornadofxAccording to the link I provided, the package-structure doesn't need to match the folder-structure. |
|
After talking with @edvin I have decided to close this PR split it into different ones, that way it will be easier to review them. |
During the last month, in my free time, I have been refactoring and cleaning up most of the TornadoFX's code, with a couple of objectives in mind:
While refactoring, I tried to keep the signatures of the functions untouched, but in some cases, it was necessary to modify them. So it is very likely that some existing code will break.
Moreover, I came across some pieces of code where I was not able to choose a return type without having to refactor a lot of code, besides it would have broken a lot of existing code. In those cases, I have been adding
// TODOand// FIXMEcomments, so you can review them and decide what to do, which means this is not ready to be merged, for now.Note: I added some tests but one of them is failing and I don't really know why.
Finally, I think that a
CONTRIBUTINGfile could be created with some contribution guidelines to ensure consistent code in the future.Sincerely, @SackCastellon