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 |
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
|
> 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 |
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
|
> 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 |
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
|
> 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 |
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 > > |
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
|
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 |
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 > |
Free forum by Nabble | Edit this page |