D7477: Limit folder panel to home directory if inside home

[Emmanuel Pescosta]

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


  Nice work! :)
  
  I have added some suggestions and questions.
  
  Please make use of `QString::SkipEmptyParts`, everything else looks really good!

INLINE COMMENTS

> kfileitemmodel.cpp:643
> +        // this happens if baseUrl is not root but a home directory, see FoldersPanel
> +        if (subDirs.at(i).isEmpty()) {
> +            continue;

Better use the `QString::SkipEmptyParts` split behavior provided by `QString::split` [1].

[1] https://doc.qt.io/qt-5/qstring.html#split

> folderspanel.cpp:195
>  
> -    loadTree(url());
> +    if (url().isValid()) {
> +        loadTree(url());

Why is this change required?

> folderspanel.cpp:329
>      if (url.isLocalFile()) {
> -        // Use the root directory as base for local URLs (#150941)
> -        baseUrl = QUrl::fromLocalFile(QDir::rootPath());
> +        if (FoldersPanelSettings::limitFoldersPanelToHome() && (Dolphin::homeUrl().isParentOf(url) || Dolphin::homeUrl() == url)) {
> +            baseUrl = Dolphin::homeUrl();

Personal taste: To make the condition more readable I would split it up like this:

  const bool isInHomeFolder = Dolphin::homeUrl().isParentOf(url) || (Dolphin::homeUrl() == url);
  if (FoldersPanelSettings::limitFoldersPanelToHome() && isInHomeFolder) {
      ....
  } else {
      ....
  }

> folderspanel.h:87
> +public slots:
> +    void refreshFoldersPanel();
> +

Personal taste: I would just name it `refresh` because the `FoldersPanel` suffix is somehow redundant.

As far as I can see, this slot can be converted into a private method? Maybe name it `reloadTree` because this is what it actually does.

REPOSITORY
  R318 Dolphin

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

To: hoffmannrobert, #dolphin, elvisangelaccio, emmanuelp
Cc: emmanuelp, elvisangelaccio, #konqueror, #dolphin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20170829/e20bb519/attachment.htm>