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