Show "Empty Trash" button inside trash directory
ClosedPublic

Authored by rominf on Feb 24 2018, 7:03 PM.

Details

Summary

Show "Empty Trash" button inside trash directory.

FEATURE: 163306

Test Plan

Diff Detail

Repository
R318 Dolphin
Branch
empty-trash-widget
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
markg added inline comments.Mar 1 2018, 10:14 PM
src/trash/dolphintrash.cpp
49–50

Ahh, that makes sense.
If dolphin only needed the icon it wouldn't need to look at that file, but it also needs to know the state (empty or full) so it looks at the file.

Thank you for the pointer! No wonder i couldn't find it in Dolphin, it wasn't there ;)

rominf edited the summary of this revision. (Show Details)Mar 2 2018, 4:29 AM
rominf marked 7 inline comments as done.Mar 2 2018, 4:52 AM
rominf updated this revision to Diff 28373.Mar 2 2018, 5:18 AM
  • Remove memory leak
rominf added a comment.Mar 2 2018, 5:22 AM

Is the singleton class really needed? It imho seems a bit overkill for merely the trash status.
On the other hand, it does neatly move some trash functionality in it's own little space. But a class?..
The functions are also all self contained so it might be better suited in it's own namespace (say a TrashHelper::<youFunctions>).

Yep, a singleton is needed, though I don't like it too. I have a signal to emit. That means I have to have QObject. Nobody can emit signals from static functions, hence I need a singleton.

rominf added inline comments.Mar 2 2018, 5:33 AM
src/trash/dolphintrash.cpp
65–67

I understand that this is a bug. I've tried to fix it but I didn't understand how does KDirLister work.
I propose to accept this quick fix for now. Meanwhile, fill a bug report in KIO. Moreover, it would be faster.

markg added a comment.Mar 2 2018, 11:48 AM

Is the singleton class really needed? It imho seems a bit overkill for merely the trash status.
On the other hand, it does neatly move some trash functionality in it's own little space. But a class?..
The functions are also all self contained so it might be better suited in it's own namespace (say a TrashHelper::<youFunctions>).

Yep, a singleton is needed, though I don't like it too. I have a signal to emit. That means I have to have QObject. Nobody can emit signals from static functions, hence I need a singleton.

Ah, right. You have a signal in there. Oke, a class is fine.

src/trash/dolphintrash.cpp
65–67

I rather not do that. Quick fixes (nearly) always stay in for far too long or people just forget about it altogether because it "seemingly works".
Lets do it this way, I will have a look at KDirLister to see if i can get this fixed. I'd have to write a valid testcase first that fails which makes finding the bug itself easier. Writing that test is the difficult part i think.

If i haven't found a fix for it by the end of sunday then i'm not going to find one anytime soon.

So: wait till sunday. If i found a fix, use it.
If i found no fix then by all means, use this workaround but put comments around it that it is a workaround and should be fixed in KIO instead.

rominf marked 3 inline comments as done.Mar 2 2018, 11:51 AM
rominf added inline comments.
src/trash/dolphintrash.cpp
65–67

OK. I agree.

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

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.

This revision now requires changes to proceed.Mar 3 2018, 3:02 PM

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 our documentation for this can be found at https://community.kde.org/Infrastructure/Phabricator#.22Please_do_that_in_a_separate_commit.22

rominf updated this revision to Diff 28503.Mar 3 2018, 4:56 PM
rominf marked an inline comment as done.

Split commit

rominf added a comment.Mar 3 2018, 4:58 PM

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.

I splitted this patch into more commits. Or did you mean phabricator diffs?

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.

I splitted this patch into more commits. Or did you mean phabricator diffs?

Yes please, I meant one phabricator diff for each commit.

D10994 is Restricted, and I can't see it.

rominf updated this revision to Diff 28557.Mar 4 2018, 8:21 AM

Split commit

rominf added a comment.EditedMar 4 2018, 8:27 AM

Blocked by D11012

Blocked by D11012

There is a way to avoid "blocking":

  • Rebase this Diff on D11012
  • Add D11012 as a dependent revision via the Edit Related Revisions button (see also here)

That way, arc patch will still work, but the changes can be reviewed in two separate Diffs.

rominf edited the summary of this revision. (Show Details)Mar 4 2018, 9:16 AM
rominf updated this revision to Diff 28568.Mar 4 2018, 9:38 AM
  • Show "Empty Trash" button inside trash directory
rominf updated this revision to Diff 28671.Mar 5 2018, 7:51 AM
  • Show "Empty Trash" button inside trash directory
rominf added a comment.Mar 5 2018, 7:52 AM
src/trash/dolphintrash.cpp
65–67

Did you find/fix a bug?

rominf updated this revision to Diff 28672.Mar 5 2018, 7:55 AM
  • Rebase on master
rominf updated this revision to Diff 28673.Mar 5 2018, 7:57 AM

More fixes

markg requested changes to this revision.Mar 5 2018, 8:47 AM

As you now also have D11012, please clean this commit up.
This one should only add the button now (and handle it), not add the class with it's helpers.

This revision now requires changes to proceed.Mar 5 2018, 8:47 AM
rominf updated this revision to Diff 29250.Mar 11 2018, 4:12 PM
  • Rebase on master
markg requested changes to this revision.Mar 11 2018, 4:45 PM

I just tried it, works neatly.
It also looks kinda fancy (even though it was not my first choice of having it next to the urlbar, it looks nice)

