Add Trash class to handle all trash operations.
Details
Diff Detail
- Repository
- R318 Dolphin
- Branch
- add-trash-class (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage
Thanks for making it easier to review and the Git history tidier ;)
@rominf Does this patch now contain the bugfix? If so, you could split the commit message too. In any case, add something to the summary and the test plan.
src/trash/dolphintrash.cpp | ||
---|---|---|
65–68 | I know i said this before, but i now think that i'm wrong in that assumption. The itemsDeleted (and itemsAdded) signal is a response to a change in a folder where you created the KDirLister job on and havent destroyed it yet. To be honest, i didn't know (till a dayago) that KDirLister was internally watching a dir for changes till you kill the object. It's basically a KDirWarch within KDirLister hidden away in a few signals. Kinda neat really :) So it's not a bug. It just smelled like it :) |
src/trash/dolphintrash.cpp | ||
---|---|---|
59–69 | This is slightly odd. Also, the m_trashDirLister should be initialized in the constructor like so: Trash::Trash() : m_trashDirLister(new KDirLister()) { // .. } In my opinion it's fine to move everything in the constructor. |
I think that's about it though. I have no further requests, the code looks OK to me (i did not test it).
After this last comment it's a +1 from me.
I do prefer if someone else also gives a +1 before you push it. @rkflx perhaps?
src/trash/dolphintrash.cpp | ||
---|---|---|
31–32 | Now you still need to fix the indentation ;) the semicolon should be on the line below and indentation should be 4 spaces. |
Uh oh, testing is sometimes kinda important ;)
After this last comment it's a +1 from me.
Note that you can "resign" if you don't want to "accept" after "requesting changes", so arc land won't block.
I do prefer if someone else also gives a +1 before you push it. @rkflx perhaps?
Sorry, I'm currently not available for in-depth code review in Dolphin, only some random UI comments in-between…
Right, then i will test more thoroughly.
I don't want to resign, i want to give this a +1. When it's ready that is :)
Verified that this fixes the bug with the following cases:
- One file, folder, or link in trash -> Restore -> icon updated correctly
- Mixed group of files, folders, and links in trash -> Select all -> Restore -> icon updated correctly
- Mixed group of files, folders, and links in trash -> Select one -> Restore -> icon correctly not updated
- Mixed group of files, folders, and links in trash -> Restore them one at a time -> icon correctly updated after last item restored
Approved from the functionality and "works as advertised" department. Wait for the final code review by the Dolphin maintainers before pushing, please!
First of all, please read https://community.kde.org/Policies/Commit_Policy#Always_add_descriptive_log_messages
At a first glance, I only see code moved around and I don't understand where the actual bugfix is.
The commit message should describe what the problem is and how the commit is going to fix it.
Done. Understood.
At a first glance, I only see code moved around and I don't understand where the actual bugfix is.
The commit message should describe what the problem is and how the commit is going to fix it.
OK. This patch had 2 aims, that is to refactor the trash code and fix the bug. I split the patch and explicitly stated the purpose of this patch (refactoring). Is this OK now?
src/trash/dolphintrash.cpp | ||
---|---|---|
31–32 | OK, but... It's the same as in files "informationpanelcontent.cpp", "filemetadataconfigurationdialog.cpp", "trashsettingspage.cpp", etc. Is it worth to make semicolon consistent in another patch? |
I think it gets complicated now.
This patch and D11216 are now both not correct anymore (they are both the same, both do too much).
Reduce this patch to just the Trash addition. Just the refactoring that is needed for that.
So no icon changes in here.
Change D11216 to only show the icon changes needed.
Don't give up now, you're nearly there :)
It's already true. If you are talking about the code that updates the icon with the notification signal, it's the part of refactoring. No logic changes involved.
Change D11216 to only show the icon changes needed.
I will, if this patch will be accepted. Refactoring is required to fix this bug, that's why I made this patch to a parent of D11216. So, first accept this patch, then D11216.
Is there the way to rebase the patch on the patch that is not accepted yet? I mean on the left side of the patch D11216 I see the code from the patch D11012 (but not from origin/master), on the right side I see lines from the patch D11216.
Don't give up now, you're nearly there :)
I don't.
In D10804#218013 I linked to our documentation which has detailed examples and exact commands on how to create dependent revisions. There is also a section on how to split patches. I assume the problem is that your local setup is different from what the wiki tells you to do, i.e. you probably missed to properly set the tracking branches.
Arcanist by default diffs against the tracking branch, which for you apparently is master, that's why your child Diffs contain changes of parent Diffs . Either fix the tracking branch config, or tell arc diff to only include specific commits (see here).
If that's too complicated, you might as well start from scratch, following exactly the docs (which I can assure you will work just fine, as can be seen here). You can reuse the commit messages and include the Differential Revision: line to tell arc to update the Diff instead of creating a new one.
Please do the next patch in a way that we can actually see what's happening, that's for D11216. I'd recommend following what @rkflx said for followup patches.
For this patch, i did actually test it now and it works as advertised. Aka, it works as it did before, only now with the Trash class.
I'm fine with shipping it. @elvisangelaccio, do you have more comments or a +1 otherwise?
Note: I'm not a Dolphin maintainer! But if multiple people agree with this change it's imho fine as well.
src/panels/places/placesitem.cpp | ||
---|---|---|
24 | Not needed, please remove. | |
src/panels/places/placespanel.cpp | ||
433 | Please don't discard this comment, let's keep it even in the new file. | |
src/trash/dolphintrash.cpp | ||
3 | Please also add Copyright (C) 2012 by Peter Penz <peter.penz19@gmail.com> who is the original author of a part of this code. | |
src/trash/dolphintrash.h | ||
41–42 | These methods don't use m_trashDirLister, right? => they should be static | |
47–49 | Why protected instead of private? |
src/dolphincontextmenu.cpp | ||
---|---|---|
143 | This could just be Trash::isEmpty() (same with the other static methods). |
@elvisangelaccio with your suggestions the generic methods are basically handled like if it were a namespace.
The only reason for the class to exist now is because of the signal in there.
Oh well, this review is finally done now :) On to the next one ;)