[Moved to invent.kde.org]Add an option to use a KUrlNavigator on the toolbar instead
AbandonedPublic

Authored by felixernst on Feb 20 2020, 4:00 PM.

Details

Summary

Moved to: https://invent.kde.org/system/dolphin/-/merge_requests/21

This commit adds a showLocationBar action to toggle between
using a location bar to navigate or using a new custom QWidgetAction.
The KUrlNavigator of this new QWidgetAction synchonises itself with
the KUrlNavigator of the active view whenever changes occur to either
of them.

When a separate location bar is already present and this QWidgetAction
is on a toolbar it will act like an empty expanding spacer but will
internally mimic the state of the KUrlNavigator of the active view.
When no separate location bar is present setWidgetVisible() is used
to show the internal KUrlNavigator. This switch is done by a
QStackedWidget which is the defaultWidget() of the QWidgetAction.

Test Plan

Everything works as expected?

Diff Detail

Repository
R318 Dolphin
Branch
urlNavInToolBar (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 22719
Build 22737: arc lint + arc unit
felixernst created this revision.Feb 20 2020, 4:00 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptFeb 20 2020, 4:00 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
felixernst requested review of this revision.Feb 20 2020, 4:00 PM
felixernst added a comment.EditedFeb 20 2020, 4:02 PM

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.
  • The right-click menu of the KUrlNavigator doesn't show the "Configure Toolbars…" and "Lock Toolbar" actions.

VDG questions:

  • 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, …
  • What would be a good default shortcut?
  • Is "Separate Location Bar" a good text for this toggleAction?
felixernst edited the test plan for this revision. (Show Details)Feb 20 2020, 4:11 PM
felixernst updated this revision to Diff 76057.Feb 20 2020, 4:17 PM

Add my copyright because I forgot

meven added subscribers: elvisangelaccio, meven.EditedFeb 21 2020, 9:46 AM

The code is a good first step.

Wait on for some basic approvals by VDG and @elvisangelaccio before putting too much work into this.

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"

src/dolphinmainwindow.cpp
834

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

1678

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

1681

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.

1684

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

src/dolphinurlnavigatorwidgetaction.cpp
71

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

src/dolphinurlnavigatorwidgetaction.h
1

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

VDG questions:

Disclaimer not a VDG member

I didn't mean to discriminate. :P

Thanks for the in-depth feedback! If I understand you correctly the biggest change you are suggesting is to make the KUrlNavigator of the DolphinUrlNavigatorWidgetAction directly control a ViewContainer instead of only synchronising it with the KUrlNavigator of the ViewContainer. I can see why this is the cleaner way of implementing this.
I went with the approach of only synchronising with the KUrlNavigator of the active ViewContainer so the "internal" logic of Dolphin is changed as little as necessary and there is the least likelihood of introducing bugs to what is already there. Maintainability was @elvisangelaccio's primary concern after all.
Do I understand you correctly? Are you sure this is the better way forward? I do think you can judge this better than me I just want to make sure. ^^

I'll wait for basic approval before doing more programming on this.

Nip-tick use qobject_cast / dynamic_cast when you want to check against nullptr, say if you know what's the type is static_cast is away faster. @meven suggestion is right, just use member variable for navigator.

Nice! It looks great, hope it gets added

In terms of the user interaction, I'm impressed. I really like the appearance and also the interaction. IMO it's fine for split view too but I'll admit I'm not a heavy user of split view so it would be nice to get opinions from others.

One thing I'm not so sure about is making it optional. If one objection was code maintainability, doing it one way without adding a provision for configurability would surely involve less code, less logic, and less potential for bugs. I'm also not really sure who the target user for returning it to the old state would be? People who don't like change? Heavy users of split views who don't like the extra click required to access the path of an inactive split view? It's kind of a slippery slope; we can't make every change optional.

This isn't a formal objection to making it optional, just a concern. Anyway, very nice work!

@manueljlin: Thanks! I hope so too.

@ngraham: Thanks! Heavy split view users are also the only user group that came to my mind who would have a real reason to be upset if this wasn't optional. Maybe second-hand experience is the best we have to think about this change because no vocal split view user showed up in these discussions yet. Workflow example: I once had a lecturer (literally this guy) who used a split view file manager (Total Commander or something, don't remember) fullscreen and keyboard-only as his primary means to interact with his notebook. He opened up presentations, pictures and GeoGebra-files to show them in the lecture. I think for such a use case having only one path visible at a time would probably be a big step down.

