Abusing #userInterfaceTheme

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Abusing #userInterfaceTheme

Stéphane Rollandin
Hello,


I would like to offer some technical feedback here, please do not take
it as a complaint directed against anyone's good work. In particular,
thanks again to all involved in designing the theming system. Only,
there are some worrisome issues in my opinion. Here we go:


1) The code below raises a DNU for #hardShadowOffset in SqueakTheme,
because #userInterfaceTheme is not used as intented (after
#softShadowOffset is sent, m gets popped out of the theme scope and is
not there anymore when #hardShadowOffset is sent):

        | m uit |

        m := Morph new.
        uit := m userInterfaceTheme.
        {uit softShadowOffset . uit hardShadowOffset}


However, for someone not aware of the intricacies of UserInterfaceTheme,
the code seems fine. In my opinion this is not a good state of affair.

Maybe the error raised here could be more explicit and in line with what
is really happening in the DNU, such as "There is no object in scope at
the moment".


2) Now consider the following code:

        | m1 uit1 m2 uit2 |

        m2 := BalloonMorph new.
        uit2 := m2 userInterfaceTheme .

        m1 := Morph new.
        uit1 := m1 userInterfaceTheme.

        {uit1 color . uit1 color}


When I evaluate it I get {nil . Color lightYellow1}

This is because while the first "uit1 color" does redirect to m1, the
second "uit1 color" redirects to m2 now that m1 has been popped out of
scope. This time there is no error raised, but the result is very
misleading.


3) For the record let me recall that doing

    100 timesRepeat: [Morph new userInterfaceTheme]

results in having the current theme referencing 100 morphs that will
never be automatically garbage-collected.



Overall the three above points make me think that the method
#userInterfaceTheme is much too easy to unknowingly abuse of, and when
this happens undesirable behaviors can be rather difficult to make sense of.

At the moment the method comment is:
"Call this to conveniently access properties from the current user
interface theme."

I believe it needs some explicit warnings about how not to use it.


Just my 2 cents...


Stef

Reply | Threaded
Open this post in threaded view
|

Re: Abusing #userInterfaceTheme

marcel.taeumel
Hi Stef,

thanks for the examples and elaborated concerns. :) We should follow your suggestions.

First quick thought: What about renaming #userInterfaceTheme to #scopedInTheme or something similar that does not look like an accessor?

Second quick thought: What about putting weak arrays onto the theme's scope-stack? How much would this slow things down?

Best,
Marcel

Am 19.09.2017 10:44:30 schrieb Stéphane Rollandin <[hidden email]>:

Hello,


I would like to offer some technical feedback here, please do not take
it as a complaint directed against anyone's good work. In particular,
thanks again to all involved in designing the theming system. Only,
there are some worrisome issues in my opinion. Here we go:


1) The code below raises a DNU for #hardShadowOffset in SqueakTheme,
because #userInterfaceTheme is not used as intented (after
#softShadowOffset is sent, m gets popped out of the theme scope and is
not there anymore when #hardShadowOffset is sent):

| m uit |

m := Morph new.
uit := m userInterfaceTheme.
{uit softShadowOffset . uit hardShadowOffset}


However, for someone not aware of the intricacies of UserInterfaceTheme,
the code seems fine. In my opinion this is not a good state of affair.

Maybe the error raised here could be more explicit and in line with what
is really happening in the DNU, such as "There is no object in scope at
the moment".


2) Now consider the following code:

| m1 uit1 m2 uit2 |

m2 := BalloonMorph new.
uit2 := m2 userInterfaceTheme .

m1 := Morph new.
uit1 := m1 userInterfaceTheme.

{uit1 color . uit1 color}


When I evaluate it I get {nil . Color lightYellow1}

This is because while the first "uit1 color" does redirect to m1, the
second "uit1 color" redirects to m2 now that m1 has been popped out of
scope. This time there is no error raised, but the result is very
misleading.