It's a +1 from me after these 2 changes.
@rkflx and @elvisangelaccio, you both had requested changes. Could you see if those are fulfilled?

src/dolphinviewcontainer.cpp
94

Trash::instance().empty(this); -> Trash::empty(this);

96

Trash::instance().isEmpty() -> Trash::isEmpty()

This revision now requires changes to proceed.Mar 11 2018, 4:45 PM
rominf updated this revision to Diff 29254.Mar 11 2018, 4:52 PM
  • Trash::instance.*empty -> Trash::*empty
rkflx resigned from this revision.Mar 11 2018, 4:55 PM

No time to test in-depth right now, but UI-wise I like it now, so +1.

(Still even after a very brief test run, something is still off, not sure which Diff this belongs to: Restore everything, button is still enabled while it should be disabled as the trash is empty now.)

rominf marked 2 inline comments as done.Mar 11 2018, 4:58 PM
markg requested changes to this revision.Mar 11 2018, 5:14 PM

No time to test in-depth right now, but UI-wise I like it now, so +1.

(Still even after a very brief test run, something is still off, not sure which Diff this belongs to: Restore everything, button is still enabled while it should be disabled as the trash is empty now.)

That should be fixed by D11216, no? That just landed.
I will retest it after this one, just to be sure.

@rominf sorry, a few more tiny things :)

src/dolphinviewcontainer.cpp
94

add "this" before the lambda (context thingy).

163

add "this" before the lambda (context thingy).

src/panels/places/placesitem.cpp
29–31

This is unrelated. I get that you don't like them not being order, but lets keep it focused on the actual thing you implement in this commit :)

This revision now requires changes to proceed.Mar 11 2018, 5:14 PM
rkflx added a comment.Mar 11 2018, 5:20 PM

That should be fixed by D11216, no? That just landed.

Ah, sorry. Nobody subscribed me to that one after the split. Still, the proper way would've been to rebase this Diff on that change. After doing that manually, it works for me.

rominf updated this revision to Diff 29257.Mar 11 2018, 5:22 PM
rominf marked 2 inline comments as done.
  • Take criticism into account
rominf marked an inline comment as done.Mar 11 2018, 5:22 PM
markg added a comment.Mar 11 2018, 5:38 PM

@rominf if i restore from within the trash (trash is empty after restoring), the trash icon in the places menu and the "Empty Trash" button both stay active.
Could you take a look at that?

rominf updated this revision to Diff 29262.Mar 11 2018, 5:46 PM
  • Rebase on master

@rominf if i restore from within the trash (trash is empty after restoring), the trash icon in the places menu and the "Empty Trash" button both stay active.
Could you take a look at that?

It works for me. Can you check it again (I rebased this on master).

markg added a comment.Mar 11 2018, 6:26 PM

@rominf if i restore from within the trash (trash is empty after restoring), the trash icon in the places menu and the "Empty Trash" button both stay active.
Could you take a look at that?

It works for me. Can you check it again (I rebased this on master).

Just done that, same as before.
The icon only updates if i click on the trash again.

@rominf if i restore from within the trash (trash is empty after restoring), the trash icon in the places menu and the "Empty Trash" button both stay active.
Could you take a look at that?

It works for me. Can you check it again (I rebased this on master).

Just done that, same as before.
The icon only updates if i click on the trash again.

Still cannot reproduce. @elvisangelaccio, @ngraham Can you ckeck it?

ngraham accepted this revision.Mar 14 2018, 6:38 PM

@rominf if i restore from within the trash (trash is empty after restoring), the trash icon in the places menu and the "Empty Trash" button both stay active.
Could you take a look at that?

It works for me. Can you check it again (I rebased this on master).

Just done that, same as before.
The icon only updates if i click on the trash again.

Still cannot reproduce. @elvisangelaccio, @ngraham Can you ckeck it?

With a clean build, works fine for me.

markg requested changes to this revision.Mar 14 2018, 10:25 PM

I did a clean rebuild as well, now it works.
Just one more question about a change i don't get.

src/dolphinviewcontainer.cpp
364

Where did:
m_urlNavigator->setVisible(!enabled);

go to? Is there a reason for that?

This revision now requires changes to proceed.Mar 14 2018, 10:25 PM
rominf added inline comments.Mar 15 2018, 6:20 AM
src/dolphinviewcontainer.cpp
364

Now m_urlNavigator is contained in m_navigatorWidget. m_navigatorWidget is a droped-in replacement to m_urlNavigator. That means all operations with m_urlNavigator use m_navigatorWidget.

markg accepted this revision.Mar 15 2018, 11:55 AM

@elvisangelaccio do you have more for this?

src/dolphinviewcontainer.cpp
164–168

How about m_emptyTrashButton->setVisible(m_view->url().scheme() == QLatin1String("trash")) ?

rominf updated this revision to Diff 29659.Mar 16 2018, 6:26 AM
  • m_emptyTrashButton->show/hide -> m_emptyTrashButton->setVisible
rominf marked 3 inline comments as done.Mar 16 2018, 6:27 AM
markg accepted this revision.Mar 16 2018, 1:17 PM
elvisangelaccio accepted this revision.Mar 18 2018, 10:27 AM
This revision is now accepted and ready to land.Mar 18 2018, 10:27 AM
This revision was automatically updated to reflect the committed changes.