Other than that we would lose the feature of putting the toolbar vertically to the side (without it becoming very wide). Not a huge loss I think because the primary reason for doing this is probably vertical space concern which is also alleviated by this change.

meven added a comment.Feb 26 2020, 3:33 PM

One thing, move the new url_navigator left of sort By (to have some alignment with the view), or center it in the middle of the toolbar.

@manueljlin: Thanks! I hope so too.

@ngraham: Thanks! Heavy split view users are also the only user group that came to my mind who would have a real reason to be upset if this wasn't optional. Maybe second-hand experience is the best we have to think about this change because no vocal split view user showed up in these discussions yet. Workflow example: I once had a lecturer (literally this guy) who used a split view file manager (Total Commander or something, don't remember) fullscreen and keyboard-only as his primary means to interact with his notebook. He opened up presentations, pictures and GeoGebra-files to show them in the lecture. I think for such a use case having only one path visible at a time would probably be a big step down.

I am a frequent split view user, and if I recognize the feature might slightly hurt my workflow, but marginally : Currently in split view mode, all navigation interaction except the url navigator require the user to give the focus to the desired split view. And at least for me the url navigator is not may main navigation workflow.
All in all to me acceptable, configurable or not.

@manueljlin: Thanks! I hope so too.

@ngraham: Thanks! Heavy split view users are also the only user group that came to my mind who would have a real reason to be upset if this wasn't optional. Maybe second-hand experience is the best we have to think about this change because no vocal split view user showed up in these discussions yet. Workflow example: I once had a lecturer (literally this guy) who used a split view file manager (Total Commander or something, don't remember) fullscreen and keyboard-only as his primary means to interact with his notebook. He opened up presentations, pictures and GeoGebra-files to show them in the lecture. I think for such a use case having only one path visible at a time would probably be a big step down.

Other than that we would lose the feature of putting the toolbar vertically to the side (without it becoming very wide). Not a huge loss I think because the primary reason for doing this is probably vertical space concern which is also alleviated by this change.

That's a good point. I have periodically seen screenshots of people with vertical toolbars, and yeah, making it non-optional would make it suck a lot for them. Maybe we could make it non-optional, but when using a vertical toolbar, the URL bar could automatically move back to the top of the view?

cfeck added a subscriber: cfeck.Feb 26 2020, 6:41 PM

Another option is to adopt what Konqueror does: It has a main toolbar, and a toolbar for the URL lineedit. You can either drag them side-by-side, use two toolbar rows, or drag one of them to a different side of the window.

I have a confession to make: after living with the URL navigator in the toolbar for a few days, I'm feeling less sure about it. Putting the it there takes up all the remaining horizontal space in the toolbar, which reduces pretty substantially the amount of space available for toolbar buttons and makes it almost impossible to have any buttons with text, especially on small, narrow, or side-tiled windows. And without the visual changes proposed in T11662, the URL navigator's breadcrumbs mode looks visually muddy, and there's a large amount of space in the toolbar that looks draggable but actually isn't because it belongs to the URL Navigator.

However there is clearly a desire for this by some people. So here's a potential counter-proposal: we make it configurable after all, but don't add a toggle in the menubar. Instead, people who want it in the toolbar can add this new Url Navigator (auto-hide) item to their toolbar, and it will automatically disappear from the main view. People who don't like it can just not add that item to their toolbar (or remove it, if we opt to make it the default).

Thoughts?

I strongly disagree with the space conern. In many configurations putting the UrlNavigator into the toolbar actually increases the space available for the path. As an example in the following screenshots the information panel is open and the UrlNavigator in the toolbar therefore only has ~14 px (< 3%) less space.



Keep in mind that when split view mode is enabled the two UrlNavigators currently only get half of this space. Also keep in mind that the space in the toolbar can be greatly increased by removing unwanted buttons from there.
Furthermore this change mainly increases the available space of the view area.

we make it configurable after all, but don't add a toggle in the menubar.

I don't see a reason to make this feature less discoverable. There are clearly a lot of ppl who want it just looking at T11663 and T12308. To me it seems like a clear majority wants this and the feature is very easy to understand.

