Add a utility accessor to get a QUrl from a ResultSet::Result
ClosedPublic

Authored by meven on Oct 17 2019, 3:43 PM.

Details

Summary

Improve API UI to prevent user handling QString to QUrl.
Would prevent error such as D22005 and D24728 and allow to simplify their code

Diff Detail

Repository
R159 KActivities Statistics
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17820
Build 17838: arc lint + arc unit
meven created this revision.Oct 17 2019, 3:43 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 17 2019, 3:43 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Oct 17 2019, 3:43 PM
meven updated this revision to Diff 68162.Oct 17 2019, 3:45 PM

Fix indentation

meven updated this revision to Diff 68163.Oct 17 2019, 3:46 PM

Improve comment

meven added a comment.Oct 17 2019, 3:53 PM

Relates to D16087 as well

meven updated this revision to Diff 68172.Oct 17 2019, 4:53 PM

Add @since

ivan requested changes to this revision.Oct 17 2019, 5:56 PM
ivan added inline comments.
src/resultset.h
78

url or resourceUrl?

I hoped we are not going to have these problems after the death of Nepomuk. Thought file paths for files and urls for everything else would be a sane default. :)

Also, can you add a TODO: KF6 rething the function names for these two.

This revision now requires changes to proceed.Oct 17 2019, 5:56 PM
meven added inline comments.Oct 17 2019, 6:47 PM
src/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 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.

ivan accepted this revision.Oct 17 2019, 7:01 PM

Just add TODO and you are free to push

src/resultset.h
78

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 :)

This revision is now accepted and ready to land.Oct 17 2019, 7:01 PM
meven marked 3 inline comments as done.Oct 17 2019, 7:04 PM
meven updated this revision to Diff 68194.Oct 17 2019, 7:04 PM

Add a KF6 TODO

This revision was automatically updated to reflect the committed changes.