D7847: Don't block unmounting when terminal panel's cwd is the mountpoint

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added a comment.
This revision now requires changes to proceed.


  Thanks for the updated diff, please fix the inline issues.

INLINE COMMENTS

> dolphinmainwindow.cpp:253
>  {
> +    if (m_tearDownFromPlacesRequested && url.toString() == QStringLiteral("file://") + QDir::homePath()) {
> +        m_placesPanel->proceedWithTearDown();

Please use `QUrl::fromLocalFile()`.

> dolphinmainwindow.h:420
> +     */
> +    void slotStorageTearDownFromPlacesRequested(const QString & mountPath);
> +

Nitpick, please remove the space between & and the argument. Same in the other occurrences below.

> placesitemmodel.cpp:595
> +    m_deviceToTearDown->disconnect();
> +    m_deviceToTearDown = 0;
>  }

nullptr

> placesitemmodel.cpp:962-963
> +
> +    connect(m_deviceToTearDown, SIGNAL(teardownDone(Solid::ErrorType,QVariant,QString)),
> +            this, SLOT(slotStorageTearDownDone(Solid::ErrorType,QVariant)));
> +    m_deviceToTearDown->teardown();

Please use the new syntax:

  connect(m_deviceToTearDown, &Solid::StorageAccess::teardownDone,
              this, &PlacesItemModel::slotStorageTearDownDone);

> placesitemsignalhandler.h:67
> +signals:
> +    void teardownSignal(const QString & udi);
> +

This signal should have a more descriptive name. Maybe `tearDownExternallyRequested()` ?

> placespanel.h:50
>      void errorMessage(const QString& error);
> +    void storageTearDownRequested(const QString mountPath);
> +    void storageTearDownExternallyRequested(const QString mountPath);

Missing pass-by-reference

> terminalpanel.cpp:67
> +    }
> +    return QStringLiteral("");
> +}

Prefer `QString()`.

> terminalpanel.h:77
>      void changeDir(const QUrl& url);
> -    void sendCdToTerminal(const QString& path);
> +    void sendCdToTerminal(const QString& path, bool addToHistory = true);
>  

boolean arguments are a bad practice (bad readability of code), please use an enum instead.

REPOSITORY
  R318 Dolphin

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

To: martonmiklos, #dolphin, ngraham, elvisangelaccio
Cc: ltoscano, ngraham, elvisangelaccio, emmanuelp, zhigalin, spoorun, navarromorales, firef, andrebarros
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20171001/b940e6b1/attachment.htm>