<table><tr><td style="">davidedmundson 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/D27861">View Revision</a></tr></table><br /><div><div><p>We should be discussing big changes before doing the full implementation. It's quite frustrating.</p>

<p>RE: Kwayland<br />
Yes there are definitely some problems and it needs some directional planning and changes. That needs a broader mailing list discussion, it shouldn't be done within the context of any specific patch.<br />
You've been promising to send an email on that for weeks now :(</p>

<hr class="remarkup-hr" />

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>The new wrappers weren't implemented in<br />
KWayland because I cannot guarantee that the future versions of the<br />
wrappers will be API compatible.</p></blockquote>

<p>Whilst I do agree with the overall comments on kwayland, this specific case is a bad example.</p>

<p>WMBase is finally stable API. Any new changes within WMBase versions have to be backwards compatible at a protocol level. Those have proved to be relatively easy to maintain compatibility at a library level.<br />
XdgShellV6 is obviously also stable at this point.</p>

<p>We know that XdgShellV6(v1) will /always/ be basically identical to XdgWmBase(v1). As XdgWmBase goes through different versions, we will be keeping an abstraction layer inside XdgShellClient back to v1.<br />
It seems very backwards that we've successfully fought an abstraction layer over something unstable, then removed it the instant things are actually stable!</p>

<hr class="remarkup-hr" />

<p>Ending on some positive things:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">I like the split of XdgTopLevel and XdgPopup that 100% makes sense. Also paves the way for future roles nicely.</li>
<li class="remarkup-list-item">We probably do want to redo our interface side to be based around WMBase rather than XdgShellV5. A rewrite probably was needed.</li>
<li class="remarkup-list-item">I like use of Qt's wayland scanner. It is a lot easier to read.</li>
</ul></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/D27861#inline-157388">View Inline</a><span style="color: #4b4d51; font-weight: bold;">xdgshellclient_test.cpp:1417</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 class="n">QCOMPARE</span><span class="p">(</span><span class="n">client</span><span style="color: #aa2211">-></span><span class="n">frameGeometry</span><span class="p">().</span><span class="n">topLeft</span><span class="p">(),</span> <span class="n">oldPosition</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);">    <span class="n">QCOMPARE</span><span class="p">(</span><span class="n">client</span><span style="color: #aa2211">-></span><span class="n">frameGeometry</span><span class="p">().</span><span class="n">size</span><span class="p">(),</span> <span class="n">QSize</span><span class="p">(<span class="bright"></span></span><span class="bright"></span><span style="color: #601200"><span class="bright">100</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span style="color: #601200"><span class="bright">5</span>0</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">QCOMPARE</span><span class="p">(</span><span class="n">client</span><span style="color: #aa2211">-></span><span class="n">frameGeometry</span><span class="p">().</span><span class="n">size</span><span class="p">(),</span> <span class="n">QSize</span><span class="p">(<span class="bright"></span></span><span class="bright"></span><span style="color: #601200"><span class="bright">90</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span style="color: #601200"><span class="bright">4</span>0</span><span class="p">));</span>
</div><div style="padding: 0 8px; margin: 0 4px; ">    <span class="n">QCOMPARE</span><span class="p">(</span><span class="n">client</span><span style="color: #aa2211">-></span><span class="n">bufferGeometry</span><span class="p">().</span><span class="n">topLeft</span><span class="p">(),</span> <span class="n">oldPosition</span> <span style="color: #aa2211">-</span> <span class="n">QPoint</span><span class="p">(</span><span style="color: #601200">10</span><span class="p">,</span> <span style="color: #601200">10</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This needs explanation.</p>

<p style="padding: 0; margin: 8px;">The one thing that makes such a big change somewhat safe is the extensive unit tests. <br />
If we change them at the same time, then we've lost all that.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R108 KWin</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D27861">https://phabricator.kde.org/D27861</a></div></div><br /><div><strong>To: </strong>zzag, KWin<br /><strong>Cc: </strong>davidedmundson, kwin, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, hardening, romangg, jensreuterberg, abetts, sebas, apol, ahiemstra, mart<br /></div>