D10990: Add "Open symlink destination folder" into symlink's context menu

[Elvis Angelaccio]

elvisangelaccio reopened this revision.
elvisangelaccio added a comment.
This revision is now accepted and ready to land.


  In fact, I do have comments and I'm unhappy this patch got pushed so fast.
  
  First of all, I already pointed @rominf to https://community.kde.org/Policies/Commit_Policy
  
  Yet I see a commit message which says "This is not complete ... I dislike this" which is not acceptable. 
  For this time I'm not gonna revert it because there is bugzilla ticket involed, but please fix the inline comments at least.

INLINE COMMENTS

> dolphinmainwindow.cpp:1219
> +    QAction* showOriginal = actionCollection()->addAction(QStringLiteral("show_original"));
> +    showOriginal->setText(i18nc("@action:inmenu", "Show Original"));
> +    showOriginal->setIcon(QIcon::fromTheme(QStringLiteral("document-open-folder")));

Why "Show Original"? What does original mean?

I'd have expected either "Show Destination" or "Show Target".

> dolphinmainwindow.cpp:1222
> +    showOriginal->setEnabled(false);
> +    connect(showOriginal, &QAction::triggered, [this]() {
> +        const auto link = m_activeViewContainer->view()->selectedItems().at(0);

Missing `this` as receiver parameter.

> dolphinmainwindow.cpp:1223-1234
> +        const auto link = m_activeViewContainer->view()->selectedItems().at(0);
> +        const auto linkLocationDir = QFileInfo(link.localPath()).absoluteDir();
> +        auto linkDestination = link.linkDest();
> +        if (QFileInfo(linkDestination).isRelative())
> +            linkDestination = linkLocationDir.filePath(linkDestination);
> +        if (QFileInfo(linkDestination).exists()) {
> +            KIO::highlightInFileManager({QUrl::fromLocalFile(linkDestination).adjusted(QUrl::StripTrailingSlash)});

This should go in a dedicated function. Lambdas should not be bigger than 2/3 lines of code.

> dolphinmainwindow.cpp:1226-1227
> +        auto linkDestination = link.linkDest();
> +        if (QFileInfo(linkDestination).isRelative())
> +            linkDestination = linkLocationDir.filePath(linkDestination);
> +        if (QFileInfo(linkDestination).exists()) {

Missing braces

> dolphinmainwindow.cpp:1228
> +            linkDestination = linkLocationDir.filePath(linkDestination);
> +        if (QFileInfo(linkDestination).exists()) {
> +            KIO::highlightInFileManager({QUrl::fromLocalFile(linkDestination).adjusted(QUrl::StripTrailingSlash)});

Use the static `QFileInfo::exists()` which is faster.

> dolphinmainwindow.cpp:1396-1399
> +        QAction* renameAction            = col->action(KStandardAction::name(KStandardAction::RenameFile));
> +        QAction* moveToTrashAction       = col->action(KStandardAction::name(KStandardAction::MoveToTrash));
> +        QAction* deleteAction            = col->action(KStandardAction::name(KStandardAction::DeleteFile));
> +        QAction* cutAction               = col->action(KStandardAction::name(KStandardAction::Cut));

Unrelated code style change. Please revert.

REPOSITORY
  R318 Dolphin

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

To: rominf, #dolphin, ngraham
Cc: rkflx, ngraham, elvisangelaccio, markg, #dolphin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20180315/d6db060e/attachment.htm>