The Trunk: System-tpr.962.mcz

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

The Trunk: System-tpr.962.mcz

commits-2
tim Rowledge uploaded a new version of System to project The Trunk:
http://source.squeak.org/trunk/System-tpr.962.mcz

==================== Summary ====================

Name: System-tpr.962
Author: tpr
Time: 18 September 2017, 4:45:22.01173 pm
UUID: e1c54891-6fa2-45fd-9acb-58c76815aa62
Ancestors: System-dtl.961

Add and expand some comments that should help to explain UserInterfaceTheme for future readers

=============== Diff against System-dtl.961 ===============

Item was changed:
  ----- Method: UserInterfaceTheme>>doesNotUnderstand: (in category 'lookup') -----
+ doesNotUnderstand: aMessage
+ "In order to be able to use a UserInterfaceTheme as a container of fairly arbitrary properties whilst making it seem like we are sending regular messages as a way of accessing those properties we can handle the dNU: and check the not understood message name against our property list. This is clever, devious, interesting and may confuse users until they get to see an explanation like this"
+ "Answer nil or a value for the property specified by aMessage's #selector. Searching for the property proceeds by
+ a) searching my dictionary for a key made up of an Association with a key of the class of the 'client' and a value of the message name. For example SimpleButtonMorph->borderColor
+ b) searching again for a key made from the superclass(es) of the client - e.g. RectangleMorph->borderColor and then if required BorderedMorph->borderColor etc.
+ c) if there is a linked theme, search it in the same manner.
+
+ As an extreme example, consider a basic theme with a linked theme specifically for buttons.
+ mySimpleButtonMorph borderColor
+ would search the basic theme for SimpleButtonMorph->borderColor, the superclass equivalents as in b) above, then search the linked theme for SimpleButtonMorph->borderColor and, we hope, find something meaningful"
- doesNotUnderstand: aMessage
- "Answer whether I have, or inherit, a value for the visual-attribute specified by aMessage's #selector."
 
  aMessage numArgs > 0 ifTrue: [^ super doesNotUnderstand: aMessage].
  scope isEmpty ifTrue: [^ super doesNotUnderstand: aMessage].
 
  ^ [self get: scope top class -> aMessage selector]
  ensure: [scope pop]!

Item was changed:
  ----- Method: UserInterfaceTheme>>get: (in category 'private') -----
  get: keyObject
+ "keyObject is intended to be an Association. We have two lookup strategies: 1) along the superclass chain *of the client*, 2) via a linked theme. Evaluate the result because there can be message sends stored or blocks."
- "keyObject is intended to be an Association. We have two lookup strategies: 1) along the superclass chain, 2) via a linke theme. Evaluate the result because there can be message sends stored or blocks."
 
  | k |
  properties
  at: keyObject
  ifPresent: [:prop | ^ prop value].
 
  keyObject isVariableBinding "simple key objects"
  ifFalse: [^ self getViaLink: keyObject].
 
  k := keyObject key.
  (self getViaSuperclasses: keyObject)
  ifNotNil: [:prop | ^ prop].
 
  keyObject key: k. "restore"
  ^ self getViaLink: keyObject!

Item was changed:
  ----- Method: UserInterfaceTheme>>getViaLink: (in category 'private') -----
  getViaLink: keyObject
+ "keyObject is intended to be an Association.
+ If there is a linked theme, see if it has the relevant property available"
- "keyObject is intended to be an Association"
 
  ^ next ifNotNil: [next get: keyObject]!

Item was changed:
  ----- Method: UserInterfaceTheme>>getViaSuperclasses: (in category 'private') -----
  getViaSuperclasses: keyObject