And without the visual changes proposed in T11662, the URL navigator's breadcrumbs mode looks visually muddy, and there's a large amount of space in the toolbar that looks draggable but actually isn't because it belongs to the URL Navigator.

I always saw this patch as part of the effort of the UI redesign so I would understand if this couldn't become default behaviour until T11662 is implemented. I am still certain though that putting the UrlNavigator into the Toolbar even by default is the right decision.

Sorry, I wasn't trying to throw cold water on your work here. :) I've gotten quite used to it now and don't feel like it's getting in my way anymore. I think we should see how it looks with the proposed visual refresh, yeah.

I didn't know this idiom but I did feel the cold water. ^^ Thank you for reconsidering.

I'm not really sure about having that icon, that's shown as 'Home' icon in the demo video, on the left of the URL when it's inside the toolbar, but otherwise this looks great visually on first impressions.

I'm not really sure about having that icon, that's shown as 'Home' icon in the demo video, on the left of the URL when it's inside the toolbar

Yeah, what does it do? It seems like it uses both an icon and the first breadcrumb as a way to display a path, which is inconsistent with the current design. I feel like that's the only thing that is stopping this patch from getting accepted as VDG

I'll have the icon removed for my next diff. Méven already told me the one-liner to remove it above.

If I am allowed to proceed with this patch I'll also want to make a DolphinUrlNavigator class how he suggested. I think this will also allow me to make some mini bug fixes without adding more methods to already big classes. Like how KCompletionMode-changes and showing the full path in the navigator does not instantly apply to all tabs. Or how the latter isn't persistent in contrast to every other UI change in Dolphin. Well at least I consider those bugs.
Other than that I would keep the general approach to the UI of this patch. I wouldn't want to go with @cfeck's suggestion because I think having multiple toolbars makes the UI for customising them too complicated for new users. I also wouldn't want to implement @ngraham's "move to the top when the toolbar is vertical"-suggestion because if Dolphin is supposed to allow for both positions of the location then I would think that we might as well keep the current look as a selectable option.
But for now I'll go back to waiting for the permission to continue. ^^

I think you're always allowed to proceed. :) Whether or not it gets merged is a separate matter, but you might as well make it as good as possible, which I think would increase the chances!

Well, I guess that's one way to look at it. ^^ But I already feel like I was a bit bold when I created this patch and ignored Angelaccio's concerns so I wouldn't really want to put more pressure on him by continuing work on this. If I was hungry for more work right now I would be contributing towards the consistency goal.

Please give me a few days, I'll try to have a proper look at the patch :)

Sure, take your time. :) An in-depth code review might not be necessary at the moment because the current implementation will change quite a bit if/when I implement meven's suggestion of having a DolphinUrlNavigator in the toolbar have direct control over the view. I am mainly looking for approval that you would be alright with me working on having the location in the toolbar as an option at all. And maybe you also have a strong opinion on the general approach for this.

meven added a comment.May 31 2020, 1:24 PM

Any progress @felixernst ?

I expect the review process to take a while so perhaps it would be good to move this to a git lab merge request on invent.kde.org.
But do as you please

I guess waiting for approval didn't go anywhere. I'll continue work on this soon then and I'll move it to invent.kde.org.

Sorry for the delay :(

Yes please, do continue to work on this patch.

Definitely please do continue with this patch, and maybe link to it on invent in here.

felixernst retitled this revision from Add an option to use a KUrlNavigator on the toolbar instead to [Moved to invent.kde.org]Add an option to use a KUrlNavigator on the toolbar instead.Jun 14 2020, 3:06 PM
felixernst edited the summary of this revision. (Show Details)
felixernst abandoned this revision.Jun 14 2020, 3:08 PM
felixernst marked 6 inline comments as done.

-> Moved to: https://invent.kde.org/system/dolphin/-/merge_requests/21

As expected the clean way to implement this required a lot of refactoring.

I think I addressed every single comment on this page. If I forgot anything, please remind me on invent.
I didn't address the "empty trash" button. If we really want a trash button to appear in the toolbar as well I would think it would be better to do that outside the DolphinUrlNavigatorWidgetAction class and therefore probably in a separate merge request just for the button but we can discuss this further if necessary.