Adress the first round of Angelaccio's review comments

- Split the viewContainers(bool includeInActive) into two methods
    without parameters
- Prevent users from accidently hiding all Url Navigators by
    preventing the dangerous action and then displaying a helpful
    message instead
Unrelated to review comments: Remove a useless line of code

diff --git a/src/dolphinbookmarkhandler.cpp b/src/dolphinbookmarkhandler.cpp
index efcb41692f631773827e8c4b89aa73ed13e3a53e..576a9314b15edadbd4c55ec6e8e75251eae995a5 100644 (file)
--- a/src/dolphinbookmarkhandler.cpp
+++ b/src/dolphinbookmarkhandler.cpp
@@ -68,7 +68,7 @@ bool DolphinBookmarkHandler::supportsTabs() const
 
 QList<KBookmarkOwner::FutureBookmark> DolphinBookmarkHandler::currentBookmarkList() const
 {
 
 QList<KBookmarkOwner::FutureBookmark> DolphinBookmarkHandler::currentBookmarkList() const
 {

-    const auto viewContainers = m_mainWindow->viewContainers(false);
+    const auto viewContainers = m_mainWindow->activeViewContainers();

     QList<FutureBookmark> bookmarks;
     bookmarks.reserve(viewContainers.size());
     for (const auto viewContainer : viewContainers) {
     QList<FutureBookmark> bookmarks;
     bookmarks.reserve(viewContainers.size());
     for (const auto viewContainer : viewContainers) {

diff --git a/src/dolphinmainwindow.cpp b/src/dolphinmainwindow.cpp
index 6d47afae16c5822395e5a423371dff6b6801c427..5e6b6e94a21c15d04d22ad2b4d67aa8a935622d7 100644 (file)
--- a/src/dolphinmainwindow.cpp
+++ b/src/dolphinmainwindow.cpp
@@ -49,6 +49,7 @@
 #include <KJobWidgets>
 #include <KLocalizedString>
 #include <KMessageBox>
 #include <KJobWidgets>
 #include <KLocalizedString>
 #include <KMessageBox>

+#include <KMessageWidget>

 #include <KNS3/KMoreToolsMenuFactory>
 #include <KProtocolInfo>
 #include <KProtocolManager>
 #include <KNS3/KMoreToolsMenuFactory>
 #include <KProtocolInfo>
 #include <KProtocolManager>


@@ -202,7 +203,7 @@ DolphinMainWindow::~DolphinMainWindow()
 {
 }
 
 {
 }
 

-QVector<DolphinViewContainer*> DolphinMainWindow::viewContainers(bool includeInactive) const
+QVector<DolphinViewContainer *> DolphinMainWindow::activeViewContainers() const

 {
     QVector<DolphinViewContainer*> viewContainers;
 
 {
     QVector<DolphinViewContainer*> viewContainers;
 


@@ -845,7 +846,8 @@ void DolphinMainWindow::showFilterBar()
 void DolphinMainWindow::toggleLocationInToolbar()
 {
     // collect needed variables
 void DolphinMainWindow::toggleLocationInToolbar()
 {
     // collect needed variables

-    const bool locationInToolbar = actionCollection()->action(QStringLiteral("location_in_toolbar"))->isChecked();
+    QAction *locationInToolbarAction = actionCollection()->action(QStringLiteral("location_in_toolbar"));
+    const bool locationInToolbar = locationInToolbarAction->isChecked();

     auto viewContainers = this->viewContainers();
     auto urlNavigatorWidgetAction = static_cast<DolphinUrlNavigatorWidgetAction *>
         (actionCollection()->action(QStringLiteral("url_navigator")));
     auto viewContainers = this->viewContainers();
     auto urlNavigatorWidgetAction = static_cast<DolphinUrlNavigatorWidgetAction *>
         (actionCollection()->action(QStringLiteral("url_navigator")));


@@ -856,6 +858,21 @@ void DolphinMainWindow::toggleLocationInToolbar()
     const int selectionStart = lineEdit->selectionStart();
     const int selectionLength = lineEdit->selectionLength();
 
     const int selectionStart = lineEdit->selectionStart();
     const int selectionLength = lineEdit->selectionLength();
 

+    // prevent the switching if it would leave the user without a visible UrlNavigator
+    if (locationInToolbar && !toolBar()->actions().contains(urlNavigatorWidgetAction)) {
+        QAction *configureToolbars = actionCollection()->action(KStandardAction::name(KStandardAction::ConfigureToolbars));
+        KMessageWidget *messageWidget = m_activeViewContainer->showMessage(
+            xi18nc("@info 2 is the visible text on a button just below the message",
+            "The location could not be moved onto the toolbar because there is currently "
+            "no \"%1\" item on the toolbar. Select <interface>%2</interface> and add the "
+            "\"%1\" item. Then this will work.", urlNavigatorWidgetAction->iconText(),
+            configureToolbars->iconText()), DolphinViewContainer::Information);
+        messageWidget->addAction(configureToolbars);
+        messageWidget->addAction(locationInToolbarAction);
+        locationInToolbarAction->setChecked(false);
+        return;
+    }
+

     // do the switching
     GeneralSettings::setLocationInToolbar(locationInToolbar);
     if (locationInToolbar) {
     // do the switching
     GeneralSettings::setLocationInToolbar(locationInToolbar);
     if (locationInToolbar) {


@@ -870,7 +887,7 @@ void DolphinMainWindow::toggleLocationInToolbar()
         }
     }
 
         }
     }
 

-    urlNavigatorWidgetAction->setUrlNavigatorVisible(!locationInToolbar);
+    urlNavigatorWidgetAction->setUrlNavigatorVisible(locationInToolbar);

     m_activeViewContainer->urlNavigator()->setUrlEditable(isEditable);
     if (hasFocus) { // the rest of this method is unneeded perfectionism
         m_activeViewContainer->urlNavigator()->editor()->lineEdit()->setText(lineEdit->text());
     m_activeViewContainer->urlNavigator()->setUrlEditable(isEditable);
     if (hasFocus) { // the rest of this method is unneeded perfectionism
         m_activeViewContainer->urlNavigator()->editor()->lineEdit()->setText(lineEdit->text());


@@ -1756,8 +1773,6 @@ void DolphinMainWindow::setupActions()
         "Depending on the settings this Widget is blank/invisible.",
         "Url Navigator (auto-hide)"));
     actionCollection()->addAction(QStringLiteral("url_navigator"), urlNavigatorWidgetAction);
         "Depending on the settings this Widget is blank/invisible.",
         "Url Navigator (auto-hide)"));
     actionCollection()->addAction(QStringLiteral("url_navigator"), urlNavigatorWidgetAction);

-    connect(locationInToolbar, &KToggleAction::triggered,
-            urlNavigatorWidgetAction, &DolphinUrlNavigatorWidgetAction::setUrlNavigatorVisible);

 
     // for context menu
     QAction* showTarget = actionCollection()->addAction(QStringLiteral("show_target"));
 
     // for context menu
     QAction* showTarget = actionCollection()->addAction(QStringLiteral("show_target"));

diff --git a/src/dolphinmainwindow.h b/src/dolphinmainwindow.h
index 529319e2adf1109193dcd81a171f61174b70749a..251f50d8d0fbd2d4488c2dab4bd1797d69fa96e4 100644 (file)
--- a/src/dolphinmainwindow.h
+++ b/src/dolphinmainwindow.h
@@ -65,17 +65,19 @@ public:
      * having a split view setup, the nonactive view
      * is usually shown in darker colors.
      */
      * having a split view setup, the nonactive view
      * is usually shown in darker colors.
      */

-    DolphinViewContainer* activeViewContainer() const;
+    DolphinViewContainer *activeViewContainer() const;

 
     /**
 
     /**

-     * Returns view containers for all tabs
-     * @param includeInactive   When true all view containers available in
-     *                          this window are returned. When false the
-     *                          view containers of split views that are not
-     *                          currently active are ignored.
-     *                          Default is true.
+     * Returns the active view containers of all tabs.
+     * @see activeViewContainer()
+     * Use viewContainers() to also include the inactive ones.

      */
      */

-    QVector<DolphinViewContainer*> viewContainers(bool includeInactive = true) const;
+    QVector<DolphinViewContainer *> activeViewContainers() const;
+
+    /**
+     * Returns all view containers.
+     */
+    QVector<DolphinViewContainer *> viewContainers() const;

 
     /**
      * Opens each directory in \p dirs in a separate tab. If \a splitView is set,
 
     /**
      * Opens each directory in \p dirs in a separate tab. If \a splitView is set,

diff --git a/src/dolphinviewcontainer.cpp b/src/dolphinviewcontainer.cpp
index da35041878d4749971f928c9d47922a48001c66a..3cdad13fb7a83c80adaff9e33a20a610d11d0148 100644 (file)
--- a/src/dolphinviewcontainer.cpp
+++ b/src/dolphinviewcontainer.cpp
@@ -367,10 +367,13 @@ void DolphinViewContainer::disconnectUrlNavigator()
     updateNavigatorWidgetVisibility();
 }
 
     updateNavigatorWidgetVisibility();
 }
 

-void DolphinViewContainer::showMessage(const QString& msg, MessageType type)
+KMessageWidget *DolphinViewContainer::showMessage(const QString& msg, MessageType type)

 {
     if (msg.isEmpty()) {
 {
     if (msg.isEmpty()) {

-        return;
+        return m_messageWidget;
+    }
+    for (auto action : m_messageWidget->actions()) {
+        m_messageWidget->removeAction(action);

     }
 
     m_messageWidget->setText(msg);
     }
 
     m_messageWidget->setText(msg);


@@ -396,6 +399,7 @@ void DolphinViewContainer::showMessage(const QString& msg, MessageType type)
         m_messageWidget->hide();
     }
     m_messageWidget->animatedShow();
         m_messageWidget->hide();
     }
     m_messageWidget->animatedShow();

+    return m_messageWidget;

 }
 
 void DolphinViewContainer::readSettings()
 }
 
 void DolphinViewContainer::readSettings()

diff --git a/src/dolphinviewcontainer.h b/src/dolphinviewcontainer.h
index 822d8072d1ea957db12489e733e8658a05e6db5c..77c734949ed812e03e6917a5084ef47b09dccfce 100644 (file)
--- a/src/dolphinviewcontainer.h
+++ b/src/dolphinviewcontainer.h
@@ -140,8 +140,9 @@ public:
     /**
      * Shows the message \msg with the given type non-modal above
      * the view-content.
     /**
      * Shows the message \msg with the given type non-modal above
      * the view-content.

+     * @return the KMessageWidget used to show the message

      */
      */

-    void showMessage(const QString& msg, MessageType type);
+    KMessageWidget *showMessage(const QString& msg, MessageType type);

 
     /**
      * Refreshes the view container to get synchronized with the (updated) Dolphin settings.
 
     /**
      * Refreshes the view container to get synchronized with the (updated) Dolphin settings.