D8098: Strip down and re-write the tags KIO slave.
smithjd updated this revision to Diff 22107. smithjd added a comment. - Merge local and tag file handling. REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8098?vs=21851=22107 BRANCH master-nestedTags (branched from master) REVISION DETAIL https://phabricator.kde.org/D8098 AFFECTED FILES src/kioslaves/tags/kio_tags.cpp src/kioslaves/tags/kio_tags.h src/kioslaves/tags/tags.protocol To: smithjd, #frameworks, vhanda, #dolphin, ngraham, dfaure Cc: dfaure, nicolasfella, ngraham
D8098: Strip down and re-write the tags KIO slave.
smithjd marked 14 inline comments as done. smithjd added inline comments. INLINE COMMENTS > dfaure wrote in kio_tags.cpp:64 > - merge with previous line > > - can path() really contain '?' ? I thought that wasn't possible (? delimits > the query). Yes, the url is non-hierarchical scheme, so everything is in the path. > dfaure wrote in kio_tags.cpp:292 > can this fail? No. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D8098 To: smithjd, #frameworks, vhanda, #dolphin, ngraham, dfaure Cc: dfaure, nicolasfella, ngraham
D8098: Strip down and re-write the tags KIO slave.
smithjd updated this revision to Diff 21851. smithjd added a comment. - Fix generating previews copies the file to tmp. - Fix tag navigation display labels in Dolphin. - Stricter url validation. - Create a full url instead of only a path. - Check if the destination file already has the tag. 1. Updating https://phabricator.kde.org/D8098: Strip down and re-write the tags KIO slave. # 2. Enter a brief description of the changes included in this update. 3. The first line is used as subject, next lines as comment. # 4. If you intended to create a new revision, use: 5. $ arc diff --create Review fixes. REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8098?vs=21599=21851 BRANCH master-nestedTags (branched from master) REVISION DETAIL https://phabricator.kde.org/D8098 AFFECTED FILES src/kioslaves/tags/kio_tags.cpp src/kioslaves/tags/kio_tags.h src/kioslaves/tags/tags.protocol To: smithjd, #frameworks, vhanda, #dolphin, ngraham, dfaure Cc: dfaure, nicolasfella, ngraham
D8098: Strip down and re-write the tags KIO slave.
dfaure added inline comments.
INLINE COMMENTS
> kio_tags.cpp:64
> +QString tag;
> +tag = url.path().section(QLatin1Char('?'), 0, 0);
> +while (tag.startsWith(QDir::separator()))
- merge with previous line
- can path() really contain '?' ? I thought that wasn't possible (? delimits
the query).
> kio_tags.cpp:119
> +Q_UNUSED(permissions);
> +Q_UNUSED(flags);
>
missing KIO::Overwrite support? Can the dest already exist?
> kio_tags.cpp:156
>
> -case FileUrl:
> -ForwardingSlaveBase::get(QUrl::fromLocalFile(fileUrl));
> -return;
> +if ((result.urlType == TagFileUrl) || (result.urlType == LocalUrl)) {
> +ForwardingSlaveBase::get(QUrl::fromLocalFile(result.filePath));
And otherwise? what happens if it's neither TagFileUrl nor LocalUrl? should
error(not supported) be called? The job would never finish otherwise.
> kio_tags.cpp:163
> {
> -Q_UNUSED(permissions);
> Q_UNUSED(flags);
>
same question, can dest already exist?
(if so, then the right thing to do is error(), unless `flags & Overwrite`)
> kio_tags.cpp:203
> +for (const QString& tag : md.tags()) {
> +if (tag == srcResult.tag ||
> (tag.startsWith(QString(srcResult.tag + QLatin1Char('/') {
> +QString newTag = tag;
the QString() isn't needed (repeats)
> kio_tags.cpp:262
> +} else {
> +ForwardingSlaveBase::mimetype(QUrl::fromLocalFile(result.filePath));
> }
missing return, to avoid emitting finished() twice
(like you did after the other calls to ForwardingSlaveBase)
> kio_tags.cpp:292
> +TagListJob* tagJob = new TagListJob();
> +tagJob->exec();
>
can this fail?
> kio_tags.cpp:331
> +// Extract any file path and determine tag from url
> +if (url.scheme() == QLatin1String("file")) {
> +result.urlType = LocalUrl;
That's better written as if (url.isLocalFile())
> kio_tags.cpp:337
> +bool validTag = relaxValidation;
> +if (url.path().contains(QLatin1Char('?')) || chopLastSection) {
> +QUrl choppedUrl = url;
same question as before
> kio_tags.cpp:356
> +QString query;
> +const QStringList tagSections =
> result.tag.split(QLatin1Char('/'));
> +for (int i = 0; i < tagSections.size(); ++i) {
could this whole splitting be optimized by saying something like this?
result.tag.prepend("tag:");
result.tag.replace('/', " AND tag:");
It's just that split is slow (allocates a temporary container etc.).
If this is called often, and the above isn't good enough, then something like
what I did in kcontacts, commit
https://phabricator.kde.org/R174:5de361ff80ce0a015b62a3a76fe676355d8d3a52, can
be used.
But maybe split is fine here, I guess it's likely not time critical.
> kio_tags.cpp:393
> +result.pathUDSResults << createUDSEntryForTag(QStringLiteral("."),
> result.tag);
> +int index = result.tag.count(QLatin1Char('/')) + !result.tag.isEmpty();
> +QStringList tagPaths;
This conversion from bool to int sounds ... fragile.
I would do
+ (result.tag.isEmpty() ? 0 : 1)
> kio_tags.cpp:397
> +for (const QString& tag : tags) {
> +QString tagSection = tag.section(QLatin1Char('/'), index, index,
> QString::SectionSkipEmpty);
> +if (result.tag.isEmpty() || (tag.startsWith(result.tag,
> Qt::CaseInsensitive) && !result.tag.isEmpty())) {
`tagSection` isn't used by the if condition itself, so it could move into the
if().
> kio_tags.cpp:398
> +QString tagSection = tag.section(QLatin1Char('/'), index, index,
> QString::SectionSkipEmpty);
> +if (result.tag.isEmpty() || (tag.startsWith(result.tag,
> Qt::CaseInsensitive) && !result.tag.isEmpty())) {
> +if (!tagPaths.contains(tagSection, Qt::CaseInsensitive) &&
> !tagSection.isEmpty()) {
The `&& !result.tag.isEmpty()` is useless.
If we're evaluating that part of the condition (the part after the "||"), then
result.tag is NOT empty, by definition.
> kio_tags.cpp:422
> +KIO::UDSEntry uds;
> +if (KIO::StatJob* job = KIO::stat(match, KIO::HideProgressInfo)) {
> +// we do not want to wait for the event loop to delete the job
KIO::stat never returns nullptr, so the if() isn't useful.
REPOSITORY
R293 Baloo
BRANCH
master-nestedTags (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D8098
To: smithjd, #frameworks, vhanda, #dolphin, ngraham
Cc: dfaure, nicolasfella, ngraham
D8098: Strip down and re-write the tags KIO slave.
dfaure requested changes to this revision. This revision now requires changes to proceed. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D8098 To: smithjd, #frameworks, vhanda, #dolphin, ngraham, dfaure Cc: dfaure, nicolasfella, ngraham
D8098: Strip down and re-write the tags KIO slave.
smithjd updated this revision to Diff 21599. smithjd added a comment. - Create a full url instead of only a path. REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8098?vs=21590=21599 BRANCH master-nestedTags (branched from master) REVISION DETAIL https://phabricator.kde.org/D8098 AFFECTED FILES src/kioslaves/tags/kio_tags.cpp src/kioslaves/tags/kio_tags.h src/kioslaves/tags/tags.protocol To: smithjd, #frameworks, vhanda, #dolphin, ngraham Cc: nicolasfella, ngraham
D8098: Strip down and re-write the tags KIO slave.
smithjd updated this revision to Diff 21590. smithjd added a comment. - Fix generating previews copies the file to tmp. - Fix tag navigation display labels in Dolphin. - Stricter url validation. 1. Updating https://phabricator.kde.org/D8098: Strip down and re-write the tags KIO slave. # 2. Enter a brief description of the changes included in this update. 3. The first line is used as subject, next lines as comment. # 4. If you intended to create a new revision, use: 5. $ arc diff --create This revision reverts the preview fix. There is an open bug (https://bugs.kde.org/show_bug.cgi?id=194181) about dolphin/KIO leaving around temp files. REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8098?vs=20362=21590 BRANCH master-nestedTags (branched from master) REVISION DETAIL https://phabricator.kde.org/D8098 AFFECTED FILES src/kioslaves/tags/kio_tags.cpp src/kioslaves/tags/kio_tags.h src/kioslaves/tags/tags.protocol To: smithjd, #frameworks, vhanda, #dolphin, ngraham Cc: nicolasfella, ngraham
D8098: Strip down and re-write the tags KIO slave.
smithjd added a comment. Deep tag copies (to the filesystem and also in the slave) are known to be broken (1 or more folder deep) because of the preview fix in the first revision. REPOSITORY R293 Baloo BRANCH master-nestedTags (branched from master) REVISION DETAIL https://phabricator.kde.org/D8098 To: smithjd, #frameworks, vhanda, #dolphin, ngraham Cc: nicolasfella, ngraham
D8098: Strip down and re-write the tags KIO slave.
ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. I have been using this for weeks without any negative effects. I'm giving this my blessing, but let's wait for others to weigh in. REPOSITORY R293 Baloo BRANCH master-nestedTags (branched from master) REVISION DETAIL https://phabricator.kde.org/D8098 To: smithjd, #frameworks, vhanda, #dolphin, ngraham Cc: nicolasfella, ngraham
D8098: Strip down and re-write the tags KIO slave.
ngraham added a comment. Can you add some testing details? REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D8098 To: smithjd, #frameworks, vhanda, #dolphin Cc: nicolasfella, ngraham
D8098: Strip down and re-write the tags KIO slave.
ngraham added a reviewer: Dolphin. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D8098 To: smithjd, #frameworks, vhanda, #dolphin Cc: nicolasfella, ngraham
D8098: Strip down and re-write the tags KIO slave.
smithjd updated this revision to Diff 20362. smithjd added a comment. - Fix tag navigation display labels in Dolphin. REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8098?vs=20320=20362 BRANCH master-nestedTags (branched from master) REVISION DETAIL https://phabricator.kde.org/D8098 AFFECTED FILES src/kioslaves/tags/kio_tags.cpp src/kioslaves/tags/kio_tags.h src/kioslaves/tags/tags.protocol To: smithjd, #frameworks, vhanda Cc: ngraham
D8098: Strip down and re-write the tags KIO slave.
smithjd updated this revision to Diff 20320. smithjd added a comment. - Fix generating previews copies the file to tmp. REPOSITORY R293 Baloo CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8098?vs=20222=20320 BRANCH master-nestedTags (branched from master) REVISION DETAIL https://phabricator.kde.org/D8098 AFFECTED FILES src/kioslaves/tags/kio_tags.cpp src/kioslaves/tags/kio_tags.h src/kioslaves/tags/tags.protocol To: smithjd, #frameworks, vhanda Cc: ngraham
D8098: Strip down and re-write the tags KIO slave.
smithjd added a comment. Copying/cutting file tags works both from local files and also tagged files. Breaking tags works, e.g. nested tag foo in bar can be copied or cut to foobar/barfoo tag path. Cutting and pasting a file folder results in a new tag on the folder, copying applies the destination path + file path as a tag to all files recursively inside that folder. Deleting tags removes the tag from the file or directory. Making new directories / tags is possible, entering the directory results in an error and the directory disappears. Copying or cutting tags into this directory must happen before it's entered. Previews work, and a bug that caused nested tags to show up in similary named tag folders was also fixed. REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D8098 To: smithjd, #frameworks, vhanda Cc: ngraham
D8098: Strip down and re-write the tags KIO slave.
ngraham added a reviewer: vhanda. ngraham added a comment. Oh, I'm a dolt. I was trying to apply this to KIO, not Baloo. Ignore me; works fine when you're not holding it wrong. :) REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D8098 To: smithjd, #frameworks, vhanda Cc: ngraham
D8098: Strip down and re-write the tags KIO slave.
smithjd added a comment. Maybe update your local repo? It is against the most recent commit in master. It was diff'ed from a non-master branch... REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D8098 To: smithjd, #frameworks Cc: ngraham
D8098: Strip down and re-write the tags KIO slave.
ngraham added a comment. Fantastic work. I wasn't able to apply it with `arc`, though: This diff is against commit 9383ea8927579555301eb378c1ce299dde2a2d08, but the commit is nowhere in the working copy. Try to apply it against the current working copy state? (e58d6ab06561e455576375e03cb25dd18fd4089f) Can you rebase the diff against current git master? Also, can you add some details of your testing? REPOSITORY R293 Baloo REVISION DETAIL https://phabricator.kde.org/D8098 To: smithjd, #frameworks Cc: ngraham
D8098: Strip down and re-write the tags KIO slave.
smithjd created this revision. smithjd added a reviewer: Frameworks. Restricted Application added a project: Frameworks. REVISION SUMMARY Allow for more operations on tag trees (e.g. rename, copy, delete) and fix some existing issues. BUG: 314373 BUG: 340098 BUG: 376229 BUG: 332214 BUG: 340099 REPOSITORY R293 Baloo BRANCH master-nestedTags (branched from master) REVISION DETAIL https://phabricator.kde.org/D8098 AFFECTED FILES src/kioslaves/tags/kio_tags.cpp src/kioslaves/tags/kio_tags.h src/kioslaves/tags/tags.protocol To: smithjd, #frameworks
