Tornadofx: Adopt Kotlin 1.1 DSL markers

[voddan]

TornadoFX usability greatly suffers from imprecise receiver resolve of Kotlin 1.0. DSL markers added at Kotlin 1.1 should be able to help with this.

This issue contains the general discussion on how to add DSL markers to TornadoFX

Kotlin documentations on DSL markers

KEEP: Scope control for implicit receivers

[edvin]

The main problem is that the current dsl marker support in Kotlin 1.1 requires us to add an annotation to every receiver class. That's not possible, since we don't control the JavaFX classes.

Subclassing every control we operate on is not a good idea, as that would mean moving all extension functions to our own subclasses. Then our extensions would no longer work on "plain" JavaFX classes, for example those references via FXML.

A possible solution would be to ask JetBrains to add a dsl-marker whitelist of some kind, where we could mention all pertinent JavaFX classes. I don't have any concrete suggestions right now as to how this could be implemented.

[edvin]

Another solution could the to introduce annotation support on type aliases.

[edvin]

I just played a little with dsl marker support in KDBC, because I was convinced it would be a good match there, and easy to retrofit into such a small code base. I was partially right :) It was easy to add a @DslMarker annotation to the Expr class, but I was quickly bit with issues that would pertain to TornadoFX as well.

Inside a builder you would no longer have access to constructor variables or even field memebers. You'd have to write [email protected]() a lot, and that's just plain ugly to look at.

It seems to me that introducing dsl marker in it's current form will get rid of some problems and introduce a completely new set of problems, and resulting in ugly code and workarounds.

I thought that field members at the very least would be in scope inside builders with dsl markers enabled, but when that's not the case this feels a lot less enticing.

[voddan]

I think you are supposed to have scope functions separated from building functions. In plain words that would mean that the classes of the underlying data model are NOT marked as DSL and their properties (build methods) are easily accessible from everywhere. Along side with them you would have a system of scoping functions that help you build tree-like structures. Those scoping functions need rigid DSL-like receiver resolve, so their receivers ARE marked.

Hadi Harriri in his DSL talks uses additional wrappers in his code like RouteHandler here https://youtu.be/GjGQpSFieXA?t=23m47s. In essence he uses composition instead of inheritance, which allows him to build DSLs for external classes. Maybe we can adopt his technique for the case of TornadoFX.

[voddan]

I really like your idea to experiment with a smaller project like KDBC. Let me experiment with it myself a little. I'll push a branch when I have something satisfying.

[edvin]

Perfect, Daniil. I'm very grateful for this initiative!

[voddan]

I have been trying to get my head around KDBC tonight. Oh, my head hurts! You sure do hate documenting public methods...

On a more series note, what part of the KDBC dsl were you adding the markers to? I far as I see, KDBC does not really utilise nested multi-level structures which could me improved by the marking. Could you please describe what you did, or push the experimental branch for reference, or both?

[edvin]

I want to document everything in KDBC before we release, but I've been changing stuff around so much that doing so before the API is stable wouldn't be a good idea :)

I tried creating an ExpressionMarker annotation and added it to to Expr. I've attached a patch where I simply created this marker annotation and "fixed" the code so it runs. You can apply the patch to play with it, but just looking at it should reveal the issues that needs another solution now.

I know very little about the @DslMarker support at this stage, so hopefully this will give you some ideas.
ExpressionMarker.zip

[voddan]

Ok, I'll try working with that.

The problem is that I can't come up with a KDBC example which has nested scopes. All your examples and tests are linear (which is good), so they don't require marking in the first place.

[edvin]

I've been playing with this some more, and I think even though it would improve discoverability, it would impede us even more for locking down the outer scopes. This is actually a very useful "feature". If only there was a way to get the IDE to show what scope a function applies to. I think that would be a better way to deal with this actually. In it's current form, DslMaker is just not what we need. Do you agree or should we continue to investigate this? I would rather use some time to see if we can deal with this in the plugin.

[voddan]

Have you been playing with KDBC or TornadoFX? Because I haven't managed to adopt DSL marking for KDBC, mostly for reasons stated earlier. But I still have hope for it in TornadoFX

[voddan]

I like your idea of scope highlighting. It sounds like something useful in general, not only for TornadoFX or DSL markings. Maybe it should be implemented in IJ itself, not TornadoFX plugin

[edvin]

Yeah, it should probably go into IDEA core. In the mean time, to have the discoverability you can of course just use this. and hit Ctrl+Space. Then everything will be on point. It would be cool if some combination of Ctrl+Space could operate solely on this.

[edvin]

DSL markers just won't help us, so I'll close this issue, but we should consider adding a request into IDEA to get the scope highlighting feature.

[voddan]

The main problem is that the current dsl marker support in Kotlin 1.1 requires us to add an annotation to every receiver class. That's not possible, since we don't control the JavaFX classes.

Looks like it what was needed, but I haven't tested it yet

[edvin]

That sounds more like it! Will you give it a shot or should I?

[voddan]

I think I'll have some time late this weekend

[edvin]

Great!

[voddan]

One thing is for sure - it works :)

I added one @DSL annotation and added it to some lambda receivers. All tests kept working, as well as the app I was testing.

