<table><tr><td style="">romangg requested changes to this revision.<br />romangg added inline comments.<br />This revision now requires changes to proceed.
</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/D9255" rel="noreferrer">View Revision</a></tr></table><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/D9255#inline-41781" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ThumbnailStrip.qml:113</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; "> <span style="color: #aa4000">preventStealing:</span> <span style="color: #000a65">true</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">cursorShape:</span> <span style="color: #004012">Qt</span><span class="p">.</span><span style="color: #004012">OpenHandCursor</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">acceptedButtons:</span> <span style="color: #004012">Qt</span><span class="p">.</span><span style="color: #004012">LeftButton</span> <span style="color: #aa2211">|</span> <span style="color: #004012">Qt</span><span class="p">.</span><span style="color: #004012">RightButton</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Since now the thumbnail is clickable, remove.</p>
<p style="padding: 0; margin: 8px;">Also could you give the thumbnail an effect to show that it is clickable? Best would be imo a frame in highlight color, which switches to the context menu button, when mouse cursor moves from thumbnail to inside of context menu button area. This would be consistent with other elements of the workspace (with breeze).</p>
<p style="padding: 0; margin: 8px;">But since we have not yet decided on a consistent highlighting scheme in general, you can also try other styles.</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/D9255#inline-41788" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ThumbnailStrip.qml:233</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">PlasmaCore</span><span class="p">.</span><span style="color: #004012">IconItem</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">anchors.fill:</span> <span style="color: #004012">parent</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">acceptedButto</span>ns:</span> <span class="bright"></span><span style="color: #004012"><span class="bright">Qt</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span style="color: #004012"><span class="bright">RightButton</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"> </span><span style="color: #aa4000"><span class="bright">anchors.margi</span>ns:</span> <span class="bright"></span><span style="color: #004012"><span class="bright">parent</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span style="color: #004012"><span class="bright">padding</span></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The hamburger menu lines are wide stretched in my scaling 2:<br />
<a href="https://phabricator.kde.org/F5537098" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">F5537098: Screenshot_20171209_231619.png</a><br />
This could be the same problem we had with the ones of plasma-pa. Maybe the solution we did there could apply here too.</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/D9255#inline-41785" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">thumbnailer.cpp:189</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">pos</span> <span style="color: #aa2211">=</span> <span class="n">ctx</span><span style="color: #aa2211">-></span><span class="n">mapToGlobal</span><span class="p">(</span><span class="n">QPointF</span><span class="p">(</span><span style="color: #601200">0</span><span class="p">,</span> <span class="n">ctx</span><span style="color: #aa2211">-></span><span class="n">height</span><span class="p">())).</span><span class="n">toPoint</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The following code positions the menu according to the button. But it still covers half of it. The context menu should not cover the button at all. Can you somehow use PlasmaComponents.Menu? It already has suitable placement code covering special cases (not enough space etc.).</p>
<p style="padding: 0; margin: 8px;">Otherwise copy the placement code from its C++ part and use it here.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R120 Plasma Workspace</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D9255" rel="noreferrer">https://phabricator.kde.org/D9255</a></div></div><br /><div><strong>To: </strong>broulik, Plasma, VDG, ngraham, romangg<br /><strong>Cc: </strong>januz, romangg, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart<br /></div>