+ "keyObject is intended to be an Association.
+ Find the superclass of the key of the keyObject (which will initially be the client's class) and make a new keyObject using that and the original message name, then try searching for that."
- "keyObject is intended to be an Association"
 
  "We know we're the only referencer of keyObject.  Update it rather than create new ones, for performance reasons."
  keyObject key: keyObject key superclass.
 
  keyObject key ifNil: [^ nil].
 
  properties
  at: keyObject
  ifPresent: [:prop | ^ prop value].
 
  ^ self getViaSuperclasses: keyObject!


Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: System-tpr.962.mcz

Chris Muller-3
With all due respect, Tim, this comment reads like your "reaction".
It doesn't leave the reader with any more clarity than from simply
reading the code.  We (myself, Marcel and Hannes) spent a good part of
a year critically thinking and discussing Themeing in Squeak to arrive
at this balanced and effective design.  Only a few minutes tracing the
sparse amount of code and thinking about the requirements is needed to
understand and appreciate it.

You've mentioned the "dreadful state" of the wiki documentation
before, and Hannes provided the link to the existing page about
UserInterfaceTheme.  Your contributions are better appreciated by working
with original authors and using the Inbox first.

Thanks.

On Mon, Sep 18, 2017 at 6:45 PM,  <[hidden email]> wrote:

> tim Rowledge uploaded a new version of System to project The Trunk:
> http://source.squeak.org/trunk/System-tpr.962.mcz
>
> ==================== Summary ====================
>
> Name: System-tpr.962
> Author: tpr
> Time: 18 September 2017, 4:45:22.01173 pm
> UUID: e1c54891-6fa2-45fd-9acb-58c76815aa62
> Ancestors: System-dtl.961
>
> Add and expand some comments that should help to explain UserInterfaceTheme for future readers
>
> =============== Diff against System-dtl.961 ===============
>
> Item was changed:
>   ----- Method: UserInterfaceTheme>>doesNotUnderstand: (in category 'lookup') -----
> + doesNotUnderstand: aMessage
> +       "In order to be able to use a UserInterfaceTheme as a container of fairly arbitrary properties whilst making it seem like we are sending regular messages as a way of accessing those properties we can handle the dNU: and check the not understood message name against our property list. This is clever, devious, interesting and may confuse users until they get to see an explanation like this"
> +       "Answer nil or a value for the property specified by aMessage's #selector. Searching for the property proceeds by
> +       a) searching my dictionary for a key made up of an Association with a key of the class of the 'client' and a value of the message name. For example SimpleButtonMorph->borderColor
> +       b) searching again for a key made from the superclass(es) of the client - e.g. RectangleMorph->borderColor and then if required BorderedMorph->borderColor etc.
> +       c) if there is a linked theme, search it in the same manner.
> +
> +       As an extreme example, consider a basic theme with a linked theme specifically for buttons.
> +       mySimpleButtonMorph borderColor
> +       would search the basic theme for SimpleButtonMorph->borderColor, the superclass equivalents as in b) above, then search the linked theme for SimpleButtonMorph->borderColor and, we hope, find something meaningful"
> - doesNotUnderstand: aMessage
> -       "Answer whether I have, or inherit, a value for the visual-attribute specified by aMessage's #selector."
>
>         aMessage numArgs > 0 ifTrue: [^ super doesNotUnderstand: aMessage].
>         scope isEmpty ifTrue: [^ super doesNotUnderstand: aMessage].
>
>         ^ [self get: scope top class -> aMessage selector]
>                 ensure: [scope pop]!
>
> Item was changed:
>   ----- Method: UserInterfaceTheme>>get: (in category 'private') -----
>   get: keyObject
> +       "keyObject is intended to be an Association. We have two lookup strategies: 1) along the superclass chain *of the client*, 2) via a linked theme. Evaluate the result because there can be message sends stored or blocks."
> -       "keyObject is intended to be an Association. We have two lookup strategies: 1) along the superclass chain, 2) via a linke theme. Evaluate the result because there can be message sends stored or blocks."
>
>         | k |
>         properties
>                 at: keyObject
>                 ifPresent: [:prop | ^ prop value].
>
>         keyObject isVariableBinding "simple key objects"
>                 ifFalse: [^ self getViaLink: keyObject].
>
>         k := keyObject key.
>         (self getViaSuperclasses: keyObject)
>                 ifNotNil: [:prop | ^ prop].
>
>         keyObject key: k. "restore"
>         ^ self getViaLink: keyObject!
>
> Item was changed:
>   ----- Method: UserInterfaceTheme>>getViaLink: (in category 'private') -----
>   getViaLink: keyObject
> +       "keyObject is intended to be an Association.
> +       If there is a linked theme, see if it has the relevant property available"
> -       "keyObject is intended to be an Association"
>
>         ^ next ifNotNil: [next get: keyObject]!
>
> Item was changed:
>   ----- Method: UserInterfaceTheme>>getViaSuperclasses: (in category 'private') -----
>   getViaSuperclasses: keyObject
> +       "keyObject is intended to be an Association.
> +       Find the superclass of the key of the keyObject (which will initially be the client's class) and make a new keyObject using that and the original message name, then try searching for that."
> -       "keyObject is intended to be an Association"
>
>         "We know we're the only referencer of keyObject.  Update it rather than create new ones, for performance reasons."
>         keyObject key: keyObject key superclass.
>
>         keyObject key ifNil: [^ nil].
>
>         properties
>                 at: keyObject
>                 ifPresent: [:prop | ^ prop value].
>
>         ^ self getViaSuperclasses: keyObject!
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: System-tpr.962.mcz

marcel.taeumel
Hi Tim,

if you want to bench the lookup in your system, take a look at UserInterfaceThemeTestObject >> #benchLookup.

Note that Squeak's current widgets got changed to only use themes in their initialization code. It is not a regular lookup to, for example, draw a button.

Best,
Marcel

Am 19.09.2017 06:21:42 schrieb Chris Muller <[hidden email]>:

With all due respect, Tim, this comment reads like your "reaction".
It doesn't leave the reader with any more clarity than from simply
reading the code. We (myself, Marcel and Hannes) spent a good part of
a year critically thinking and discussing Themeing in Squeak to arrive
at this balanced and effective design. Only a few minutes tracing the
sparse amount of code and thinking about the requirements is needed to
understand and appreciate it.

You've mentioned the "dreadful state" of the wiki documentation
before, and Hannes provided the link to the existing page about
UserInterfaceTheme. Your contributions are better appreciated by working
with original authors and using the Inbox first.

Thanks.

On Mon, Sep 18, 2017 at 6:45 PM, wrote:

> tim Rowledge uploaded a new version of System to project The Trunk:
> http://source.squeak.org/trunk/System-tpr.962.mcz
>
> ==================== Summary ====================
>
> Name: System-tpr.962
> Author: tpr
> Time: 18 September 2017, 4:45:22.01173 pm
> UUID: e1c54891-6fa2-45fd-9acb-58c76815aa62
> Ancestors: System-dtl.961
>
> Add and expand some comments that should help to explain UserInterfaceTheme for future readers
>
> =============== Diff against System-dtl.961 ===============
>
> Item was changed:
> ----- Method: UserInterfaceTheme>>doesNotUnderstand: (in category 'lookup') -----
> + doesNotUnderstand: aMessage
> + "In order to be able to use a UserInterfaceTheme as a container of fairly arbitrary properties whilst making it seem like we are sending regular messages as a way of accessing those properties we can handle the dNU: and check the not understood message name against our property list. This is clever, devious, interesting and may confuse users until they get to see an explanation like this"
> + "Answer nil or a value for the property specified by aMessage's #selector. Searching for the property proceeds by
> + a) searching my dictionary for a key made up of an Association with a key of the class of the 'client' and a value of the message name. For example SimpleButtonMorph->borderColor
> + b) searching again for a key made from the superclass(es) of the client - e.g. RectangleMorph->borderColor and then if required BorderedMorph->borderColor etc.
> + c) if there is a linked theme, search it in the same manner.
> +
> + As an extreme example, consider a basic theme with a linked theme specifically for buttons.
> + mySimpleButtonMorph borderColor
> + would search the basic theme for SimpleButtonMorph->borderColor, the superclass equivalents as in b) above, then search the linked theme for SimpleButtonMorph->borderColor and, we hope, find something meaningful"
> - doesNotUnderstand: aMessage
> - "Answer whether I have, or inherit, a value for the visual-attribute specified by aMessage's #selector."
>
> aMessage numArgs > 0 ifTrue: [^ super doesNotUnderstand: aMessage].
> scope isEmpty ifTrue: [^ super doesNotUnderstand: aMessage].
>
> ^ [self get: scope top class -> aMessage selector]
> ensure: [scope pop]!
>
> Item was changed:
> ----- Method: UserInterfaceTheme>>get: (in category 'private') -----
> get: keyObject
> + "keyObject is intended to be an Association. We have two lookup strategies: 1) along the superclass chain *of the client*, 2) via a linked theme. Evaluate the result because there can be message sends stored or blocks."
> - "keyObject is intended to be an Association. We have two lookup strategies: 1) along the superclass chain, 2) via a linke theme. Evaluate the result because there can be message sends stored or blocks."
>
> | k |
> properties
> at: keyObject
> ifPresent: [:prop | ^ prop value].
>
> keyObject isVariableBinding "simple key objects"
> ifFalse: [^ self getViaLink: keyObject].
>
> k := keyObject key.
> (self getViaSuperclasses: keyObject)
> ifNotNil: [:prop | ^ prop].
>
> keyObject key: k. "restore"
> ^ self getViaLink: keyObject!
>
> Item was changed:
> ----- Method: UserInterfaceTheme>>getViaLink: (in category 'private') -----
> getViaLink: keyObject
> + "keyObject is intended to be an Association.
> + If there is a linked theme, see if it has the relevant property available"
> - "keyObject is intended to be an Association"
>
> ^ next ifNotNil: [next get: keyObject]!
>
> Item was changed:
> ----- Method: UserInterfaceTheme>>getViaSuperclasses: (in category 'private') -----
> getViaSuperclasses: keyObject
> + "keyObject is intended to be an Association.
> + Find the superclass of the key of the keyObject (which will initially be the client's class) and make a new keyObject using that and the original message name, then try searching for that."
> - "keyObject is intended to be an Association"
>
> "We know we're the only referencer of keyObject. Update it rather than create new ones, for performance reasons."
> keyObject key: keyObject key superclass.
>
> keyObject key ifNil: [^ nil].
>
> properties
> at: keyObject
> ifPresent: [:prop | ^ prop value].
>
> ^ self getViaSuperclasses: keyObject!
>
>



Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: System-tpr.962.mcz

timrowledge
In reply to this post by Chris Muller-3

> On 18-09-2017, at 9:20 PM, Chris Muller <[hidden email]> wrote:
>
> With all due respect, Tim, this comment reads like your "reaction".
> It doesn't leave the reader with any more clarity than from simply
> reading the code.

“Read the code” is never an acceptable response to someone wanting to understand what some code is *supposed* to do or how to use it. Except just possibly if said code is so utterly beautifully written and complete with all possible cases clearly handled and easily comprehend examples are trivially findable that it really does just explain everything. I do not believe any such piece of code has ever been written, nor is likely to be so.


tim
--
tim Rowledge; [hidden email]; http://www.rowledge.org/tim
Strange OpCodes: PBF: Pay Bus Fare



Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: System-tpr.962.mcz

Chris Muller-3
I couldn't disagree more about your "never" and "ever", but that's
beside the point.  My complaint is about the comment itself.  Some
authors like for their system to maintain a cohesive and consistently
terse commenting style that _compliments_ the code, not supplants it.
I wanted you to "read the code" so you would see and respect this
existing original style of minimal, carefully-crafted word selections.

I know you meant well, but when I saw "devious" it read like you were
reacting, because they teach us in Smalltalk school that we should not
override DNU.  Your long comment feels like you're "punishing" that
4 lines of code with 24 lines of over-explanation, replete with
examples.  That's good stuff for the swiki, but orthogonal to the rest
of the style and a distraction from this particular code, IMO.

Breathe...

On Thu, Sep 21, 2017 at 11:57 AM, tim Rowledge <[hidden email]> wrote:

>
>> On 18-09-2017, at 9:20 PM, Chris Muller <[hidden email]> wrote:
>>
>> With all due respect, Tim, this comment reads like your "reaction".
>> It doesn't leave the reader with any more clarity than from simply
>> reading the code.
>
> “Read the code” is never an acceptable response to someone wanting to understand what some code is *supposed* to do or how to use it. Except just possibly if said code is so utterly beautifully written and complete with all possible cases clearly handled and easily comprehend examples are trivially findable that it really does just explain everything. I do not believe any such piece of code has ever been written, nor is likely to be so.
>
>
> tim
> --
> tim Rowledge; [hidden email]; http://www.rowledge.org/tim
> Strange OpCodes: PBF: Pay Bus Fare
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: System-tpr.962.mcz

John Pfersich-2
In reply to this post by timrowledge
+1

Sent from my iPhone

> On Sep 21, 2017, at 09:57, tim Rowledge <[hidden email]> wrote:
>
>
>> On 18-09-2017, at 9:20 PM, Chris Muller <[hidden email]> wrote:
>>
>> With all due respect, Tim, this comment reads like your "reaction".
>> It doesn't leave the reader with any more clarity than from simply
>> reading the code.
>
> “Read the code” is never an acceptable response to someone wanting to understand what some code is *supposed* to do or how to use it. Except just possibly if said code is so utterly beautifully written and complete with all possible cases clearly handled and easily comprehend examples are trivially findable that it really does just explain everything. I do not believe any such piece of code has ever been written, nor is likely to be so.
>
>
> tim
> --
> tim Rowledge; [hidden email]; http://www.rowledge.org/tim
> Strange OpCodes: PBF: Pay Bus Fare
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: System-tpr.962.mcz

Hannes Hirzel
In reply to this post by Chris Muller-3
On 9/22/17, Chris Muller <[hidden email]> wrote:

> I couldn't disagree more about your "never" and "ever", but that's
> beside the point.  My complaint is about the comment itself.  Some
> authors like for their system to maintain a cohesive and consistently
> terse commenting style that _compliments_ the code, not supplants it.
> I wanted you to "read the code" so you would see and respect this
> existing original style of minimal, carefully-crafted word selections.
>
> I know you meant well, but when I saw "devious" it read like you were
> reacting, because they teach us in Smalltalk school that we should not
> override DNU.  Your long comment feels like you're "punishing" that
> 4 lines of code with 24 lines of over-explanation, replete with
> examples.  That's good stuff for the swiki,
+1 for having more explanations on the swiki

http://wiki.squeak.org/squeak/6508  UserInterfaceTheme

in particular

http://wiki.squeak.org/squeak/6576 UserInterfaceTheme design note

> but orthogonal to the rest
> of the style and a distraction from this particular code, IMO.
>
> Breathe...
>
> On Thu, Sep 21, 2017 at 11:57 AM, tim Rowledge <[hidden email]> wrote:
>>
>>> On 18-09-2017, at 9:20 PM, Chris Muller <[hidden email]> wrote:
>>>
>>> With all due respect, Tim, this comment reads like your "reaction".
>>> It doesn't leave the reader with any more clarity than from simply
>>> reading the code.
>>
>> “Read the code” is never an acceptable response to someone wanting to
>> understand what some code is *supposed* to do or how to use it. Except
>> just possibly if said code is so utterly beautifully written and complete
>> with all possible cases clearly handled and easily comprehend examples are
>> trivially findable that it really does just explain everything. I do not
>> believe any such piece of code has ever been written, nor is likely to be
>> so.
>>
>>
>> tim
>> --
>> tim Rowledge; [hidden email]; http://www.rowledge.org/tim
>> Strange OpCodes: PBF: Pay Bus Fare
>>
>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: The Trunk: System-tpr.962.mcz

Hannes Hirzel
The comment may contain a link to the wiki page and the wiki page may
have more than 24 lines ...   :-) This is very welcome.