Adding the marker prevented calling/autocompleting prefWidth from imageview {} by mistake. That is one example that I had in my app, not sure how common this problem is.

[voddan]

Meat a roadblock: comment
Not clear if this is a bug or a feature. Waiting for JB's reply.

[voddan]

Ok, according to @orangy, this is not a bug:

Compiler selects most local receiver and then checks DSL marker
DSL markers do not change how a call is resolved, it issues an error when such a call violate dsl rules

[voddan]

Question: does adding the DSL markers have not to affect existing code? Even if it makes it clearer with explicit receivers?

[voddan]

@edvin BTW it is time to reopen the issue

[edvin]

Great work so far! The problem is that it would break a lot of existing code so we need to figure out how to deal with this, and if this is the route we want to go. We could release one version with and one without, do this only for 2.0 together with Java 9 support etc.

[voddan]

One possible path is to use several dsl markers instead of one. For that we need somehow categorize the builder functions of TornadoFX. Do you have any ideas?

[edvin]

I'm not following. What would using several DSL markers solve?

[voddan]

OK, let me explain:

Now, when I marked all non-generic lambda receivers with a DSL marker @DSL, I get non-compiling code like that:

override val root = stackpane {
    setPrefSize(400.0, 150.0)
    hbox {
        button("Don't click me") {
            action {
                builderWindow("What do you want?", stageStyle = UNDECORATED, owner = primaryStage) {
                    vbox(10) {
                        hbox(10) {
                            button("Tell them you clicked").action {
                                [email protected] = "It's not dangerous to click the button :)"
                                close()  // ERROR: DSL markers
                            }
                            button("Close app").action {
                                Platform.exit()
                            }
                            button("Cancel").action {
                                close()  // ERROR: DSL markers
                            }
                        }
                    }
                }
            }
        }
    }
}

Here close() methods are resolved to builderWindow{}, but are blocked because both builderWindow and vbox have the same DSL marker. But that problem would disappear if we marked those two methods with different DSL markers.

For example, I created a marker for tornadofx.Component children @DslComponet, and marked builderWindow{} with it instead of @DSL. That resolved the issue.

The question is how to categorise DSL receivers in a way that will prevent any logical conflicts. My current idea is based on class hierarchies. For example, HBox and VBox are both inherit from Pane, so let's mark them with the same marker. That way some methods like setVgrow() might become unavailable in some places, but IMO it is for the better. The rest of the methods won't be affected.

[edvin]

OK, I think I understand. But what happens if you nest two hboxes in each other? Would the functions available on hbox still resolve to the inner most target without issues?

[edvin]

Ah, yes - it seems this would be the way to go. But what about migration path - as I mentioned, we will still break a lot of existing code, and to fix it you need to litter your code with [email protected]. This looks like a syntax regression to me. If we do this, we should also try to find a nicer way to deal with accessing the parent properties, because you need that all the time.

[voddan]

What do you mean by parent properties? Properties of a class's parent?

[edvin]

No, I mean parent as in the builder hierarchy parent.

hbox {
    hbox {
        // Is it OK to call functions on hbox here?
    }
}

[voddan]

This case must be ok. Shadowing the parent is the essence of DSL building, right? Such code will never even brake.

[edvin]

Sweet. One thing that I really love about the builders right now, is that you very seldom (or ever) need to create instance variables for UI nodes. This will probably become more common if we close down the possibility of accessing properties from a parent without being explicit about it. You could also run into an issue where you can't use this@hbox because it is ambiguous. These are probably all good things ™️ though, as you would have to be more explicit.

[ruckustboom]

The places where this@hbox is ambiguous already cannot be accessed without creating instance variables, correct?

hbox {
    hbox {
        // Is there any way to access properties of the
        // outer hbox here without an instance variable?
    }
}

[voddan]

@t-boom you can label hboxs with different labels:

hbox a@{
    hbox b@{
        this@a
        this@b
    }
}

[edvin]

I think labels might be the tool of choice for these situations. It's more light weight than creating variables and serves the same purpose. OK, so it looks like we have the means to implement. Now we need to decide how to introduce this. Version 2.0? Compile two versions for every release and deprecate the old way after a while?

[edvin]

About the labels.. parent.parent also works inside the builders, but you loose the type. If you just need access to a Parent it is enough though.

[ruckustboom]

@voddan Indeed. My question was really whether the addition of DSL annotations would add any this@hbox type ambiguities that don't already exist.

[edvin]

I have an idea here. We could add marker annotations to the right functions, and just not let those annotations be marked with @DslMarker. Everything would work as normal, but by changing those annotations we could easily create a separate build that enforces the dsl marker rules. This would be an easy way to experiment with dsl markers and actually doing separate releases with them so people can test them out. What do you guys think?

[edvin]

@voddan Do you have any plans to look at this in the (nearish) future? I think I will not do anything with this for now myself, as I see as many issues as this provides solutions :) Unless you plan to work more on it I think we can close the issue for now, concluding that while it is doable it introduces other issues and api changes that might not be desirable.

[voddan]

Ye, let's close it till we know how to do it properly.

[edvin]

Great :)