ivan accepted this revision. ivan added a comment. This revision is now accepted and ready to land.
Just add TODO and you are free to push INLINE COMMENTS > meven wrote in resultset.h:78 > `url` makes more sense to me, no need to decorate it, this is idiomatic > KDE/Qt. `toUrl` might make sense alternatively since it is not a free > operation as it is a copy. > > I am not too aware of the history around Nepomuk. > > File paths for files and urls for everything else is fine internally but the > API was not very clear about how to use it. > 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. > That's why D22005 <https://phabricator.kde.org/D22005> happens. > > IMO we would need a type dedicated for file path, that would be a wrapper > around QString, something like C++17 > https://en.cppreference.com/w/cpp/filesystem/path or Rust > https://doc.rust-lang.org/std/path/struct.Path.html > > 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. > > About KF6 I would suggest resource would return something like > std::variant<std::filesystem::path, QString> > https://en.cppreference.com/w/cpp/utility/variant > Add a std::optionnal<QString> path() and make url std::optionnal<QUrl> could > also be interesting. > Can't wait for KF6 C++17 ! > I learned a lot of those modern C++ features first in Rust. Ok, agreed. The reason why I thought the `resourceUrl` is a better choice is that it is an url of the resource, not of the result. But I agree `url` is cleaner. We'll see about the KF6 part. variants/optionals vs a proxy type that converts to QString and QUrl in a correct way :) REPOSITORY R159 KActivities Statistics BRANCH master REVISION DETAIL https://phabricator.kde.org/D24741 To: meven, ivan Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
