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 <lecteur at zogotounga.net>:
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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20170919/9dc0d737/attachment.html>