Show "Empty Trash" button inside trash directory.
FEATURE: 163306
Show "Empty Trash" button inside trash directory.
FEATURE: 163306
No Linters Available |
No Unit Test Coverage |
src/trash/dolphintrash.cpp | ||
---|---|---|
49–50 | Ahh, that makes sense. Thank you for the pointer! No wonder i couldn't find it in Dolphin, it wasn't there ;) |
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.
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. |
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". 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. |
src/trash/dolphintrash.cpp | ||
---|---|---|
65–67 | OK. I agree. |
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
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.
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 | ||
---|---|---|
96 | Trash::instance().empty(this); -> Trash::empty(this); | |
98 | Trash::instance().isEmpty() -> Trash::isEmpty() |
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.)
src/dolphinviewcontainer.cpp | ||
---|---|---|
96 | add "this" before the lambda (context thingy). | |
165 | add "this" before the lambda (context thingy). | |
src/panels/places/placesitem.cpp | ||
24–26 | 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 :) |
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 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?
I did a clean rebuild as well, now it works.
Just one more question about a change i don't get.
src/dolphinviewcontainer.cpp | ||
---|---|---|
366 | Where did: go to? Is there a reason for that? |
src/dolphinviewcontainer.cpp | ||
---|---|---|
366 | 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. |
src/dolphinviewcontainer.cpp | ||
---|---|---|
166–170 | How about m_emptyTrashButton->setVisible(m_view->url().scheme() == QLatin1String("trash")) ? |