3) For the record let me recall that doing

100 timesRepeat: [Morph new userInterfaceTheme]

results in having the current theme referencing 100 morphs that will
never be automatically garbage-collected.



Overall the three above points make me think that the method
#userInterfaceTheme is much too easy to unknowingly abuse of, and when
this happens undesirable behaviors can be rather difficult to make sense of.

At the moment the method comment is:
"Call this to conveniently access properties from the current user
interface theme."

I believe it needs some explicit warnings about how not to use it.


Just my 2 cents...


Stef



Reply | Threaded
Open this post in threaded view
|

Re: Abusing #userInterfaceTheme

Stéphane Rollandin
> First quick thought: What about renaming #userInterfaceTheme to
> #scopedInTheme or something similar that does not look like an accessor?

It is certainly better.

Now, since #userInterfaceTheme must always be followed by another
selector (else bad things may happen), it seems to me that it should not
be a unary method, but a binary one.

So what about

        Object>>themed: aSelector
          ^ self userInterfaceTheme perform: aSelector

... which of course could be simplified so what we get rid of
#userInterfaceTheme altogether (and actually of the full scoping mechanism):

        Object>>themed: aSelector
          ^ UserInterfaceTheme current get: self class -> aSelector


The usage would be

        BalloonMorph new themed: #color

where we now have

        BalloonMorph new userInterfaceTheme color

I understand the latest looks better, but I do not think it is worth all
the DNU shenanigans it requires the end user to be aware of...



Best,

Stef

Reply | Threaded
Open this post in threaded view
|

Re: Abusing #userInterfaceTheme

marcel.taeumel
Well, on the one hand, we do want to avoid the need for parentheses:

