Tornadofx: CSS Splitting Bug

[ruckustboom]

I'm not sure why I didn't notice this before, but there is a bug in the way CSS comma separated selectors are split. For example:

s("A, B") {
    s("C, D") {
        textFill = c("green")
    }
}

should produce:

A C, B C, A D, B D {
    -fx-text-fill: rgba(0, 128, 0, 1);
}

but instead produces:

AC, B C, AD, B D {
    -fx-text-fill: rgba(0, 128, 0, 1);
}

(notice the lack of space after A both times).

This has to do with the fact that the , immediately follows A when stored. A naive solution would be to just add a space after splitting the string, but that would break in the following case:

s("A, B") {
    +s("C, D") {
        textFill = c("green")
    }
}

where you would get:

A C, BC, A D, BD {
    -fx-text-fill: rgba(0, 128, 0, 1);
}

This could be fixed by doing checks on whether the value of current ends in a space, but I'm not sure how many more weird bugs are going to pop up in these kinds of corner cases, each of which requiring its own hack to get around.

[edvin]

Nice catch. Since we still need to support strings for selectors, can you think of a better approach to solve this and also avoid similar bugs in the future?

[ruckustboom]

I'm working on a couple ideas, I'll let you know what I come up with.

[ruckustboom]

I've created a structure that should work. I just need to create a method to parse strings now...

[edvin]

Nice! We could probably do most of the heavy lifting with a regex. Let me know if you get stuck or if I should give it a go.

[ruckustboom]

I've got a branch up (fearture/css2) that has the basis of the system I've designed. It doesn't parse strings yet (that's next on my list), but it has quite a few breaking changes. The upside is the syntax is even cleaner.

For example instead of doing s(label) { ... ), you can just do label { ... }
(and even label or button { ... } instead of s(label, button) { ... }.

I don't have string parsing yet, but when I finish it, you'll be able to do ".box > .label" { ... } (notice no s() again.

There is still an s() function in there, but all it does is call the associated non-s-wrapped function, so we could just do away with it.

[edvin]

Oh my, that's fantastic!! The s is a bit ugly, this will be much easier on the eyes :) Great work!

[edvin]

We should probably just kill the s once and for all.

[ruckustboom]

I've updated StylesheetTests to use the new version in that branch if you want to look at a couple examples.

[edvin]

Great, checking now!

[thomasnield]

What?!?! Okay, I'll live with that if it is cleaner...

Haha you know why the world belongs to procrastinators? Because things change at the last minute. I'll update the documentation if we all agree this is better.

The s() did feel weird to me at first, and then I got used to it, and then attached to it. Weird how developers are trained to be counterintuitive.

[edvin]

Haha, the new syntax was different indeed. I can't really "see" what the selectors mean as easily now. Need to understand it a bit better first.

[ruckustboom]

It is experimental, so if we need to change it, I'm fine with that.

[ruckustboom]

Part of it may just be the naming of the functions (not my strongest point, so any help with better names is appreciated :) )

[edvin]

Not really bad, but descendant is hard to write :) hehe.. I though about using the operator function compareTo instead of child, but that won't work I think. Then we could have written vbox > wrapper instead of vbox.child(wrapper).

[edvin]

compareTo must return int, so it's a no go :/

[ruckustboom]

It should even be possible (with less breaking changes) to (mostly) keep the old syntax, but use the new structure (which properly handles Cartesian joins and the like).

[edvin]

What about using infix functions? Maybe it would be even cleaner if we got rid of some parenthesis?

[ruckustboom]

@edvin Yeah, I looked into compareTo as well, but even if it could return something other than Int, we can't control the use of > over <.

The main reason I didn't use infix functions is that I have or as an infix function, and it requires lower precedence than other selector scoping in CSS. If we want to change that, we could use infix functions for scoping.

[ruckustboom]

Also, unaryPlus has a higher precedence than infix functions, so +box child label { ... } wouldn't work properly.

[ruckustboom]

The unaryPlus issue can be bypassed if we wrap the selection in () (e.g. +(box child label) { ... }), but that's only one character difference from s(), so it may be worth it to have a single character function for nested scope refinement (it would clear up the confusion between +selection { ... } refining scope and +mixin adding information).

[ruckustboom]

For the infix thing, we could do something like or(a child b child c, x child y child z) { ... } (again, a different name would probably be better (maybe s() ;) )).

