--96f82716a7da4363bc25fb967db673b0
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="ascii"
Mime-Version: 1.0

davidedmundson requested changes to this revision.
davidedmundson added a comment.
This revision now requires changes to proceed.


  Concept is good.

INLINE COMMENTS

> colorscope.cpp:127
>      }
> +    m_lastGroup = m_group;
>      return m_group;

It's weird to be caching in a public getter.

It opens us up for problems

If some client code (for whatever reason) did 
connect(scope, inheritChanged, []() {scope->colorGroup());

then our signals in checkColorGroupChanged won't get emitted.

Can we move this member var into checkColorGroupChanged? Means we can get rid of the mutable too

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D8917

To: apol, #plasma, mart, davidedmundson
Cc: davidedmundson, plasma-devel, #frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

--96f82716a7da4363bc25fb967db673b0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/html; charset="ascii"
Mime-Version: 1.0

<table><tr><td style=3D"">davidedmundson requested changes to this revision=
.<br />davidedmundson added a comment.<br />This revision now requires chan=
ges to proceed.
</td><a style=3D"text-decoration: none; padding: 4px 8px; margin: 0 8px 8px=
; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; back=
ground-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1=
f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href=
=3D"https://phabricator.kde.org/D8917" rel=3D"noreferrer">View Revision</a>=
</tr></table><br /><div><div><p>Concept is good.</p></div></div><br /><div>=
<strong>INLINE COMMENTS</strong><div><div style=3D"margin: 6px 0 12px 0;"><=
div style=3D"border: 1px solid #C7CCD9; border-radius: 3px;"><div style=3D"=
padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid=
; border-width: 0 0 1px 0; margin: 0;"><div style=3D"color: #74777d; backgr=
ound: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style=3D"float: righ=
t; text-decoration: none;" href=3D"https://phabricator.kde.org/D8917#inline=
-40793" rel=3D"noreferrer">View Inline</a><span style=3D"color: #4b4d51; fo=
nt-weight: bold;">colorscope.cpp:127</span></div>
<div style=3D"font: 11px/15px &quot;Menlo&quot;, &quot;Consolas&quot;, &quo=
t;Monaco&quot;, monospace; white-space: pre-wrap; clear: both; padding: 4px=
 0; margin: 0;"><div style=3D"padding: 0 8px; margin: 0 4px; ">    <span cl=
ass=3D"p">}</span>
</div><div style=3D"padding: 0 8px; margin: 0 4px; background: rgba(151, 23=
4, 151, .6);">    <span class=3D"n">m_lastGroup</span> <span style=3D"color=
: #aa2211">=3D</span> <span class=3D"n">m_group</span><span class=3D"p">;</=
span>
</div><div style=3D"padding: 0 8px; margin: 0 4px; ">    <span style=3D"col=
or: #aa4000">return</span> <span class=3D"n">m_group</span><span class=3D"p=
">;</span>
</div></div></div>
<div style=3D"margin: 8px 0; padding: 0 12px;"><p style=3D"padding: 0; marg=
in: 8px;">It&#039;s weird to be caching in a public getter.</p>

<p style=3D"padding: 0; margin: 8px;">It opens us up for problems</p>

<p style=3D"padding: 0; margin: 8px;">If some client code (for whatever rea=
son) did <br />
connect(scope, inheritChanged, []() {scope-&gt;colorGroup());</p>

<p style=3D"padding: 0; margin: 8px;">then our signals in checkColorGroupCh=
anged won&#039;t get emitted.</p>

<p style=3D"padding: 0; margin: 8px;">Can we move this member var into chec=
kColorGroupChanged? Means we can get rid of the mutable too</p></div></div>=
</div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R242 Plas=
ma Framework (Library)</div></div></div><br /><div><strong>REVISION DETAIL<=
/strong><div><a href=3D"https://phabricator.kde.org/D8917" rel=3D"noreferre=
r">https://phabricator.kde.org/D8917</a></div></div><br /><div><strong>To: =
</strong>apol, Plasma, mart, davidedmundson<br /><strong>Cc: </strong>david=
edmundson, plasma-devel, Frameworks, ZrenBot, progwolff, lesliezhai, ali-mo=
hamed, jensreuterberg, abetts, sebas, apol, mart<br /></div>
--96f82716a7da4363bc25fb967db673b0--