D27523: Add an option to use a KUrlNavigator on the toolbar instead

[Méven Car]

meven added subscribers: elvisangelaccio, meven.
meven added a comment.


  The code is a good first step.
  
  Wait on for some basic approvals by #VDG <https://phabricator.kde.org/tag/vdg/> and @elvisangelaccio before putting too
  
  In D27523#614702 <https://phabricator.kde.org/D27523#614702>, @felixernst wrote:
  
  > I want to preface this with saying that this is not a push to ignore the maintainers opinion on this matter. I was hoping that if I provide a decent implementation of this it won't seem that unmaintainable anymore. I have to admit it was more tricky than I expected but I think it worked out okay.
  >
  > Known issues:
  >
  > - The KUrlNavigator of the QWidgetAction has an icon in front. I don't know why that is which is why I wasn't able to hide it yet.
  
  
  KUrlNavigator->setPlacesSelectorVisible ;)
  
  > - The right-click menu of the KUrlNavigator doesn't show the "Configure Toolbars…" and "Lock Toolbar" actions.
  
  No strong opinion on this, either way would be fine.
  
  Notice: the navigator history was per view container (by split view), we would need a way to have this for single urlNavigator mode as well.
  We have the empty trash button to move to DolphinUrlNavigatorWidgetAction from Dolphinview.
  
  A lot of my comments make me think we might want a class DolphinUrlNavigator that encompasses most of its code and that can be used in DolphinUrlNavigatorWidgetAction and DolphinViewContainer.
  
  > VDG questions:
  
  Disclaimer not a VDG member
  
  > - I wasn't sure where to best put the toggle action. As it stands now I didn't find a good spot for it in the Hamburger Menu. Maybe similar to the panels menu there should be a bars menu for Menubar, Toolbar, Find Bar, Location Bar, Filter Bar, …
  
  Below show filter bar perhaps ?
  I would suggest adding a submenu with "show filter bar" and "Separate Location Bar" with a name like "toolbar"
  
  > - What would be a good default shortcut?
  
  F12 as you proposed seems nice to me
  
  > - Is "Separate Location Bar" a good text for this toggleAction?
  
  My 2 cents: "Location bar in toolbar"

INLINE COMMENTS

> dolphinmainwindow.cpp:834
> +        (actionCollection()->action(QStringLiteral("url_navigator")))
> +        ->setWidgetVisible(!action->isChecked());
> +}

To implement a navigatorUrl() accessor returning the right  and use it everywhere `m_activeViewContainer->urlNavigator()` is used.
Exception with `DolphinMainWindow::updateHistory` that will probably need to update both urlNavigator

> dolphinmainwindow.cpp:1678
> +    auto *urlNavigatorWidgetAction = new DolphinUrlNavigatorWidgetAction(this);
> +    // i18n: "auto-hide": Depending on the settings this Widget is blank/invisible.
> +    urlNavigatorWidgetAction->setText(i18nc("@action:inmenu", "Url Navigator (auto-hide)"));

I believe you can put this directly in `i18nc("@action:inmenu auto-hide: Depending on the settings this Widget is blank/invisible"`,

> dolphinmainwindow.cpp:1681
> +    actionCollection()->addAction(QStringLiteral("url_navigator"), urlNavigatorWidgetAction);
> +    connect(this, &DolphinMainWindow::urlChanged,
> +            urlNavigatorWidgetAction, &DolphinUrlNavigatorWidgetAction::setUrl);

We probably would want to toggle those connections in toggleLocationBar as well as those for the regular DolphinViewContainer m_navigatorWidget, to avoid noise signals and simplify implementation in DolphinUrlNavigatorWidgetAction.
This would make `DolphinUrlNavigatorWidgetAction::transmitUrlChanged` unnecessary for instance.

> dolphinmainwindow.cpp:1684
> +    connect(urlNavigatorWidgetAction, &DolphinUrlNavigatorWidgetAction::urlChanged,
> +            this, &DolphinMainWindow::changeUrl);
> +    connect(urlNavigatorWidgetAction, &DolphinUrlNavigatorWidgetAction::tabRequested,

We need to emulate `DolphinViewContainer::slotUrlNavigatorLocationChanged` for urlNavigatorWidgetAction
and `    connect(m_urlNavigator, &KUrlNavigator::urlAboutToBeChanged,

          this, &DolphinViewContainer::slotUrlNavigatorLocationAboutToBeChanged);
  connect(m_urlNavigator, &KUrlNavigator::urlChanged,
          this, &DolphinViewContainer::slotUrlNavigatorLocationChanged);
  connect(m_urlNavigator, &KUrlNavigator::urlSelectionRequested,
          this, &DolphinViewContainer::slotUrlSelectionRequested);
  connect(m_urlNavigator, &KUrlNavigator::returnPressed,
          this, &DolphinViewContainer::slotReturnPressed);
  connect(m_urlNavigator, &KUrlNavigator::urlsDropped, this, [=](const QUrl &destination, QDropEvent *event) {
      m_view->dropUrls(destination, event, m_urlNavigator->dropWidget());
  });`

And maybe more from DolphinViewContainer

> dolphinurlnavigatorwidgetaction.cpp:71
> +
> +void DolphinUrlNavigatorWidgetAction::setWidgetVisible(bool visible)
> +{

Rename to setUrlNavigatorVisible ?
It is just that "setWidgetVisible" does not convey what it does, it is to generic and could be wrongly interpreted.

> dolphinurlnavigatorwidgetaction.h:1
> +/*
> + * Copyright 2020  Felix Ernst <fe.a.ernst at gmail.com>

Move this file and .cpp to the view folder for good manner

REPOSITORY
  R318 Dolphin

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

To: felixernst, #vdg, #dolphin
Cc: meven, elvisangelaccio, kfm-devel, pberestov, iasensio, fprice, MrPepe, fbampaloukas, alexde, Codezela, feverfew, spoorun, navarromorales, firef, ngraham, andrebarros, emmanuelp, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20200221/ca87ad47/attachment.htm>