<table><tr><td style="">ivan accepted this revision.<br />ivan added a comment.<br />This revision is now accepted and ready to land.
</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/D24741">View Revision</a></tr></table><br /><div><div><p>Just add TODO and you are free to push</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/D24741#inline-139738">View Inline</a><span style="color: #4b4d51; font-weight: bold;">meven</span> wrote in <span style="color: #4b4d51; font-weight: bold;">resultset.h:78</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">url</tt> makes more sense to me, no need to decorate it, this is idiomatic KDE/Qt. <tt style="background: #ebebeb; font-size: 13px;">toUrl</tt> might make sense alternatively since it is not a free operation as it is a copy.</p>
<p style="padding: 0; margin: 8px;">I am not too aware of the history around Nepomuk.</p>
<p style="padding: 0; margin: 8px;">File paths for files and urls for everything else is fine internally but the API was not very clear about how to use it.<br />
Given you would need to basically parse the resource to know which one it is, if you did not forget to do it in the first place.<br />
That's why <a href="https://phabricator.kde.org/D22005" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D22005</a> happens.</p>
<p style="padding: 0; margin: 8px;">IMO we would need a type dedicated for file path, that would be a wrapper around QString, something like C++17 <a href="https://en.cppreference.com/w/cpp/filesystem/path" class="remarkup-link" target="_blank" rel="noreferrer">https://en.cppreference.com/w/cpp/filesystem/path</a> or Rust <a href="https://doc.rust-lang.org/std/path/struct.Path.html" class="remarkup-link" target="_blank" rel="noreferrer">https://doc.rust-lang.org/std/path/struct.Path.html</a></p>
<p style="padding: 0; margin: 8px;">While we are at it I could add a isPath() or similar to tell if resource contains a url or a path QDir::isAbsolutePath(resource()) basically.</p>
<p style="padding: 0; margin: 8px;">About KF6 I would suggest resource would return something like std::variant<std::filesystem::path, QString> <a href="https://en.cppreference.com/w/cpp/utility/variant" class="remarkup-link" target="_blank" rel="noreferrer">https://en.cppreference.com/w/cpp/utility/variant</a><br />
Add a std::optionnal<QString> path() and make url std::optionnal<QUrl> could also be interesting.<br />
Can't wait for KF6 C++17 !<br />
I learned a lot of those modern C++ features first in Rust.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Ok, agreed. The reason why I thought the <tt style="background: #ebebeb; font-size: 13px;">resourceUrl</tt> is a better choice is that it is an url of the resource, not of the result. But I agree <tt style="background: #ebebeb; font-size: 13px;">url</tt> is cleaner.</p>
<p style="padding: 0; margin: 8px;">We'll see about the KF6 part. variants/optionals vs a proxy type that converts to QString and QUrl in a correct way :)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R159 KActivities Statistics</div></div></div><br /><div><strong>BRANCH</strong><div><div>master</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D24741">https://phabricator.kde.org/D24741</a></div></div><br /><div><strong>To: </strong>meven, ivan<br /><strong>Cc: </strong>kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>