Add Trash (empty, isEmpty, emptinessChanged)
ClosedPublic

Authored by rominf on Mar 4 2018, 8:00 AM.

Details

Summary

Add Trash class to handle all trash operations.

Diff Detail

Repository
R318 Dolphin
Branch
add-trash-class
Lint
No Linters Available
Unit
No Unit Test Coverage
rominf requested review of this revision.Mar 4 2018, 8:00 AM
rominf created this revision.
rominf updated this revision to Diff 28555.Mar 4 2018, 8:12 AM

Add comments

rkflx added a subscriber: rkflx.Mar 4 2018, 9:07 AM

Thanks for making it easier to review and the Git history tidier ;)

I like the UI now, but it's kinda hard to review the code. Can you please split this patch into more commits? At least one for the bugfix and one for the new feature.

@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.

rominf edited the summary of this revision. (Show Details)Mar 4 2018, 9:14 AM
rominf edited the test plan for this revision. (Show Details)
rominf updated this revision to Diff 28565.Mar 4 2018, 9:35 AM
rominf edited the test plan for this revision. (Show Details)

Rebase on master

rominf updated this revision to Diff 28567.Mar 4 2018, 9:38 AM
  • Show "Empty Trash" button inside trash directory
rominf updated this revision to Diff 28569.Mar 4 2018, 9:39 AM

Revert wrong commits

rominf updated this revision to Diff 28665.Mar 5 2018, 7:25 AM

Fix comment

markg added a subscriber: markg.Mar 5 2018, 8:45 AM
markg added inline comments.
src/trash/dolphintrash.cpp
65–68

I know i said this before, but i now think that i'm wrong in that assumption.
The completed signal is a response to a request (give me a list of all the entries for example). When that request is completed, it emits the completed signal. That is how you know that there is nothing more to come.

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 :)
I think it's safe to remove the comment (or explain what's really going on) and keep this logic.

markg requested changes to this revision.Mar 5 2018, 8:55 AM
markg added inline comments.
src/dolphinviewcontainer.cpp
39

This one is redundant.

src/panels/places/placesitem.h
93–94

Where is the function body? It's not in placesitem.cpp.

112

Where is the function body? It's not in placesitem.cpp.
Also, why here? move it up ~20 lines next to the other - also private - function.

This revision now requires changes to proceed.Mar 5 2018, 8:55 AM
rominf updated this revision to Diff 28736.Mar 5 2018, 5:30 PM
rominf marked 4 inline comments as done.
  • Cleanup

@markg, can you please review it again?

markg requested changes to this revision.Mar 10 2018, 2:31 PM
markg added inline comments.
src/trash/dolphintrash.cpp
59–69

This is slightly odd.
You made it in a separate function probably to keep the constructor clean, but this is all just constructor stuff.

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.

This revision now requires changes to proceed.Mar 10 2018, 2:31 PM
rominf updated this revision to Diff 29162.Mar 10 2018, 2:40 PM
  • Move watchEmptinessChanged to constructor
rominf marked an inline comment as done.Mar 10 2018, 2:41 PM
markg requested changes to this revision.Mar 10 2018, 2:49 PM

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
30–31

Now you still need to fix the indentation ;)
Sorry, nitpicking...

the semicolon should be on the line below and indentation should be 4 spaces.

This revision now requires changes to proceed.Mar 10 2018, 2:49 PM
rkflx added a comment.Mar 10 2018, 2:54 PM

I think that's about it though. I have no further requests, the code looks OK to me (i did not test it).

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…

markg added a comment.Mar 10 2018, 2:58 PM

I think that's about it though. I have no further requests, the code looks OK to me (i did not test it).

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

I can do an in-depth test.

ngraham accepted this revision as: ngraham.Mar 10 2018, 3:09 PM

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!

elvisangelaccio requested changes to this revision.Mar 10 2018, 3:46 PM

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.

rominf updated this revision to Diff 29185.Mar 10 2018, 7:21 PM
  • Split refactoring and bug fixing
  • Make commit message more descriptive

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
30–31

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?

rominf edited the summary of this revision. (Show Details)Mar 10 2018, 7:24 PM
rominf edited the test plan for this revision. (Show Details)
markg added a comment.Mar 10 2018, 9:02 PM

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

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.

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.

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.

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.

markg accepted this revision.Mar 11 2018, 12:45 PM

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.

elvisangelaccio requested changes to this revision.Mar 11 2018, 1:34 PM
elvisangelaccio added inline comments.
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
47–48

These methods don't use m_trashDirLister, right? => they should be static

52–54

Why protected instead of private?

This revision now requires changes to proceed.Mar 11 2018, 1:34 PM
rominf marked 7 inline comments as done.Mar 11 2018, 1:47 PM
rominf updated this revision to Diff 29230.Mar 11 2018, 1:49 PM
  • Accept feedback
elvisangelaccio accepted this revision.Mar 11 2018, 2:28 PM
elvisangelaccio added inline comments.
src/dolphincontextmenu.cpp
143

This could just be Trash::isEmpty() (same with the other static methods).

This revision is now accepted and ready to land.Mar 11 2018, 2:28 PM
rominf updated this revision to Diff 29237.Mar 11 2018, 2:32 PM
  • Trash::instance.*empty -> Trash::*empty
This revision was automatically updated to reflect the committed changes.
markg added a comment.Mar 11 2018, 2:38 PM

@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 ;)