<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"><!-- P {margin-top:0;margin-bottom:0;} --></style>
</head>
<body dir="ltr">
<div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr">
<div id="divtagdefaultwrapper" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr">
<p>Hi Subbu, thanks for your fast review!</p>
<p><br>
</p>
<p>> <span style="font-size: 12pt;">is isPicking really required? Why introduce a mode?</span></p>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>> <span style="font-size: 12pt;">targetColor is getting set regardless of isPicking here.</span>
<div><br>
</div>
<div>I do not understand you?</div>
<div><br>
</div>
<div>> <span style="font-size: 12pt;">Could this be simplified to :</span>
<div>> </div>
<div>> self pickColorFromDisplay ifNotNil: [:pickedColor | self</div>
<div>> selectedColor: pickedColor ].</div>
<div><br>
</div>
<div>Actually not, because #isPicking is enabled until the #ensure: block has been passed. But you
<span style="font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols; font-size: 16px;">
probably </span>have identified a code smell in my current approach here :-)</div>
<div>Alternatively, we could also say:</div>
<div><br>
</div>
</div>
</div>
</div>
<blockquote style="margin: 0 0 0 40px; border: none; padding: 0px;">
<div style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr">
<div>
<div>
<div>
<div>pickColor</div>
</div>
</div>
</div>
</div>
<div style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr">
<div>
<div><span style="font-size: 12pt;"> [</span></div>
</div>
</div>
<div style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr">
<div>
<div> | previousColor selectedColor |</div>
</div>
</div>
<div style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr">
<div>
<div> previousColor := self selectedColor.</div>
</div>
</div>
<div style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr">
<div>
<div> selectedColor := self pickColorFromDisplay.</div>
</div>
</div>
<div style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr">
<div>
<div> selectedColor</div>
</div>
</div>
<div style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr">
<div>
<div><span> ifNil: [self selectedColor: previousColor]</span></div>
</div>
</div>
<div style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr">
<div>
<div> ifNotNil: [self targetColor: selectorColor].</div>
</div>
</div>
<div style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr">
<div>
<div> ] ensure: [</div>
</div>
</div>
<div style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr">
<div>
<div> self isPicking: false].</div>
</div>
</div>
</blockquote>
<div style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr">
<div><br>
</div>
<div>Would this be more intuitive?</div>
<div><br>
</div>
<div>Best,</div>
<div>Christoph</div>
<p></p>
<div id="Signature">
<div id="divtagdefaultwrapper" dir="ltr" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;">
<div name="divtagdefaultwrapper" style="font-family:Calibri,Arial,Helvetica,sans-serif; font-size:; margin:0">
<div><font size="2" color="#808080"></font></div>
</div>
</div>
</div>
<br>
<br>
<div style="color: rgb(0, 0, 0);">
<div>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>Von:</b> Squeak-dev <squeak-dev-bounces@lists.squeakfoundation.org> im Auftrag von K K Subbu <kksubbu.ml@gmail.com><br>
<b>Gesendet:</b> Samstag, 29. Februar 2020 12:39 Uhr<br>
<b>An:</b> squeak-dev@lists.squeakfoundation.org<br>
<b>Betreff:</b> Re: [squeak-dev] The Inbox: Morphic-ct.1634.mcz</font>
<div> </div>
</div>
</div>
<font size="2"><span style="font-size:10pt;">
<div class="PlainText">On 29/02/20 10:46 AM, commits@source.squeak.org wrote:<br>
> Morph subclass: #NewColorPickerMorph<br>
> + instanceVariableNames: 'target setColorSelector hsvaMorph colorPresenter isPicking'<br>
> - instanceVariableNames: 'target setColorSelector hsvaMorph colorPresenter'<br>
<br>
is isPicking really required? Why introduce a mode?<br>
<br>
> ----- Method: NewColorPickerMorph>>colorSelected: (in category 'model') -----<br>
> colorSelected: aColor<br>
> + self isPicking ifFalse: [<br>
> + self targetColor: aColor].<br>
> - self targetColor: aColor.<br>
<br>
targetColor is getting set regardless of isPicking here.<br>
<br>
> + ----- Method: NewColorPickerMorph>>pickColor (in category 'picking') -----<br>
> + pickColor<br>
> + <br>
> + | selectedColor |<br>
> + [<br>
> + | previousColor |<br>
> + previousColor := self selectedColor.<br>
> + selectedColor := self pickColorFromDisplay.<br>
> + selectedColor ifNil: [^ self selectedColor: previousColor].<br>
> + ] ensure: [<br>
> + self isPicking: false].<br>
> + self selectedColor: selectedColor.!<br>
<br>
Could this be simplified to :<br>
<br>
self pickColorFromDisplay ifNotNil: [:pickedColor | self <br>
selectedColor: pickedColor ].<br>
<br>
Regards .. Subbu<br>
<br>
</div>
</span></font></div>
</div>
</div>
</body>
</html>