Another probably better option is to have a help topic which titles like

     - How do I create a user interface theme?
     - How do user interface themes work?
     - User interface themes migration issues



On 9/22/17, H. Hirzel <[hidden email]> wrote:

> On 9/22/17, Chris Muller <[hidden email]> wrote:
>> I couldn't disagree more about your "never" and "ever", but that's
>> beside the point.  My complaint is about the comment itself.  Some
>> authors like for their system to maintain a cohesive and consistently
>> terse commenting style that _compliments_ the code, not supplants it.
>> I wanted you to "read the code" so you would see and respect this
>> existing original style of minimal, carefully-crafted word selections.
>>
>> I know you meant well, but when I saw "devious" it read like you were
>> reacting, because they teach us in Smalltalk school that we should not
>> override DNU.  Your long comment feels like you're "punishing" that
>> 4 lines of code with 24 lines of over-explanation, replete with
>> examples.  That's good stuff for the swiki,
> +1 for having more explanations on the swiki
>
> http://wiki.squeak.org/squeak/6508  UserInterfaceTheme
>
> in particular
>
> http://wiki.squeak.org/squeak/6576 UserInterfaceTheme design note
>
>> but orthogonal to the rest
>> of the style and a distraction from this particular code, IMO.
>>
>> Breathe...
>>
>> On Thu, Sep 21, 2017 at 11:57 AM, tim Rowledge <[hidden email]> wrote:
>>>
>>>> On 18-09-2017, at 9:20 PM, Chris Muller <[hidden email]> wrote:
>>>>
>>>> With all due respect, Tim, this comment reads like your "reaction".
>>>> It doesn't leave the reader with any more clarity than from simply
>>>> reading the code.
>>>
>>> “Read the code” is never an acceptable response to someone wanting to
>>> understand what some code is *supposed* to do or how to use it. Except
>>> just possibly if said code is so utterly beautifully written and
>>> complete
>>> with all possible cases clearly handled and easily comprehend examples
>>> are
>>> trivially findable that it really does just explain everything. I do not
>>> believe any such piece of code has ever been written, nor is likely to
>>> be
>>> so.
>>>
>>>
>>> tim
>>> --
>>> tim Rowledge; [hidden email]; http://www.rowledge.org/tim
>>> Strange OpCodes: PBF: Pay Bus Fare
>>>
>>>
>>>
>>
>>
>