D10804: Show "Empty Trash" button inside trash directory

[Mark Gaiser]

markg added a comment.


  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>).
  Also, don't forget the const correctness. All you functions are const functions (they don't change class members).
  
  For all your new functions. The the function braces themselves should be on a new line. So:
  
    void func()
    {
        // your code
    }

INLINE COMMENTS

> dolphintrash.cpp:48-49
> +bool Trash::isEmpty() {
> +    KConfig trashConfig(QStringLiteral("trashrc"), KConfig::SimpleConfig);
> +    return (trashConfig.group("Status").readEntry("Empty", true));
> +}

I see this was there before (in a different location), but i just don't get this part. "trashrc" is only used here (I've searched through the code) and it looks like nothing sets it hence the default is used which is true which in turn indicates the trash is empty.

Why?
This just doesn't make sense to me.

Or.. i can't search and it is used elsewhere, but then i didn't find it.

> dolphintrash.cpp:57
> +    // The update of the icon is handled in onTrashEmptinessChanged().
> +    auto trashDirLister = new KDirLister();
> +    trashDirLister->setAutoErrorHandlingEnabled(false, nullptr);

You now have a memory leak. The trashDirLister is never deleted.

> dolphintrash.cpp:60
> +    trashDirLister->setDelayedMimeTypes(true);
> +    auto trashDirContentChanged = [this,trashDirLister](){
> +        auto isTrashEmpty = trashDirLister->items().isEmpty();

missing space after the comma.

> dolphintrash.cpp:61
> +    auto trashDirContentChanged = [this,trashDirLister](){
> +        auto isTrashEmpty = trashDirLister->items().isEmpty();
> +        emit emptinessChanged(isTrashEmpty);

Don't overuse auto. It makes sense for the lambda above and for otherwise link complicated types. In this case, just use bool.

> dolphintrash.cpp:64-66
> +    connect(trashDirLister, static_cast<void(KDirLister::*)()>(&KDirLister::completed), trashDirContentChanged);
> +    connect(trashDirLister, static_cast<void(KDirLister::*)(const KFileItemList&)>(&KDirLister::itemsDeleted),
> +            trashDirContentChanged);

This - as i said in my last message - smells like a bug. If that is required then KIO likely has a bug, Don't fix this here, hunt for the issue in KIO please.

REPOSITORY
  R318 Dolphin

REVISION DETAIL
  https://phabricator.kde.org/D10804

To: rominf, #dolphin, #kde_applications, ngraham, rkflx, markg
Cc: markg, emateli, broulik, elvisangelaccio, rkflx, mmustac, ngraham, #dolphin, spoorun, navarromorales, isidorov, firef, andrebarros, emmanuelp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20180301/971068e6/attachment.htm>