self color: (self themed: #color)

...which we did not manage because of the default values:

self color: (self userInterfaceTheme color ifNil: [Color yellow]).

So, that option is still on the table, Chris, right?

Just for the record: There was an idea to fully get rid of the parentheses by falling back to an object's message:

Tree >> defaultLeafColor
   ^ Color green

Tree >> initialize
   self leafColor: self userInterfaceTheme leafColor

But the current tools in Squeak would make editing all related artifacts a hassle.

Best,
Marcel

Am 19.09.2017 11:43:56 schrieb Stéphane Rollandin <[hidden email]>:

> First quick thought: What about renaming #userInterfaceTheme to
> #scopedInTheme or something similar that does not look like an accessor?

It is certainly better.

Now, since #userInterfaceTheme must always be followed by another
selector (else bad things may happen), it seems to me that it should not
be a unary method, but a binary one.

So what about

Object>>themed: aSelector
^ self userInterfaceTheme perform: aSelector

... which of course could be simplified so what we get rid of
#userInterfaceTheme altogether (and actually of the full scoping mechanism):

Object>>themed: aSelector
^ UserInterfaceTheme current get: self class -> aSelector


The usage would be

BalloonMorph new themed: #color

where we now have

BalloonMorph new userInterfaceTheme color

I understand the latest looks better, but I do not think it is worth all
the DNU shenanigans it requires the end user to be aware of...



Best,

Stef


Reply | Threaded
Open this post in threaded view
|

Re: Abusing #userInterfaceTheme

Stéphane Rollandin
> Well, on the one hand, we do want to avoid the need for parentheses:
>
> self color: (self themed: #color)
Hmm... if we are sure that all setters are always named from the
getters, then this may do the trick:

        Object>>getThemed: aSelector ifNone: defaultValue
          self perform: (aSelector, ':') asSymbol
                with: ((self themed: aSelector) ifNil: [defaultValue])

Then

    self color: (self userInterfaceTheme color ifNil: [Color yellow])

would become:

    self getThemed: #color ifNone: Color yellow


The only hurdle I see is the #asSymbol send which may slow things down a
little.


Stef

Reply | Threaded
Open this post in threaded view
|

Re: Abusing #userInterfaceTheme

marcel.taeumel
Well, not good. :) Then you would rely on a quite hidden side effect. You do not know in advance how the object stored in a theme will be used by the client. It is not always a simple instVar write.

Consider this example:

slider borderColor: ((self userInterfaceTheme borderColorModifier
ifNil: [ [:c | c adjustBrightness: -0.3] ]) value: aColor).

Best,
Marcel

Am 19.09.2017 12:19:17 schrieb Stéphane Rollandin <[hidden email]>:

> Well, on the one hand, we do want to avoid the need for parentheses:
>
> self color: (self themed: #color)
Hmm... if we are sure that all setters are always named from the
getters, then this may do the trick:

Object>>getThemed: aSelector ifNone: defaultValue
self perform: (aSelector, ':') asSymbol
with: ((self themed: aSelector) ifNil: [defaultValue])

Then

self color: (self userInterfaceTheme color ifNil: [Color yellow])

would become:

self getThemed: #color ifNone: Color yellow


The only hurdle I see is the #asSymbol send which may slow things down a
little.


Stef


Reply | Threaded
Open this post in threaded view
|

Re: Abusing #userInterfaceTheme

Stéphane Rollandin
> You do not know in advance how the object stored in a theme will be used
> by the client. It is not always a simple instVar write.
>
> Consider this example:
>
> slider borderColor: ((self userInterfaceTheme borderColorModifier
> ifNil: [ [:c | c adjustBrightness: -0.3] ]) value: aColor).


This can easily be folded back into the getter/setter pattern:

        BorderedMorph>>borderColorModifier: aBlock
            self borderColor: (aBlock value: self borderColor)

and then

        self borderColor: aColor.
        self getThemed: #borderColorModifier
                ifNone: [:c | c adjustBrightness: -0.3]


So I guess it's down to a design choice. I would vote for consistency of
pattern, and having everything written in getter/setter pairs.


Best,


Stef

Reply | Threaded
Open this post in threaded view
|

Re: Abusing #userInterfaceTheme

Hannes Hirzel
Stéphane

Could you please summarize in one mail which changes to the
UserInterface theme mechanism you propose and why you think that
should be done?

--HH.

On 9/19/17, Stéphane Rollandin <[hidden email]> wrote:

>> You do not know in advance how the object stored in a theme will be used
>> by the client. It is not always a simple instVar write.
>>
>> Consider this example:
>>
>> slider borderColor: ((self userInterfaceTheme borderColorModifier
>> ifNil: [ [:c | c adjustBrightness: -0.3] ]) value: aColor).
>
>
> This can easily be folded back into the getter/setter pattern:
>
> BorderedMorph>>borderColorModifier: aBlock
>    self borderColor: (aBlock value: self borderColor)
>
> and then
>
> self borderColor: aColor.
> self getThemed: #borderColorModifier
> ifNone: [:c | c adjustBrightness: -0.3]
>
>
> So I guess it's down to a design choice. I would vote for consistency of
> pattern, and having everything written in getter/setter pairs.
>
>
> Best,
>
>
> Stef
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Abusing #userInterfaceTheme

marcel.taeumel
Looks like several changes to me. :)

Bert thinks that we should just create a helper object as a result of #userInterfaceTheme that captures the sending object and handles the DNU code. There would be no need for having "scope" anymore and client code can safely store the result of #userInterfaceTheme and pass around.

Best,
Marcel

Am 22.09.2017 12:39:24 schrieb H. Hirzel <[hidden email]>:

Stéphane

Could you please summarize in one mail which changes to the
UserInterface theme mechanism you propose and why you think that
should be done?

--HH.

On 9/19/17, Stéphane Rollandin wrote:
>> You do not know in advance how the object stored in a theme will be used
>> by the client. It is not always a simple instVar write.
>>
>> Consider this example:
>>
>> slider borderColor: ((self userInterfaceTheme borderColorModifier
>> ifNil: [ [:c | c adjustBrightness: -0.3] ]) value: aColor).
>
>
> This can easily be folded back into the getter/setter pattern:
>
> BorderedMorph>>borderColorModifier: aBlock
> self borderColor: (aBlock value: self borderColor)
>
> and then
>
> self borderColor: aColor.
> self getThemed: #borderColorModifier
> ifNone: [:c | c adjustBrightness: -0.3]
>
>
> So I guess it's down to a design choice. I would vote for consistency of
> pattern, and having everything written in getter/setter pairs.
>
>
> Best,
>
>
> Stef
>
>


Reply | Threaded
Open this post in threaded view
|

Re: Abusing #userInterfaceTheme

Stéphane Rollandin
In reply to this post by Hannes Hirzel
> Stéphane
>
> Could you please summarize in one mail which changes to the
> UserInterface theme mechanism you propose and why you think that
> should be done?

I do not have a definite mechanism in mind; I just did some quick
brainstorming with Marcel...

I do think we should get rid of the scope mechanism though (see my
initial post in this thread for details).


Stef

Reply | Threaded
Open this post in threaded view
|

Re: Abusing #userInterfaceTheme

Hannes Hirzel
The scope mechanism now seems to be gone through the update The Trunk:
System-mt.963.mcz


Removes "scope" from UI themes. Encapsulates look-up for properties in
the current theme into short-living a request object.


Thank you Marcel for the update


If I now do the first example of Stephane (initial mail in this thread).

  | m uit |

        m := Morph new.
        uit := m userInterfaceTheme.

 an UserInterfaceThemeRequest

The UserInterfaceThemeRequest object has the following instance variables

1. a properties dictionary (323 entries for the Squeak theme)
2. name (which is 'SqueakTheme')
3. ignoreApply boolean
4. next (which is nil)
5. lastScaleFactor (which is nil).

Questions:
a) I do not see when and how the properties dictionary is initialized

    UserInterfactTheme just has
    initialize
        super initialize.
        name := 'unnamed'.
        properties := Dictionary new.

    There is a method SqueakTheme create [1]