[ruckustboom]

Possible better names:

• descendant => contains
• adjacent => next
• refine => and

[ruckustboom]

I just pushed up some naming fixes (includes both the changes I suggested above and some leftover tests from when I was creating the thing)

[edvin]

Oh, now we're talking. That feels much better!

[ruckustboom]

I'm having an issue with infix functions, but I think I've almost got it figured out.

[edvin]

OK. If you can make this work I think we have a winner syntax-wise. Very easy to read and reason about IMO.

[ruckustboom]

Man, order of operations is killing me here

[edvin]

I believe you :) hehe.. quite rough to get right, but man it will be nice to use :)

[ruckustboom]

Alright, I've pushed up the changes. Tell me what you think.

[edvin]

Checking!

[ruckustboom]

I'm using s() in place of or, so a better name would be nice.

[edvin]

Aha. OK, I see you have the selector vbox child wrapper contains label. How would you also make that selector catch for example just label as well?

[ruckustboom]

What do you mean? Like in the multiValue() or singleValue() tests?

[edvin]

Not that it makes any sense, but let's say I want to express:

.vbox > .wrapper .label, .label

How would you write that now?

[ruckustboom]

Assuming all those classes are defined (I haven't gotten to string parsing yet, though I've got it more or less figured out): s(vbox child wrapper contains label, label)

[edvin]

Perfect, that's what I though (and hoped for). I like this a lot!

[ruckustboom]

I've just added a test for it and pushed it up

[ruckustboom]

So I have two questions:

• What should I use for multi-select (to replace s() as it isn't super descriptive)? multi()? Or is that just confusing with multi used for properties?
• Should we replace the CssSelection.unaryPlus() used for nested selection modification with a function to avoid confusion with mixins?

[edvin]

• If s is optional for single-select and required for multi-select I think it's fine. If you want consitency, then just always use s.
• That might be a good idea. What would be a good function name? :)

[ruckustboom]

We use add for the infix version. Should we use it here, or is add not very clear? Should we use a different function name for both? join?

[edvin]

Hmm... I don't think join is good. Add might be a bit general, but it's better IMO.

[ruckustboom]

I'll go with `add for now then. If you come up with something better, let me know. In the mean time I'll work on parsing strings.

[ruckustboom]

I've pushed up the change with add(). It will accept anything s() will select, so you don't have to type add(s(...))

[edvin]

Very nice! I'll play with it a little tomorrow to see if it feels as good as it looks. Is there still a style2 function in there btw?

A bit unfortunate to introduce more breaking changes, but it's better to do it now. @thomasnield What do you think?

[ruckustboom]

There was a style2. It was a remnant from when I was testing the old and new CSS side by side.

I just pushed up the string parsing. I also added a test to verify that

".label     >lab   #la:l          ,.label,#fred" {
    textFill = Color.BLANCHEDALMOND
    add(":hover") {
        backgroundColor += Color.CHARTREUSE
    }
}

renders to

.label > lab #la:l, .label, #fred {
    -fx-text-fill: rgba(255, 235, 205, 1);
}

.label > lab #la:l:hover, .label:hover, #fred:hover {
    -fx-background-color: rgba(127, 255, 0, 1);
}

[ruckustboom]

@edvin The String.toRuleSet() function in CSS.kt may not be very well optimized. If you get the chance, would you check it out to see if it can be done better?

[ruckustboom]

@thomasnield If it makes you feel any better, almost every thing in the guide is still valid. I tried to keep it code compatible as much as possible. I believe the only thing in there that isn't true any more is that +s(...) { ... } is now add(...) { ... }.

The real change is that anywhere multiple selectors aren't required, s() is now optional (but being optional your examples should still work).

[edvin]

This is fantastic, @t-boom - I'm impressed! OK, will have a look at toRuleSet tonight, and then we can merge the branch :)

[ruckustboom]

I had inspiration strike while I was sleeping and cleaned it up a bit. Let me know if you think it could still be done better.

[edvin]

That looks pretty clean to me. Should we merge?

[ruckustboom]

I'm not sure. How would this affect the release? Would it just be a 1.5.1 despite the breaking changes?

[edvin]

Yeah, it is a bit unfortunate, but it will not affect many people at this stage. The old syntax is still valid for the most part, right?

[ruckustboom]

