Hi Subbu, thanks for your fast review!


> is isPicking really required? Why introduce a mode?

If we don't introduce a mode, the target's color will be updated about 50 times per second (depending on your image's speed). This can be really slow ... Using #isPicking allows deferring the color update until the user really clicks at any point. Still, the currently hovered color is displayed in the picker.

> targetColor is getting set regardless of isPicking here.

I do not understand you?

> Could this be simplified to :
>
>   self pickColorFromDisplay ifNotNil: [:pickedColor | self
> selectedColor: pickedColor ].

Actually not, because #isPicking is enabled until the #ensure: block has been passed. But you probably have identified a code smell in my current approach here :-)
Alternatively, we could also say:

pickColor
     [
             | previousColor selectedColor |
             previousColor := self selectedColor.
             selectedColor := self pickColorFromDisplay.
             selectedColor
                 ifNil: [self selectedColor: previousColor]
                 ifNotNil: [self targetColor: selectorColor].
     ] ensure: [
             self isPicking: false].

Would this be more intuitive?

Best,
Christoph


________________________________
Von: Squeak-dev <squeak-dev-bounces at lists.squeakfoundation.org> im Auftrag von K K Subbu <kksubbu.ml at gmail.com>
Gesendet: Samstag, 29. Februar 2020 12:39 Uhr
An: squeak-dev at lists.squeakfoundation.org
Betreff: Re: [squeak-dev] The Inbox: Morphic-ct.1634.mcz

On 29/02/20 10:46 AM, commits at source.squeak.org wrote:
>    Morph subclass: #NewColorPickerMorph
> +      instanceVariableNames: 'target setColorSelector hsvaMorph colorPresenter isPicking'
> -      instanceVariableNames: 'target setColorSelector hsvaMorph colorPresenter'

is isPicking really required? Why introduce a mode?

>    ----- Method: NewColorPickerMorph>>colorSelected: (in category 'model') -----
>    colorSelected: aColor
> +      self isPicking ifFalse: [
> +              self targetColor: aColor].
> -      self targetColor: aColor.

targetColor is getting set regardless of isPicking here.

> + ----- Method: NewColorPickerMorph>>pickColor (in category 'picking') -----
> + pickColor
> +
> +      | selectedColor |
> +      [
> +              | previousColor |
> +              previousColor := self selectedColor.
> +              selectedColor := self pickColorFromDisplay.
> +              selectedColor ifNil: [^ self selectedColor: previousColor].
> +      ] ensure: [
> +              self isPicking: false].
> +      self selectedColor: selectedColor.!

Could this be simplified to :

  self pickColorFromDisplay ifNotNil: [:pickedColor | self
selectedColor: pickedColor ].

Regards .. Subbu

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/squeak-dev/attachments/20200229/2fe90926/attachment.html>