<table><tr><td style="">aheinecke requested changes to this revision.<br />aheinecke 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/D6516" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>Looks good in general to me apart from my inline comments. The filter stuff I will test now because I can't see from the code if it works as intended.</p>

<p>Regarding style: Please replace all tabs with 4 spaces. And we don't have a space between "!" and the statement.</p>

<p>The Style is documented under:<br />
<a href="https://techbase.kde.org/Policies/Kdelibs_Coding_Style" class="remarkup-link" target="_blank" rel="noreferrer">https://techbase.kde.org/Policies/Kdelibs_Coding_Style</a></p>

<p>It even has an Emacs Mode for KDE ;-P</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/D6516#inline-26931" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">justusw</span> wrote in <span style="color: #4b4d51; font-weight: bold;">formatting.cpp:825</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">So obviously, a predicate is not about formatting, so it shouldn't reside here.  I thought about adding a module, e.g. 'compliance' or something, but I did not feel confident enough to do so and decide how to name it and where to put it.</p>

<p style="padding: 0; margin: 8px;">Guidance appreciated.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I think it's ok to have it in Formatting: isKeyValid -> Should we display this as a valid key? Is kind of formatting for me.</p>

<p style="padding: 0; margin: 8px;">A better place might be GpgMEpp directly, we might add it there but still add it here to avoid a new round of release / dependency bump.</p>

<p style="padding: 0; margin: 8px;">I'd like you to rename the function though to something like "uidsHaveFullValidity" to avoid confusion because "valid" in other contexts is used as some kind of "is key usable" (e.g. not expired, not revoked, etc.)" and the use of just "valid" tends to be confusing.</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/D6516#inline-26955" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">formatting.cpp:829</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">for</span> <span class="p">(</span><span style="color: #aa4000">const</span> <span style="color: #aa4000">auto</span> <span style="color: #aa2211">&</span><span style="color: #a0a000">uid</span><span class="p">:</span> <span class="n">key</span><span class="p">.</span><span class="n">userIDs</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">  <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">uid</span><span class="p">.</span><span class="n">validity</span><span class="p">()</span> <span style="color: #aa2211">>=</span> <span class="n">UserID</span><span style="color: #aa2211">::</span><span class="n">Validity</span><span style="color: #aa2211">::</span><span class="n">Full</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">      <span class="n">fullyTrusted</span><span style="color: #aa2211">++</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I think it's ok for a key not to have any user id. In that case this function would return true.<br />
Also I think we need to skip revoked or expired UserID's here similar to isDeVs</p>

<p style="padding: 0; margin: 8px;">As a minor nitpick I would just return false once a uid.validity() < UserID::Validity::Full is found.</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/D6516#inline-26959" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">formatting.cpp:885</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="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">unsigned</span> <span style="color: #aa4000">int</span> <span class="n">fullyTrusted</span> <span style="color: #aa2211">=</span> <span style="color: #601200">0</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">const</span> <span style="color: #aa4000">auto</span> <span style="color: #aa2211">&</span><span style="color: #a0a000">uid</span><span class="p">:</span> <span class="n">key</span><span class="p">.</span><span class="n">userIDs</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;">I think we can use the now called isKeyValid function for this.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R90 PIM: Kleo Library</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D6516" rel="noreferrer">https://phabricator.kde.org/D6516</a></div></div><br /><div><strong>To: </strong>justusw, aheinecke<br /><strong>Cc: </strong>KDE PIM, dvasin, winterz, vkrause, mlaurent, knauss, dvratil<br /></div>