edvin / tornadofx

Lightweight JavaFX Framework for Kotlin
Apache License 2.0
3.68k stars 271 forks source link

TreeView error when using autocommit with extractors #1208

Open SKeeneCode opened 4 years ago

SKeeneCode commented 4 years ago

I'm hitting an error that is simply beyond my ability to debug, although I have narrowed it down and created a minimal example that reproduces the problem. Apologies for the length of the minimal example (150 lines), I can't get it much shorter. I am praying someone smarter and more educated than me can help me out.

To reproduce the the 'bug' (or my error), expand the tree, click on any tree item that isn't a group, then click on a group and try and edit it below. It throws the error:

SEVERE: Uncaught error java.lang.IllegalArgumentException: Parameter specified as non-null is null: method com.sak33.wiki.app.MyItemModel.setGroup, parameter <set-?> at com.sak33.wiki.app.MyItemModel.setGroup(MinimalExample.kt) at com.sak33.wiki.app.MyItemViewModel$group$1.set(MinimalExample.kt:29) at tornadofx.PropertiesKt$observable$1.set(Properties.kt:68) (and so on...)

It will not throw this error if you edit the groups before clicking anything else, although it will bug again if you then click a nongroup tree item and try to edit another group.

Removing autocommit = true from MyGroupViewModel will prevent the bug, but of course changes are not comitted and not reflected in the treeview.

Removing autocommit = true from MyItemViewModel will prevent the bug, but then when the user changed the group of an item, it will not be reflected in the treeview.

This only started happening when I started using extractors to fire update events for the treeview to organize itself. Something about the autocommit = true in MyItemViewModel and MyGroupViewModel are conflicting. My guess in the dark is it must be something to do with the update events that are fired from using extractors, because before I had no extractors and was using a very ugly set of listeners for each property and did not have this problem.

SchweinchenFuntik commented 4 years ago

java.lang.IllegalArgumentException: Parameter specified as non-null is null: method - you apply null somewhere where notnull is expected

look at these code fragments (I think the problem may be in them):

if (it?.value is MyGroupModel) { // MyItemModel
    item = it.value as MyGroupModel // casting type is bad
}

itemViewModel.rebindOnChange(this.selectionModel.selectedItemProperty()) - binds your properties when nothing is selected there null, and you declared the model as notnull

SKeeneCode commented 4 years ago

So I fixed it by fluke but I don't understand how it fixed it:

If I remove var group: MyGroupModel by groupProperty from MyItemModel and in MyItemViewModel I bind directly to the property with val group = bind(MyItemModel::groupProperty, autocommit = true)

And change any reference from itemViewModel.group to itemViewModel.groupProperty.value

It somehow fixes the problem, not sure why...


I have a second issue that again is probably something easy but I don't understand.

The SortedFilteredList I was using in the populate method was causing me all kinds of weird bugs, so I've tried to replace it with a Sorted(FilteredList()) like so:

SortedList(FilteredList(itemList) { it.groupProperty.value == value}, Comparator.comparing<MyItemModel, String> { it.nameProperty.value.decapitalize() })

The problem is this is not responding to change events and updating the tree view! Once again I'm at a loss >.<

SchweinchenFuntik commented 4 years ago

probably because your tree is not associated with a list, these are different structures. I already answered a similar question, maybe yours. You need to give ObservableList to populate, also save it a link to it, and listen to the changes in your reference list, and change the secondary tree-like lists, or redraw the tree. It’s also worth going to Slack instead of chatting in git

SKeeneCode commented 4 years ago

It’s also worth going to Slack instead of chatting in git

Sure. I haven't used Slack before but send me an invite link and I'll jump in. I think there's some things I need clarification on.

SchweinchenFuntik commented 4 years ago

https://kotlinlang.slack.com/messages/tornadofx/

SKeeneCode commented 4 years ago

https://kotlinlang.slack.com/messages/tornadofx/

Eh, sorry for being stupid, but I don't understand how I can actually join?

SchweinchenFuntik commented 4 years ago

I can invite, I need mail and your name (how to sign you)

ruckustboom commented 4 years ago

You can get an invite to the Kotlin Slack here: https://surveys.jetbrains.com/s3/kotlin-slack-sign-up. From there, you can join the TornadoFX channel.

deggers commented 4 years ago

Wow @SKeeneCode your example solved a problem i had for days! I wasnt able to figure out how to do a prober binding between my tree and its value .. I did some refactoring because you missed some nice extension-function tornadoFX provides. Furthermore your first mentioned error you could avoid by having a validation check before commiting any ill-formed values to your model i think. If you want i will try to make a gist with your code refactored - because the ItemViewModels also do not need to get a Model passed - i think this makes the code all in all cleaner.

Thank you again very much for showing your code - it helped me a lot!

SKeeneCode commented 4 years ago

Hey @deggers

I did some refactoring because you missed some nice extension-function tornadoFX provides.

Would love to see what ones I missed!

Furthermore your first mentioned error you could avoid by having a validation check before commiting any ill-formed values to your model i think.

In the end I simply removed the property delegates var name: String by nameProperty etc. I simply couldn't find where and why I was getting a null so I accepted this refactor instead.

If you want i will try to make a gist with your code refactored

This would be great, would be nice to see your improvements.

because the ItemViewModels also do not need to get a Model passed - i think this makes the code all in all cleaner.

This is true, I chose to put in the optional parameter because I am often reading in data from a file and constructing the model object from it. Then I can simply use the view model constructor to supply it with my newly created model.

deggers commented 4 years ago

Hey @SKeeneCode

Would love to see what ones I missed!

Sure. I am not sure if this behaves like you want it to behave - but you can edit the group now anytime, or i didn't test it properly enough - let me know what you think :) Gist

Something missing would be adding a validator to the textfield to make sure no incorrect data is autocommited Guide

This is true, I chose to put in the optional parameter because I am often reading in data from a file and constructing the model object from it. Then I can simply use the view model constructor to supply it with my newly created model.

Sure, makes totally sense! Did not had this in mind :-)