I believe the only real exception is the change from +s() to add(), but I haven't done a thorough test to make sure.

Due to the fact that the cssid, cssclass, and csselement delegates now return a new type (CssRule), there could be problems there if people explicitly defined the type, but if they didn't it should be okay.

[ruckustboom]

There may also be quirks depending on how string selectors were used before, but I'm not sure how common those will be.

[edvin]

We needed to clean this up, so this has to be done. I have some apps I can test a lot of the old syntax against to see how it fares.

[ruckustboom]

Okay, cool

[ruckustboom]

@edvin Did you get the chance to test this against any of your apps?

[ruckustboom]

@edvin Don't merge this yet It may not be as future proof as I'd hoped.

There are issues with how the whole CSS thing is designed (the root concept, so it affect both the old and new versions). One in particular I just came across is the inability to use custom properties. This is a big deal, and I can't think of a good way to overcome it. JavaFX CSS lets you define variables in the CSS and use them later. A good example of this is -fx-base defined in modena.css, which sets the base color for the whole whole stylesheet. By overriding this value, you can quickly give your program a global custom color palette. Another place this could be useful is using custom controls (or controls from libraries) that may define their own (overridable) properties.

Currently you can get around this by using properties["-fx-base"] = "red", but I would like to make properties private (or protected or internal) as I think it would make the whole experience cleaner, but I can't think of a good way to allow this behavior. We could let the user use custom implementations of CssSelectionBlock, but I feel that would get ugly fast (and also tedious).

[ruckustboom]

We may just have to add a setProperty(String, Any) function.

[ruckustboom]

I'm thinking we could have some sort of CssProperty<T: Any> that can be created with a cssproperty delegate (like cssclass) and use some reification to do fun <T: Any> setProperty(CssProperty<T>, T). Then the use could do (in the companion object) val base by cssproperty<Color>("-fx-base"), and (in the CssSelectionBlock) setProperty(base, c("red")).

Would that work? (Also, would reification even be needed here? I still have a hard time getting my head around it for some reason. Too many years of type erasure I guess.)

[ruckustboom]

To bad we can't overload =

[ruckustboom]

I added a quick test for custom property support (no reification needed :) ) using the method above. I also added a shortcut method for fun <T> setProperty(CssProperty<T>, T) as inline fun <T> CssProperty<T>.set(T) so you can use base set c("red") instead of setProperty(base, c("red")). What do you guys think about this?

[edvin]

Hm... it's concise, but it's not necessarily easy to understand what that does. What if we used a property delegate to do something like:

base.value = c("red")

I haven't tried to make that work but I think it would be possible.

That would make it easier to understand that you're assigning a different value to the base property. What do you think?

Hm.. can any of those be multi values? Maybe we need two delegates, cssproperty and cssmultiproperty or something..

[ruckustboom]

@edvin That would be a lot better, but I'm not sure how to do that. If you want to take a shot at it when you get the chance that would be awesome.

[edvin]

I will :)

[edvin]

I think I got it working and I updated the splitting test to use the new syntax. I had to introduce a threadlocal scope variable to make it happen, see the companion object of PropertyHolder. Basically, every time a new PropertyHolder is created, the scope changes to that PropertyHolder, so that the value property of a cssproperty can know what map to put the property into. CssProperty now has this value variable:

class CssProperty<T>(val name: String) {
    var value: T get() = scope.get().properties[name] as T; set(value) { scope.get().properties.put(name, value as Any) }
}

If you think it's dirty we can figure something else out :)

[ruckustboom]

There is a problem with that implementation. The value is set for the most recently opened scope, so

label {
    add(":hover") {
        base.value = c("blue")
    }
    base.value = c("red")
}

results in

.label:hover {
    -fx-base: rgba(255, 0, 0, 1)
}

when it should be

.label {
    -fx-base: rgba(255, 0, 0, 1)
}
.label:hover {
    -fx-base: rgba(0, 0, 255, 1)
}

[edvin]

You're right, I though of that just after I posted it yesterday, but never got around to comment on it. I'll find another approach :)

[edvin]

Allright, tonight's crazy attempt: _Change the scope in the constructor of CssSelection_.

1. Save the current scope
2. Set the newly created block as the current scope
3. Run the op: CssSelectionBlock.() against the block
4. Revert the scope to the one fetched in step 1

Implementation:

init {
    val currentScope = PropertyHolder.selectionScope.get()
    PropertyHolder.selectionScope.set(block)
    block.apply(op)
    PropertyHolder.selectionScope.set(currentScope)
}

This test now passes:

    @Test
    fun propertySelectionScope() {
        stylesheet {
            label {
                add(":hover") {
                    base.value = c("blue")
                }
                base.value = c("red")
            }
        } shouldEqual {
            """
            .label {
                -fx-base: rgba(255, 0, 0, 1);
            }
            .label:hover {
                -fx-base: rgba(0, 0, 255, 1);
            }
            """
        }
    }

[ruckustboom]

Looks good to me. Does it work properly with mixins, add(), and multi-values?

[edvin]

I think the scoping should work good now. I'll try with multivalues tonight but now I've gotta run to Jiu Jitsu training :)

[ruckustboom]

Sweet. I've got meetings all day, but I'll see if I can squeeze in some time to look at it.

[ruckustboom]

It doesn't work with mixins as selectionScope is initialized in the CssSelection instead of the PropertyHolder

[ruckustboom]

Multi-values also do not seem to work with the += syntax (I think it has to do with the fact that get is returning null instead of an empty MultiValue on first use), though using the multi function seems to work fine.

[edvin]

OK, I'll try to move it to PropertyHolder. MultiValue += requires some trickery but is doable, I'll have a look :)

[edvin]

Hmm.. Moving it to PropertyHolder might not be that easy, we have to hook into it whenever we process an op block..

[edvin]

But it should be possible to add the same process to the mixin() function..

[edvin]

Yeah, I think we could add a function to prosess an op: CssSelectionBlock.() and change scope before and after. Looking at multivalue now, will try in a sec.

[ruckustboom]

Okay, cool

[edvin]

Allright, I think I fixed the mixin issue by letting the constructor of CssSelectionBlock that the op as parameter, set the scope and then process that op.

I also "fixed" the += for multi values, but since the type of the multivalue is a class type parameter, it is not reified so I cannot actually know that the property should be a MultiValue (now I create one whenever the value is currently null and a get is issued). I'll try to find a better way to solve this now.

[edvin]

I found a way to fix it :) I made the cssproperty() function inline so that T could be reified. Now it's possible to send a boolean parameter to the CssPropertyDelegate, which in turn can send this parameter to PropertyHolder.CssProperty, so that if get is called when the value is null and the property represents a MultiValue, the MultiValue object can be created on the fly :)

[ruckustboom]

Cool :)

[ruckustboom]

I think I found one more possible edge case. If you use a mixin that sets a multivalue, it will clear any previous values (as it uses its own SelectionBlock)

For example, if you do

val mix = mixin {
    multiProp.value += Color.GREEN
}
val ss = stylesheet {
    label {
        multiProp.value += Color.RED
        +mix
        multiProp.value += Color.BLUE
    }
}

you get

.label {
    multiProp: rgba(0, 128, 0, 1), rgba(0, 0, 255, 1);
}

instead of

.label {
    multiProp: rgba(255, 0, 0, 1), rgba(0, 128, 0, 1), rgba(0, 0, 255, 1);
}

[ruckustboom]

This probably needs special handling in the CssSelectionBlock.mix() function if the property is a MultiValue and already exists.

[edvin]

OK. I have to run now, I'll try to take a look later tonight :)

[edvin]

Fixed :) I made a check for an existing multivalue and add to the list instead of overwriting:

mixin.properties.forEach { k, v ->
    if (properties[k] is MultiValue<*>)
        (properties[k] as MultiValue<Any>).addAll(v as MultiValue<Any>)
    else
        properties[k] = v
}

[ruckustboom]

Looks good to me. I think that's covered everything I've found. I'll pound on it more later when I get time, but nothing stands out.

[edvin]

Perfect :))

[ruckustboom]

I'm not seeing anything else

[edvin]

Cool, let's merge :)

[edvin]

Merged. Once again, fantastic work @t-boom :)

[thomasnield]

TL;DR, sorry. Does this finalize the CSS API?

[edvin]

It would seem so. I can't think of anything that isn't future proof, and all the bugs have been squashed :)

[edvin]

Plus, the new syntax where you don't need to wrap a single selection in s() or select() is very nice :)

[thomasnield]

Awesome. Thanks for all your work t-boom. This feature even in prototype has been enormously helpful at work for me.