b) What are instance variables 3..5 used for?


c) UserInterfaceTheme subclasses  {CommunityTheme . MonokaiTheme .
SolarizedTheme . SqueakTheme . EtoysTheme}

Are there other themes in addition? One like a traditional OS uses?

--Hannes


[1] SqueakTheme create

        "This is the default theme for Squeak.
       
        self create apply. "
       
        ^ (self named: 'Squeak') in: [:theme |
                "General morph stuff."
                theme
                        set: #keyboardFocusColor for: #Morph to: (TranslucentColor r: 0.3
g: 0.5 b: 0.5 alpha: 0.5);
                        set: #keyboardFocusWidth for: #Morph to: 3;
                        set: #softShadowColor for: #Morph to: (Color black alpha: 0.01);
                        set: #softShadowOffset for: #Morph to: (10@8 corner: 10@12);
                        set: #hardShadowColor for: #Morph to: (Color black alpha: 0.5);
                        set: #hardShadowOffset for: #Morph to: 1@1.
                       
                theme set: #background for: #MorphicProject to: self linenblue.
                       
                self
                        addFonts: theme;
                        addWindowColors: theme;
                        addSyntaxHighlighting: theme;
                        addMenusAndDockingBars: theme;
                        addDialogs: theme;
                        addButtons: theme;
                        addScrollables: theme;
                        addToolColors: theme;
                        addMVC: theme.
               
                theme]


On 9/22/17, Stéphane Rollandin <[hidden email]> wrote:

>> Stéphane
>>
>> Could you please summarize in one mail which changes to the
>> UserInterface theme mechanism you propose and why you think that
>> should be done?
>
> I do not have a definite mechanism in mind; I just did some quick
> brainstorming with Marcel...
>
> I do think we should get rid of the scope mechanism though (see my
> initial post in this thread for details).
>
>
> Stef
>