<table><tr><td style="">gladhorn added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D14855">View Revision</a></tr></table><br /><div><div><p>This looks generally nice, I like it. Since we're short on people maintaining this stuff, I'd like more of the code to be shared though.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14855#inline-79226">View Inline</a><span style="color: #4b4d51; font-weight: bold;">daemon.h:66</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span style="color: #74777d">// DBus</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span style="color: #aa4000">void</span> <span style="color: #004012">applyLayoutPreset</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span class="n">presetName</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; ">    <span style="color: #a0a000">Q_SIGNALS</span><span class="p">:</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Could this be unified in some nice way with the above function (applyOsdAction) which does the same based on the enum...? Maybe just use the enum, or is there a reason for using a string here but not in other places?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14855#inline-79231">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kscreenapplet.cpp:51</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">connect</span><span class="p">(</span><span style="color: #aa4000">new</span> <span class="n">KScreen</span><span style="color: #aa2211">::</span><span class="n">GetConfigOperation</span><span class="p">(</span><span class="n">KScreen</span><span style="color: #aa2211">::</span><span class="n">GetConfigOperation</span><span style="color: #aa2211">::</span><span class="n">NoEDID</span><span class="p">),</span> <span style="color: #aa2211">&</span><span class="n">KScreen</span><span style="color: #aa2211">::</span><span class="n">ConfigOperation</span><span style="color: #aa2211">::</span><span class="n">finished</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">            <span style="color: #aa4000">this</span><span class="p">,</span> <span class="p">[</span><span style="color: #aa4000">this</span><span class="p">](</span><span class="n">KScreen</span><span style="color: #aa2211">::</span><span class="n">ConfigOperation</span> <span style="color: #aa2211">*</span><span class="n">op</span><span class="p">)</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">What is the ownership of the GetConfigOperation? Is it deleted? (I don't know this code very well)</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14855#inline-79233">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kscreenapplet.cpp:62</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #aa4000">void</span> <span class="n">KScreenApplet</span><span style="color: #aa2211">::</span><span class="n">configChanged</span><span class="p">()</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This seems completely unused? Remove it maybe.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14855#inline-79229">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kscreenapplet.h:43</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">enum</span> <span class="n">Action</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span class="n">SwitchToExternal</span><span class="p">,</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This enum is a duplicate of OsdAction::Action, and it's off by one. I don't think that's a good idea. Let's share the enum somehow (and move to enum class for new enums).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14855#inline-79234">View Inline</a><span style="color: #4b4d51; font-weight: bold;">main.qml:51</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #004012">readonly</span> <span style="color: #004012">property</span> <span style="color: #aa4000">var</span> <span style="color: #aa4000">screenLayouts:</span> <span class="p">[</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This is completely duplicated from the OSD, is there no way to share the data instead? I know it doesn't have the "do nothing" action, but I'd rather see improvements in the OSD at the same time (which maybe could get rid of do nothing in favor of just disappearing when the user clicks somewhere else...).</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D14855#inline-79225">View Inline</a><span style="color: #4b4d51; font-weight: bold;">metadata.desktop:3</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">Name=Display Configuration
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">Comment=Quickly switch between monitor layouts and presentation mode
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">Icon=preferences-desktop-display
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">We usually refer to the "monitors" as screen or display, this adds a third term ;) So I'd prefer to have either screen or display here.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R104 KScreen</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D14855">https://phabricator.kde.org/D14855</a></div></div><br /><div><strong>To: </strong>broulik, Plasma, VDG, fischbach, harmathy<br /><strong>Cc: </strong>gladhorn, abetts, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart<br /></div>