D22594: [Dolphin] Open Preferred Search Tool action

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> dolphinmainwindow.cpp:894
> +    KMoreToolsMenuFactory("dolphin/search-tools").fillMenuFromGroupingNames(
> +        &m_searchTools, { "files-find" }, QUrl(localPath())
> +    );

`QUrl::fromLocalFile()`

> dolphinmainwindow.cpp:910
> +    QAction* openPreferredSearchTool = actionCollection()->action(QStringLiteral("open_preferred_search_tool"));
> +    for (QWidget* widget : openPreferredSearchTool->associatedWidgets()) {
> +        QMenu* menu = dynamic_cast<QMenu*>(widget);

This might detach the QList container. Either use a temp  'const QList` variable or use `qAsConst`

> dolphinmainwindow.cpp:911
> +    for (QWidget* widget : openPreferredSearchTool->associatedWidgets()) {
> +        QMenu* menu = dynamic_cast<QMenu*>(widget);
> +        if (menu) {

`qobject_cast`

> dolphinmainwindow.cpp:916-919
> +    connect(
> +        actionCollection()->action(KStandardAction::name(KStandardAction::KeyBindings)), &QAction::hovered,
> +        this, &DolphinMainWindow::updateOpenPreferredSearchToolAction
> +    );

I don't get this connection. Can you explain what's needed for?

> dolphinmainwindow.h:579-584
> +    /**
> +     * Returns local path for the active container URL.
> +     * If the given directory is not local, it can still be the URL of an
> +     * ioslave using UDS_LOCAL_PATH which to be converted first.
> +     * Return user's home path if unsuccessful.
> +     */

I know this is the old comment, but it's not clear. How about something like this?

  Returns the KIO::StatJob::mostLocalUrl() for the active container URL if it's a local file.
  Otherwise returns the user's home path.

> dolphinmainwindow.h:585
> +     */
> +    QString localPath();
> +

I'd call this method  `activeContainerLocalPath()` to make it more clear what it is about.

> dolphinpart.h:237
>  
> +    QString localPath();
> +

Hmm, what about `KParts::ReadOnlyPart::localFilePath()` ? Could we use that?

REPOSITORY
  R318 Dolphin

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

To: pdabrowski, #dolphin, ngraham, elvisangelaccio
Cc: pkloc, kfm-devel, kde-doc-english, iasensio, fprice, gennad, MrPepe, fbampaloukas, alexde, Codezela, feverfew, meven, spoorun, navarromorales, firef, andrebarros, skadinna, emmanuelp, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20190922/4dbb4337/attachment.htm>