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
Details
Details
- Reviewers
vhanda ngraham dfaure - Group Reviewers
Frameworks Dolphin - Commits
- R293:16664fccb7b7: Strip down and re-write the baloo tags KIO slave
Diff Detail
Diff Detail
- Repository
- R293 Baloo
- Branch
- master-nestedTags (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
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?
Comment Actions
Maybe update your local repo? It is against the most recent commit in master. It was diff'ed from a non-master branch...
Comment Actions
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. :)
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
- Stricter url validation.
- Fix deep copies.
This revision reverts the preview fix. There is an open bug (https://bugs.kde.org/show_bug.cgi?id=194181) regarding dolphin/KIO leaving around partial copies.
| src/kioslaves/tags/kio_tags.cpp | ||
|---|---|---|
| 63 ↗ | (On Diff #20362) |
|
| 130 ↗ | (On Diff #20362) | missing KIO::Overwrite support? Can the dest already exist? |
| 172–175 ↗ | (On Diff #20362) | And otherwise? what happens if it's neither TagFileUrl nor LocalUrl? should error(not supported) be called? The job would never finish otherwise. |
| 181 ↗ | (On Diff #20362) | same question, can dest already exist? |
| 237 ↗ | (On Diff #20362) | the QString() isn't needed (repeats) |
| 276 ↗ | (On Diff #20362) | missing return, to avoid emitting finished() twice |
| 305 ↗ | (On Diff #20362) | can this fail? |
| 329 ↗ | (On Diff #20362) | This conversion from bool to int sounds ... fragile. + (result.tag.isEmpty() ? 0 : 1) |
| 333 ↗ | (On Diff #20362) | tagSection isn't used by the if condition itself, so it could move into the if(). |
| 334 ↗ | (On Diff #20362) | The && !result.tag.isEmpty() is useless. |
| 347 ↗ | (On Diff #20362) | That's better written as if (url.isLocalFile()) |
| 353 ↗ | (On Diff #20362) | same question as before |
| 358 ↗ | (On Diff #20362) | KIO::stat never returns nullptr, so the if() isn't useful. |
| 372 ↗ | (On Diff #20362) | 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.). But maybe split is fine here, I guess it's likely not time critical. |
Comment Actions
- Check if the destination file already has the tag.
- Simplify file query creation.
Review fixes.
Comment Actions
- Merge local and tag file handling.
- Create the tag directory entries and validate the tag at the same time.
| src/kioslaves/tags/kio_tags.cpp | ||
|---|---|---|
| 130 ↗ | (On Diff #20362) | I see an added check for "already exists" and an early return (good), but no support for KIO::Overwrite. If the user uses dolphin to copy a tag they might click on "overwrite" and it won't work, if copy() just ignores KIO::Overwrite. |
| 171–172 ↗ | (On Diff #20362) | Could be another "else if" for symmetry, or better, this could all be a switch statement. It still seems odd to me that if it's not invalid, tag, or local, then get() finishes without an error, is that expected? |
| 181 ↗ | (On Diff #20362) | same issue here, still no support for Overwrite. |
| 291 ↗ | (On Diff #20362) | No you don't ;) This comment confused me. So in fact rewriteUrl() is never called, AFAICS. You could verify that with a Q_ASSERT(false) in here, and extensive testing ;-) Maybe don't commit that though. But at least, if I'm right, change the comment to "So rewriteUrl is never called.". |
| 305 ↗ | (On Diff #20362) | This was a rethorical question :-) |
| 372 ↗ | (On Diff #20362) | Would it be simpler to use QDir::cleanPath() for all this path cleaning? Just an idea, ignore if bogus. |
| src/kioslaves/tags/kio_tags.cpp | ||
|---|---|---|
| 130 ↗ | (On Diff #20362) | KIO::Overwrite is not usable here, because the calls are not relayed to the forwarding slave base. Silently failing is good enough here, IMHO. There's no user action required to recover when a file already has a tag, and no data loss. A popup is unjustifiably intrusive. |
| 305 ↗ | (On Diff #20362) | Sorry, I though that I was using start() here, and obviously missed Baloo::TagJob's inherited members.. |
| src/kioslaves/tags/kio_tags.cpp | ||
|---|---|---|
| 265–266 ↗ | (On Diff #20362) | You can use boolean conditions rather than if statements, example validTag = validTag || tag.startWith(...) Why?
|
| 269 ↗ | (On Diff #20362) | Better reverse conditions if (chopLastSection ||...) You don't need expensive call like contains |
| src/kioslaves/tags/kio_tags.h | ||
| 99 ↗ | (On Diff #20362) | metaData(QString{}) |
| 101–102 ↗ | (On Diff #20362) | Use enum rather than boolean trap |
Comment Actions
Currently known issues:
- Deep copying a tag tree with a tagged folder with multiple recursed entries to the local filesystem places all of the tagged folder's recursed items into the root of the destination.
Comment Actions
Excellent! @smithjd, is this ready to go in now, or are you still making more changes?
Comment Actions
- 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.
- Merge local and tag file handling.
- Make io slave work with kioclient.
- Review changes.
- Review suggestions.
- Fix valid tag calculation.
- Fix for slash-less tag urls.
- Fix tagged directory listing.
Comment Actions
There are still known problems with tagged directory copies. Overall however it's in a pretty good state and these can probably be left until later. I'll look more into the directory copies after merging this into master.