<table><tr><td style="">graesslin requested changes to this revision.<br />graesslin added a comment.<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/D7369" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>Overall lots of documentation is missing.</p>
<p>From API layout that looks much better now, but we still have a few places where "Unstable" is needlessly in the API. Most important in Display the create method is not forward compatible by not having an enum to describe which interface version should be created.</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/D7369#inline-31574" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">registry.h:527-528</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">zxdg_exporter_v1</span> <span style="color: #aa2211">*</span><span style="color: #004012">bindXdgExporterUnstableV1</span><span class="p">(</span><span style="color: #aa4000">uint32_t</span> <span class="n">name</span><span class="p">,</span> <span style="color: #aa4000">uint32_t</span> <span class="n">version</span><span class="p">)</span> <span style="color: #aa4000">const</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">zxdg_importer_v1</span> <span style="color: #aa2211">*</span><span style="color: #004012">bindXdgImporterUnstableV1</span><span class="p">(</span><span style="color: #aa4000">uint32_t</span> <span class="n">name</span><span class="p">,</span> <span style="color: #aa4000">uint32_t</span> <span class="n">version</span><span class="p">)</span> <span style="color: #aa4000">const</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #74777d">///@}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">please add documentation</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/D7369#inline-31575" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">registry.h:940-941</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">XdgExporter</span> <span style="color: #aa2211">*</span><span style="color: #004012">createXdgExporterUnstable</span><span class="p">(</span><span class="n">quint32</span> <span class="n">name</span><span class="p">,</span> <span class="n">quint32</span> <span class="n">version</span><span class="p">,</span> <span class="n">QObject</span> <span style="color: #aa2211">*</span><span class="n">parent</span> <span style="color: #aa2211">=</span> <span class="n">nullptr</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">XdgImporter</span> <span style="color: #aa2211">*</span><span style="color: #004012">createXdgImporterUnstable</span><span class="p">(</span><span class="n">quint32</span> <span class="n">name</span><span class="p">,</span> <span class="n">quint32</span> <span class="n">version</span><span class="p">,</span> <span class="n">QObject</span> <span style="color: #aa2211">*</span><span class="n">parent</span> <span style="color: #aa2211">=</span> <span class="n">nullptr</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #74777d">///@}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">please add documentation</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/D7369#inline-31576" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">registry.h:940-941</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">XdgExporter</span> <span style="color: #aa2211">*</span><span style="color: #004012">createXdgExporterUnstable</span><span class="p">(</span><span class="n">quint32</span> <span class="n">name</span><span class="p">,</span> <span class="n">quint32</span> <span class="n">version</span><span class="p">,</span> <span class="n">QObject</span> <span style="color: #aa2211">*</span><span class="n">parent</span> <span style="color: #aa2211">=</span> <span class="n">nullptr</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">XdgImporter</span> <span style="color: #aa2211">*</span><span style="color: #004012">createXdgImporterUnstable</span><span class="p">(</span><span class="n">quint32</span> <span class="n">name</span><span class="p">,</span> <span class="n">quint32</span> <span class="n">version</span><span class="p">,</span> <span class="n">QObject</span> <span style="color: #aa2211">*</span><span class="n">parent</span> <span style="color: #aa2211">=</span> <span class="n">nullptr</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #74777d">///@}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Why did you keep the suffix "Unstable"?</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/D7369#inline-31577" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">registry.h:1132-1133</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 style="color: #004012">exporterUnstableV1Announced</span><span class="p">(</span><span class="n">quint32</span> <span class="n">name</span><span class="p">,</span> <span class="n">quint32</span> <span class="n">version</span><span class="p">);</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">importerUnstableV1Announced</span><span class="p">(</span><span class="n">quint32</span> <span class="n">name</span><span class="p">,</span> <span class="n">quint32</span> <span class="n">version</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #74777d">///@}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">please add documentation</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/D7369#inline-31578" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">registry.h:1288-1290</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 style="color: #004012">exporterUnstableV1Removed</span><span class="p">(</span><span class="n">quint32</span> <span class="n">name</span><span class="p">);</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">importerUnstableV1Removed</span><span class="p">(</span><span class="n">quint32</span> <span class="n">name</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #74777d">///@}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">please add documentation</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/D7369#inline-31579" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">display.h:223</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">XdgForeignInterface</span> <span style="color: #aa2211">*</span><span style="color: #004012">createXdgForeignUnstableInterface</span><span class="p">(</span><span class="n">QObject</span> <span style="color: #aa2211">*</span><span class="n">parent</span> <span style="color: #aa2211">=</span> <span class="n">nullptr</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #74777d">/**</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">please add documentation.</p>
<p style="padding: 0; margin: 8px;">Also the common way would be to have an enum to describe which interface should be created, but therefore drop the Unstable from the API name.</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/D7369#inline-31580" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">xdgforeign_interface.h:37</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">class</span> <span class="n">XdgImporterUnstableV1Interface</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">class</span> <span class="n">KWAYLANDSERVER_EXPORT</span> <span style="color: #a0a000">XdgForeignInterface</span> <span class="p">:</span> <span class="n">public</span> <span class="n">QObject</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Something like an enum ForeignInterfaceVersion is missing, please compare TextInputInterface</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/D7369#inline-31581" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">xdgforeign_v1_interface.cpp:260</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">QPointer</span><span style="color: #aa2211"><</span><span class="n">XdgImportedUnstableV1Interface</span><span style="color: #aa2211">></span> <span class="n">imp</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">XdgImportedUnstableV1Interface</span><span class="p">(</span><span class="n">s</span><span style="color: #aa2211">-></span><span class="n">q</span><span class="p">,</span> <span class="n">surface</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">imp</span><span style="color: #aa2211">-></span><span class="n">create</span><span class="p">(</span><span class="n">s</span><span style="color: #aa2211">-></span><span class="n">display</span><span style="color: #aa2211">-></span><span class="n">getConnection</span><span class="p">(</span><span class="n">client</span><span class="p">),</span> <span class="n">wl_resource_get_version</span><span class="p">(</span><span class="n">resource</span><span class="p">),</span> <span class="n">id</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">what's an imp? Could you please write full variable names?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R127 KWayland</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D7369" rel="noreferrer">https://phabricator.kde.org/D7369</a></div></div><br /><div><strong>To: </strong>mart, Plasma, KWin, davidedmundson, graesslin<br /><strong>Cc: </strong>davidedmundson, graesslin, plasma-devel, Frameworks, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart<br /></div>