<table><tr><td style="">sitter requested changes to this revision.<br />sitter 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/D21795">View Revision</a></tr></table><br /><div><div><p>Unfortunately I've noticed a huge blocker...<br />
polkit-qt-1 with the <tt style="background: #ebebeb; font-size: 13px;">checkAuthorizationWithDetails</tt> change is not actually released. That needs fixing :| <a href="https://community.kde.org/ReleasingSoftware" class="remarkup-link" target="_blank" rel="noreferrer">https://community.kde.org/ReleasingSoftware</a></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/D21795#inline-130973">View Inline</a><span style="color: #4b4d51; font-weight: bold;">chinmoyr</span> wrote in <span style="color: #4b4d51; font-weight: bold;">AuthBackend.h:61</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">the mac backend modifies <tt style="background: #ebebeb; font-size: 13px;">callerID</tt> later on so I think it was deliberately kept here.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I wonder if we should change that. Right now every backend gets a QBA copy even when they don't need to modify it. So it sounds to me like it should be const& and the mac backend should make a copy on its stack. Not that it matters a great deal though, so if you disagree that's fine 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/D21795#inline-132517">View Inline</a><span style="color: #4b4d51; font-weight: bold;">DBusHelperProxy.cpp:102</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">QDBusMessage</span> <span class="n">message</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">message</span> <span style="color: #aa2211">=</span> <span class="n">QDBusMessage</span><span style="color: #aa2211">::</span><span class="n">createMethodCall</span><span class="p">(</span><span class="n">helperID</span><span class="p">,</span> <span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">"/"</span><span class="p">),</span> <span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">"org.kde.kf5auth"</span><span class="p">),</span> <span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">"performAction"</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">We need to be backwards compatible here. As far as I can tell this is where we call the actual helper binary. The helper binary may have been built with an older version of kauth, so it doesn't necessarily understand the new API.</p>
<p style="padding: 0; margin: 8px;">You could call org.freedesktop.DBus.Introspectable to figure out which method arguments it supports, or possibly the simpler approach is to could with new arguments and if that results in org.freedesktop.DBus.Error.InvalidArgs try again with old arguments before giving up.</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/D21795#inline-132458">View Inline</a><span style="color: #4b4d51; font-weight: bold;">chinmoyr</span> wrote in <span style="color: #4b4d51; font-weight: bold;">Polkit1Backend.cpp:193</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I am not sure what to do here. The commit didn't really change the version and it is not of KF5 either.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">What I mean is <tt style="background: #ebebeb; font-size: 13px;">PolkitQt1::checkAuthorizationWithDetails</tt> was only introduced some months ago, so you can't just assume it's available.<br />
You'll want to guard it with something like</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">#if POLKITQT1_IS_VERSION(0, 113, 0)
authority->checkAuthorizationWithDetails(...
#else
authority->checkAuthorization(...
#endif</pre></div></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/D21795#inline-132518">View Inline</a><span style="color: #4b4d51; font-weight: bold;">Polkit1Backend.h:55</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">bool</span> <span style="color: #004012">actionExists</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span class="n">action</span><span class="p">)</span> <span class="n">override</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QVariantMap</span> <span style="color: #004012">getBackendDetails</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">DetailsMap</span> <span style="color: #aa2211">&</span><span class="n">details</span><span class="p">)</span> <span class="n">override</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">We generally do not use get prefixes.</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/D21795#inline-132521">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kauthaction.h:253</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"> */</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">setDetails</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">DetailsMap</span> <span style="color: #aa2211">&</span><span class="n">details</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">For consistency with the associated getter I would actually call this setDetailsV2 even though technically not necessary.</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/D21795#inline-132519">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kauthaction.h:262</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"> * @return The action's details</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * @deprecated since 5.62, use details() with DetailsMap.</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;">should say V2</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R283 KAuth</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D21795">https://phabricator.kde.org/D21795</a></div></div><br /><div><strong>To: </strong>chinmoyr, apol, bruns, davidedmundson, Frameworks, dfaure, cfeck, sitter<br /><strong>Cc: </strong>ngraham, sitter, mreeves, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns<br /></div>