Add navigation history to forward/back buttons
Needs RevisionPublic

Authored by hallas on Feb 25 2019, 5:44 PM.

Details

Reviewers
ngraham
elvisangelaccio
Group Reviewers
Dolphin
VDG
Summary

Adds navigation history to forward/back buttons in the toolbar. This
changes the forward/back buttons in the toolbar to use the
KToolBarPopupAction class which provides access to a drop down menu.

Test Plan

Browse some folders
Click the back drop down menu and navigate somewhere
Click the forward drop down menu and navigate somewhere

FEATURE: 157819
FIXED-IN: 19.12.0

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D19311
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16200
Build 16218: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

In terms of Breeze, I would like for toolbuttons with dropdown menus to always have a little drop-down arrow in the corner no matter whether the menu is activated on a regular click or a click-and-hold. See also https://bugs.kde.org/show_bug.cgi?id=344746

In terms of Breeze, I would like for toolbuttons with dropdown menus to always have a little drop-down arrow in the corner no matter whether the menu is activated on a regular click or a click-and-hold. See also https://bugs.kde.org/show_bug.cgi?id=344746

Now I am a little confused :) Didn't you just argue that these toolbuttons should _not_ have a little drop-down arrow? That was why I tried to remove them, hence the change in the Breeze theme (which is what actually adds them).

In terms of Breeze, I would like for toolbuttons with dropdown menus to always have a little drop-down arrow in the corner no matter whether the menu is activated on a regular click or a click-and-hold. See also https://bugs.kde.org/show_bug.cgi?id=344746

Now I am a little confused :) Didn't you just argue that these toolbuttons should _not_ have a little drop-down arrow? That was why I tried to remove them, hence the change in the Breeze theme (which is what actually adds them).

In general, I want toolbuttons that show menus when clicked to show arrows in the corner of their icon regardless of whether the menu appears on regular click or click-and-hold.

In this specific use case, I think omitting the arrow (if possible) is fine since that's what web browsers do. But if that's too technically thorny, I'm okay with following the desired convention and showing the arrow.

hallas added a comment.Mar 2 2019, 5:12 AM

In terms of Breeze, I would like for toolbuttons with dropdown menus to always have a little drop-down arrow in the corner no matter whether the menu is activated on a regular click or a click-and-hold. See also https://bugs.kde.org/show_bug.cgi?id=344746

Now I am a little confused :) Didn't you just argue that these toolbuttons should _not_ have a little drop-down arrow? That was why I tried to remove them, hence the change in the Breeze theme (which is what actually adds them).

In general, I want toolbuttons that show menus when clicked to show arrows in the corner of their icon regardless of whether the menu appears on regular click or click-and-hold.

In this specific use case, I think omitting the arrow (if possible) is fine since that's what web browsers do. But if that's too technically thorny, I'm okay with following the desired convention and showing the arrow.

That makes sense :D

I will look into adding a kind of Web Browser style for toolbuttons to have this functionality - Stay Tuned!

hallas added a comment.Mar 6 2019, 5:44 PM

I am now working on a prototype that consists of three pieces:

  1. A new class in KWidgetAddons called BrowserToolBarAction (naming suggestions are welcome :) ) - This will be a subclass of KToolBarPopupAction but it will not add any functionality, it is only used so that Breeze has a way to differentiate the way it is drawn.
  2. Special handling in BreezeStyle so that the BrowserToolBarAction is drawn without a drop down arrow, this is done by checking if the widget derives from BrowserToolBarAction
  3. Use the BrowserToolBarAction in Dolphin

Do you guys think this is a viable way? Is there a better way to get special handling in Breeze? I was also looking at using QObject::setProperty/QObject::property but it doesn't appear to be a common way of doing this. Any suggestions would be greatly appreciated :)

That seems viable.

However the more I think about it, the more I think maybe we should just stop trying to work around the default style and save ourselves this headache :). If this button has a menu that only appears on press-and-hold, maybe that's okay, and what we should do instead is try to make sure the little arrow in the corner of the icon isn't visually obtrusive and doesn't horizontally expand the clickable area.

hallas updated this revision to Diff 53905.Mar 14 2019, 6:14 PM

Changed KToolBarPopUpAction to have the stickyMenu attribute set as false.
Various cleanups.
Implement middle click handling.

@ngraham - I have found out that if I set the KToolBarPopupAction::delayed attribute to true and the KToolBarPopupAction::stickyMenu to false the breeze theme draws the arrows much more compact. I will just add a couple of screenshots so you can see the difference.

Before screenshot:

After screenshot:

Yeah. I'd prefer them even smaller, though. Ideally the button's width wouldn't increase at all; the arrow would just sit unobtrusively in the bottom-right corner. :)

I have made a small modification to the breeze theme, so now the arrow down indicator will not cause the tool button to be any wider, see this screenshot:

@ngraham - What do you think about this?

Definitely better. It looks like the icon gets pushed over to the left side though? I'd like it if the icon stayed where it is, and the arrow gets even smaller and even farther into the bottom-right corner. The arrow really doesn't need to be huge--even a little hint that there's a menu under there is enough IMO.

Hi there,
I just wanted to point out that Falkon, a KDE application, already implements this with a kind of buttons very similar (if not the same) to the ones Dolphin uses:


If it may be of any help, I believe the behaviour of these buttons is defined in https://cgit.kde.org/falkon.git/tree/src/lib/navigation/navigationbar.cpp. Falkon's drop-down history is compatible with both long click and right click.

If you ask me, adding a visual hint to let the user know there is a drop-down menu avaiable is unnecessary. In my honest opinion, any user who expects Dolphin to offer a navigation history will attempt right-clicking or holding one of these buttons. The only reason why they wouldn't try is because they already tried it in previous versions of Dolphin to no success, not because it's not intuitive. Thus I would find it a better idea to reach the userbase and inform them instead of having us to deal with unsightly or obtrusive dropdown arrows.

Also, thank you David Hallas for taking the lead in adding this feature.

Hi there,
I just wanted to point out that Falkon, a KDE application, already implements this with a kind of buttons very similar (if not the same) to the ones Dolphin uses:


If it may be of any help, I believe the behaviour of these buttons is defined in https://cgit.kde.org/falkon.git/tree/src/lib/navigation/navigationbar.cpp. Falkon's drop-down history is compatible with both long click and right click.

If you ask me, adding a visual hint to let the user know there is a drop-down menu avaiable is unnecessary. In my honest opinion, any user who expects Dolphin to offer a navigation history will attempt right-clicking or holding one of these buttons. The only reason why they wouldn't try is because they already tried it in previous versions of Dolphin to no success, not because it's not intuitive. Thus I would find it a better idea to reach the userbase and inform them instead of having us to deal with unsightly or obtrusive dropdown arrows.

Also, thank you David Hallas for taking the lead in adding this feature.

Hi @david.fontanals thanks for the pointers, I will take a look at the code for reference :)

@ngraham - what do you think? Should I go ahead and see if I can tweak the Breeze theme to paint the arrow indicator in the way you suggested, or should we go with the Falkon way? Do we have any other references/sources we can draw feedback from? From my testing I can see that Chrome doesn't draw any indication on the navigation buttons indicating there is a drop down menu available - the same goes for Firefox.

I don't hate the idea of not showing the arrows. We can do that.

However please do submit your patch that fixes https://bugs.kde.org/show_bug.cgi?id=344746! :)

I don't hate the idea of not showing the arrows. We can do that.

However please do submit your patch that fixes https://bugs.kde.org/show_bug.cgi?id=344746! :)

Hmm, I haven't done a patch that fixes https://bugs.kde.org/show_bug.cgi?id=344746 ;) I can see there is a looong discussion there, what is the bottom line of it? Should we maybe talk about it on an email thread? Or IRC?

The gist of it is that I want for Breeze-styled ToolButtons which display a menu when left-clicked normally (not pressed-and-held) to become wider and display a downward-pointing arrow, exactly like in your screenshot here:

But you're right, this is not relevant to this patch so we can discuss it elsewhere. I just figured that your patch was probably already 9/10ths of the way there. :)

Anyway, if you rejigger this patch to remove the downward-pointing arrows, I'll re-review and we can hopefully get it in for 19.04.0 if @elvisangelaccio agrees.

I couldn't help spending a little time looking more into this arrow stuff :D I have tried to adjust the down arrow so that it is much smaller and put in the lower right corner of the button, also it doesn't change the size of the button. I guess a screenshot says more then a 1000 words:

@ngraham - What do you think? Is this closer to what you have in mind? Also, should I post a new review where we can discuss this issue?

I couldn't help spending a little time looking more into this arrow stuff :D I have tried to adjust the down arrow so that it is much smaller and put in the lower right corner of the button, also it doesn't change the size of the button. I guess a screenshot says more then a 1000 words:

@ngraham - What do you think? Is this closer to what you have in mind? Also, should I post a new review where we can discuss this issue?

That's exactly what I had in mind, yes!!!! I might even make them a bit smaller and more in-the-cornery.

I couldn't help spending a little time looking more into this arrow stuff :D I have tried to adjust the down arrow so that it is much smaller and put in the lower right corner of the button, also it doesn't change the size of the button. I guess a screenshot says more then a 1000 words:

@ngraham - What do you think? Is this closer to what you have in mind? Also, should I post a new review where we can discuss this issue?

That's exactly what I had in mind, yes!!!! I might even make them a bit smaller and more in-the-cornery.

I have created a separate patch for the down-arrow-stuff here: D19890

So is there anything else that needs to be done for this change here?

ngraham accepted this revision as: VDG, ngraham.Mar 24 2019, 8:12 PM

Thanks, that's perfect. The UI for this is now flawless in my opinion. :) I'll hand this show back over to @elvisangelaccio for code review.

ognarb added a subscriber: ognarb.Mar 24 2019, 8:14 PM

About the UI: I'd also be in favor of not showing the arrows. It's not just Falkon that doesn't show them, but also Firefox and Chrome.

src/dolphinmainwindow.cpp
2159–2168

Unrelated whitespace change

src/middleclickactioneventfilter.cpp
56–70

This could maybe go as the else branch of if (toolBar) ?

About the UI: I'd also be in favor of not showing the arrows. It's not just Falkon that doesn't show them, but also Firefox and Chrome.

After a lot of thought, I agree. I've reverted the Breeze change for the tiny arrows. Sorry for jerking you around here, @hallas! :( Would you mind updating the patch to the version that won't show any arrows at all?

About the UI: I'd also be in favor of not showing the arrows. It's not just Falkon that doesn't show them, but also Firefox and Chrome.

After a lot of thought, I agree. I've reverted the Breeze change for the tiny arrows. Sorry for jerking you around here, @hallas! :( Would you mind updating the patch to the version that won't show any arrows at all?

Hi @ngraham - no problem ;) Sometimes you have to jerk around a bit to find the best solution, especially when it comes to usability. I will update the patch.

About the UI: I'd also be in favor of not showing the arrows. It's not just Falkon that doesn't show them, but also Firefox and Chrome.

After a lot of thought, I agree. I've reverted the Breeze change for the tiny arrows. Sorry for jerking you around here, @hallas! :( Would you mind updating the patch to the version that won't show any arrows at all?

The version that doesn't show menu arrows doesn't have a delayed menu :/ There a few paths forward though:

  • Go with the version that doesn't draw any arrows but it is not a delayed menu meaning that the menu appears immediately when you click the back button. Personally I think this is a bad choice.
  • Modify/extend the Breeze theme to draw this button (via some special flag or something) without arrows, even though it has a delayed menu (maybe this should be a general change). Personally this would be my choice.
  • Implement our own button logic, similar to what is done in Falkon.

What do you guys ( @elvisangelaccio @ngraham ) think?

  • Modify/extend the Breeze theme to draw this button (via some special flag or something) without arrows, even though it has a delayed menu (maybe this should be a general change). Personally this would be my choice.

I agree, this is best. For inspiration, you might check out the following abandoned Breeze patch that did something similar and allowed opting out on a widget-by-widget basis: D13064 Maybe all we need to do is implement that opting-out mechanism for existing press-and-hold menu toolbuttons, and then set the hint in these buttons here.

This change is still WIP and is meant as a discussion point. I would like to get some feedback on this in general but most specifically around what to show in the navigation drop down? Currently I just put the full URL there but that is not very user friendly.

My thoughts on this (long sought :) feature:

  1. Since Firefox et al were already referenced, you guys surely noticed that pushing on either back or forward gives you the same dropdown-menu there. Can we expect this behavior also with this patch? (Didn't saw it mentioned yet).
  2. Re. list size: Firefox has 15 list items. So if you want to go back 20 entries, you have to invoke the menu twice (going -14 and then -6). Which I think is a reasonable approach. Showing all history entries at once, can get ugly quiet fast with a long history.
  3. Re. action title: I would be ok with just the (top) folder name. If you go with the path, I would make sure that there is a character limit, otherwise the menu could look quite ugly for users with long paths.

Below attached screenshot from firefox for reference:


Also notice the page icon in front of the title, which upon hovering changes to either the forward or backward arrow.

david.fontanals added a comment.EditedApr 14 2019, 3:44 PM

@richardl
As I see it, showing both forward and backward entries at the same time is a smart idea if your interface requires having a single button for this feature. Otherwise, it doesn't make much sense because one would never click on the backward navigation button to pick a forward entry. Having to see the forward entries first to decide which backward entry to pick is not a common case either (and vicecersa). Thus, in most cases you'd just have to deal with the drawback of limiting the amount of forward-only or backward-only entries that can be shown, which in addition is counter-productive if you want to roll back a lot of entries.

Falkon uses separate lists with a limit of 20 entries. It also has a "Clear history" button I have never used which forgets the backward/forward history for the current tab.

@richardl - thanks for the feedback! I tend to agree with @david.fontanals in that it makes most sense to only show the back history in the back button and the forward history in the forward button. This also seems to be the behavior of Chrome (though I don't know how much it matters :) ).

Regarding the title of each entry, I am pretty sure that this will be refined after this patch has landed and it gets some mileage :D But I think your point with having a limit is something we should look at. Otherwise I am curious to see how the feedback to this feature will be.

  • Modify/extend the Breeze theme to draw this button (via some special flag or something) without arrows, even though it has a delayed menu (maybe this should be a general change). Personally this would be my choice.

I agree, this is best. For inspiration, you might check out the following abandoned Breeze patch that did something similar and allowed opting out on a widget-by-widget basis: D13064 Maybe all we need to do is implement that opting-out mechanism for existing press-and-hold menu toolbuttons, and then set the hint in these buttons here.

@ngraham - I have implemented a solution based on D13064 where I add a new widget property that Breeze honors to disable the rendering of the arrow, but I am not sure what is the best way to actually set this property. Currently I have done a patch for KWidgetAddons::KToolBarPopupAction to set it, but I don't know if it would be better if Dolphin had a subclass of KWidgetAddons::KToolBarPopupAction and set the property there?

Here is the change in Breeze: F6773033
And the change in KWidgetAddons: F6773036

To test it you also need to change the following in Dolphin:

diff --git a/src/dolphinmainwindow.cpp b/src/dolphinmainwindow.cpp
index 035a174c6..c2015cb22 100644
--- a/src/dolphinmainwindow.cpp
+++ b/src/dolphinmainwindow.cpp
@@ -1252,6 +1252,7 @@ void DolphinMainWindow::setupActions()
     }
     m_backAction->setDelayed(true);
     m_backAction->setStickyMenu(false);
+    m_backAction->setMenuIndicator(false);
     connect(m_backAction, &QAction::triggered, this, &DolphinMainWindow::goBack);
     connect(m_backAction->menu(), &QMenu::aboutToShow, this, &DolphinMainWindow::slotAboutToShowBackPopupMenu);
     connect(m_backAction->menu(), &QMenu::triggered, this, &DolphinMainWindow::slotGoBack);
@@ -1287,6 +1288,7 @@ void DolphinMainWindow::setupActions()
     }
     m_forwardAction->setDelayed(true);
     m_forwardAction->setStickyMenu(false);
+    m_forwardAction->setMenuIndicator(false);
     connect(m_forwardAction, &QAction::triggered, this, &DolphinMainWindow::goForward);
     connect(m_forwardAction->menu(), &QMenu::aboutToShow, this, &DolphinMainWindow::slotAboutToShowForwardPopupMenu);
     connect(m_forwardAction->menu(), &QMenu::triggered, this, &DolphinMainWindow::slotGoForward);

What do you think? Should I post these patches in the respective repositories? Or doesn't it make sense to add this functionality to KWidgetAddons?

The Breeze patch looks sane. I'm torn on whether or not adding the new style to KWidgetsAddons makes sense though. Couldn't Dolphin just set the new _kde_toolButton_noMenuArrow on just these buttons?

@richardl
As I see it, showing both forward and backward entries at the same time is a smart idea if your interface requires having a single button for this feature. Otherwise, it doesn't make much sense because one would never click on the backward navigation button to pick a forward entry. Having to see the forward entries first to decide which backward entry to pick is not a common case either (and vicecersa). Thus, in most cases you'd just have to deal with the drawback of limiting the amount of forward-only or backward-only entries that can be shown, which in addition is counter-productive if you want to roll back a lot of entries.

I can't follow the remark about the single button, since there are clearly two buttons in firefox? Anyways, the rationale in firefox is IMHO that the user wants to jump to one point in the history, but if that's in the forward or backward history might not be known (in advance). (For a single (or double) forward/back one wouldn't invoke the menu anyways.) If you take this uncertainty into account the unified menu makes much more sense.

The Breeze patch looks sane. I'm torn on whether or not adding the new style to KWidgetsAddons makes sense though. Couldn't Dolphin just set the new _kde_toolButton_noMenuArrow on just these buttons?

I don't think I can, because the buttons are not created directly by Dolphin, but instead they are created by QToolbar using the KToolBarPopupAction::createWidget function. So therefore I think we would either need to handle it in KWidgetAddons or maybe do a subclass of KToolBarPopupAction in Dolphin which adds this property, what do you guys think?

The Breeze patch looks sane. I'm torn on whether or not adding the new style to KWidgetsAddons makes sense though. Couldn't Dolphin just set the new _kde_toolButton_noMenuArrow on just these buttons?

I don't think I can, because the buttons are not created directly by Dolphin, but instead they are created by QToolbar using the KToolBarPopupAction::createWidget function. So therefore I think we would either need to handle it in KWidgetAddons or maybe do a subclass of KToolBarPopupAction in Dolphin which adds this property, what do you guys think?

Ah yeah, that makes sense. A subclass in Dolphin that just adds that property probably makes sense.

Why would we want to handle it in dolphin? The proposed patch (F6773036) looks good to me, other applications could benefit from it without having to reinvent the wheel.

Sure thing. Please listen to @elvisangelaccio rather than my deluded ravings! :)

Sure thing. Please listen to @elvisangelaccio rather than my deluded ravings! :)

Ok - I will do that then!

I have pushed the Breeze change here: D20867 - please take a look!

What happens if the user is using an old version of breeze that doesn't have this new property? Will they get the arrows?

What happens if the user is using an old version of breeze that doesn't have this new property? Will they get the arrows?

Yes - they would simply get the arrows :)

BTW - I haven't tested this functionality with other styles then Breeze, but would assume that it should work fine there as well.

mart added a subscriber: mart.May 6 2019, 9:19 AM

needs button->setArrowType(Qt::ArrowType::NoArrow);

and then to debug and find why breeze still adds it (and check also what happens with other styles)

So what needs to happen here now? I'll admit I'm a bit lost. Do we need a Breeze patch, or can we set the correct hints here?

So what needs to happen here now? I'll admit I'm a bit lost. Do we need a Breeze patch, or can we set the correct hints here?

We can't create the hints here. The solution @mart suggested doesn't work. I have created a Breeze patch D20867 but it is still pending approval, I think that needs to be addressed before this patch can move forward.

But thanks for picking this one up :D

hallas added a comment.Aug 8 2019, 5:45 AM

@ngraham - Hey Nate, if you have time I would like to pick this patch up again, what are your thoughts on how we progress this? Currently this patch depends on D20867 being merged and released and also a change to KWidgetAddons (F6773036), so one approach could be to focus the review effort on those changes first? We have previously discussed various ways to simplify this change, but no other suitable solutions has been found, but I am still open to simpler solutions :)

I apologize for going dark on this. To be honest I feel a bit out of my technical depth here and I think you should either find someone else who knows the code better or proceed according to your own instincts. :)

I'd like to test this... ...will Oxygen need similar changes?

hallas added a comment.Mon, Sep 2, 1:26 PM

I'd like to test this... ...will Oxygen need similar changes?

@nerdopolist You should be able to test it out with the instructions I posted earlier. I don't know if Oxygen displays drop-down arrows in this particular configuration, but if it does, we might want to add the same kind of handling of this new view property (of cause if and when these patches land :) ) - But in general it would be super awesome to get some feedback on these patches, and some help with getting them landed ( if we all agree that this is the way forward ).

nerdopolist added a comment.EditedWed, Sep 4, 11:42 AM

I patched Dolphin and KWidgetAddons.
I get

dolphin: [ 63%] Building CXX object src/CMakeFiles/dolphinstatic.dir/panels/terminal/terminalpanel.cpp.o
dolphin: In file included from /srcbuild/dolphin/src/dolphinbookmarkhandler.cpp:21:
dolphin: /srcbuild/dolphin/src/dolphinmainwindow.h:633:5: error: ‘KToolBarPopupAction’ does not name a type
dolphin: KToolBarPopupAction* m_backAction;
dolphin: ^~~~~~~~~~~~~~~~~~~

Am I missing something else?

EDIT: yes, I am, sorry, noticing one of the hunks failed

Looks good. I do have a weird occurrence with three slashes appearing in the path for

seems when I leave and reenter, then leave the home dir.

hallas added a comment.Thu, Sep 5, 5:20 AM

Looks good. I do have a weird occurrence with three slashes appearing in the path for

seems when I leave and reenter, then leave the home dir.

Glad you got it working ;) I don't know why you would have 3 slashes in front of the path, that seems odd. What the code actually does is just take the URLs from your navigation history, and if it is a local URL it removes the scheme part. You could add some debug to the GetPopupMenuLabel function to see if we can point out what is going on? It might be the url.adjusted(QUrl::RemoveScheme) function not removing the two slashes from the scheme, i.e. if the url is file:///some/root/folder it should remove the scheme including slashes, file:// and return /some/root/folder (at least that is how it used to work :) ) - but I might be missing something here.

Besides that, the next step is to get the patches reviewed/accepted by someone, this is where things have stalled so far, if you want it would be nice to get some help with that?

At this point I'm inclined to say that we should go forward with the patch regardless of widget changes, because otherwise it's gonna be stuck in patch review limbo forever.

Can you make the menu show up on press-and-hold without showing any arrow on the buttons without widget changes? That's probably the simplest path forward until we sort out the appearance issue. Better to temporarily make it semi-hidden than never getting it landed. :)

Dolphin appears to not compile without F6773036 in KWidgetAddons though. All I can find for F6773036 is the diff. Is there a Dxxxxx thing in phab for that?

hallas added a comment.Fri, Sep 6, 4:48 AM

At this point I'm inclined to say that we should go forward with the patch regardless of widget changes, because otherwise it's gonna be stuck in patch review limbo forever.

Can you make the menu show up on press-and-hold without showing any arrow on the buttons without widget changes? That's probably the simplest path forward until we sort out the appearance issue. Better to temporarily make it semi-hidden than never getting it landed. :)

I will take a look at it and update this patch :) Let's see if we can get this done for the next applications release.

hallas added a comment.Fri, Sep 6, 4:48 AM

Dolphin appears to not compile without F6773036 in KWidgetAddons though. All I can find for F6773036 is the diff. Is there a Dxxxxx thing in phab for that?

No, I haven't created a Phabricator Differential for it, but that could be easily done :)

hallas added a comment.Fri, Sep 6, 8:50 AM

At this point I'm inclined to say that we should go forward with the patch regardless of widget changes, because otherwise it's gonna be stuck in patch review limbo forever.

Can you make the menu show up on press-and-hold without showing any arrow on the buttons without widget changes? That's probably the simplest path forward until we sort out the appearance issue. Better to temporarily make it semi-hidden than never getting it landed. :)

@ngraham - I have been experimenting a bit, and here are my findings:

Essentially we only have the delayed and the stickyMenu attribute we can change (and they are both boolean), here is a list of all the combinations and how they appear and behave:

delayed - true
stickyMenu - false
Arrows shown in lower right corner but not clickable
Delayed menu

delayed - false
stickyMenu - false
Arrows not shown
Instant menu

delayed - false
stickyMenu - true
Arrows not shown
Instant menu

delayed - true
stickyMenu - true
Arrows shown on the right and displays the dropdown menu when clicked
Instant menu show if the dropdown arrow is clicked

Delayed menu means that if you just click the button it navigates and doesn't show the menu (the same behavior as today), but if you click and hold, the dropdown menu is shown.
Instant menu means that the dropdown menu is shown when you click the button, so you have to select a menu entry to navigate.

If we don't want the arrow to appear then we would always get the dropdown menu, which in my view is not a good solution since it changes the existing behavior of the navigation buttons. That leaves us with two options, either the arrow in the lower right corner and the dropdown menu being shown if you click and hold, or a larger arrow shown to the right that shows the menu when you click the arrow. Personally I prefer the first solution since that most closely resemble how most browsers work today, also with the second solution you have to click the arrow, which is fairly small, to get the dropdown menu, so I don't think it is super user-friendly.

So, @ngraham - maybe you should pull the patch and play around with the options to see which one you think works best?

Thanks, will do. Might be a few days due to travel.

Hmm, can you rebase it on master?

We have previously discussed various ways to simplify this change, but no other suitable solutions has been found, but I am still open to simpler solutions :)

This is what I think can produce the wanted behaviour:
KToolBarPopupAction uses QToolButton internally so Indicators are drawn (because of style rules if I understand correctly). If there is no way to use QToolButton and hide indicators without forcing style changes (which was blocked) then it seems to me like the only way forward is not using a QToolButton.
AFAICT there are two ways forward:

  • Further modify KToolBarPopupAction to not use QToolButton internally in the case of menuIndicator being turned off and instead internally implement the same behaviour using some other button class. The QAbstractButton with it's pressed and clicked signals might be enough to handle it but there are possibly more fitting specialised button classes.

This way no changes to the Breeze style or any other style for that matter should be necessary.

  • It might be a bit difficult to imitate the behaviour of KToolBarPopupAction without using a QToolButton so alternatively it might be easier to not modify KToolBarPopupAction at all and create or use a different class. In this case you don't have to mimic all options of KToolBarPopupAction perfectly since it is expected that the new class behaves differently. You could then copy from/use Falkon's NavigationBarToolButton (like @david.fontanals suggested in D19311#432597) . I'm pretty sure the license of that class is incompatible with frameworks though so that would have to be handled if it's supposed to land in KWidgetAddons.

Take my analysis with a grain of salt though since it is mainly based on me reading documentation without having ever actually tried similar stuff.

hallas added a comment.Sat, Sep 7, 8:07 AM

Hmm, can you rebase it on master?

Yup, I will just rebase it and update the patchset :)

hallas added a comment.Sat, Sep 7, 8:08 AM

We have previously discussed various ways to simplify this change, but no other suitable solutions has been found, but I am still open to simpler solutions :)

This is what I think can produce the wanted behaviour:
KToolBarPopupAction uses QToolButton internally so Indicators are drawn (because of style rules if I understand correctly). If there is no way to use QToolButton and hide indicators without forcing style changes (which was blocked) then it seems to me like the only way forward is not using a QToolButton.
AFAICT there are two ways forward:

  • Further modify KToolBarPopupAction to not use QToolButton internally in the case of menuIndicator being turned off and instead internally implement the same behaviour using some other button class. The QAbstractButton with it's pressed and clicked signals might be enough to handle it but there are possibly more fitting specialised button classes. This way no changes to the Breeze style or any other style for that matter should be necessary.
  • It might be a bit difficult to imitate the behaviour of KToolBarPopupAction without using a QToolButton so alternatively it might be easier to not modify KToolBarPopupAction at all and create or use a different class. In this case you don't have to mimic all options of KToolBarPopupAction perfectly since it is expected that the new class behaves differently. You could then copy from/use Falkon's NavigationBarToolButton (like @david.fontanals suggested in D19311#432597) . I'm pretty sure the license of that class is incompatible with frameworks though so that would have to be handled if it's supposed to land in KWidgetAddons.

    Take my analysis with a grain of salt though since it is mainly based on me reading documentation without having ever actually tried similar stuff.

Thanks for the analysis :) I still think we should get the change in with the current KToolBarPopupAction functionality (like Nate suggest), and then work on refining the user experience. But feel free to pinch in on the user experience part.

hallas updated this revision to Diff 65550.Sat, Sep 7, 8:12 AM

Rebased

hallas edited the summary of this revision. (Show Details)Sat, Sep 7, 8:12 AM

Thanks for the analysis :) I still think we should get the change in with the current KToolBarPopupAction functionality (like Nate suggest), and then work on refining the user experience. But feel free to pinch in on the user experience part.

This is such a big discussion already. Which suggestion of Nate do you refer to specifically? I thought the current approach that shows indicators was denied.

Thanks for the analysis :) I still think we should get the change in with the current KToolBarPopupAction functionality (like Nate suggest), and then work on refining the user experience. But feel free to pinch in on the user experience part.

This is such a big discussion already. Which suggestion of Nate do you refer to specifically? I thought the current approach that shows indicators was denied.

Sorry about the confusion ;) The plan is to merge this change with arrows, and then afterwards work on removing the arrows, does that make sense?

Oh okay. I understand. I only referred to the arrow removing step then.

ngraham accepted this revision.Mon, Sep 9, 3:22 PM

Thanks @hallas. I'm good with this now. For the future, I would like one of the following UI improvements:

  • Show no visible arrow at all (i.e. what web browsers do)
  • Make the downward-pointing arrow be really tiny and sit in the bottom-right corner of a square toolbutton, rather than making the toolbutton wider to accommodate it
This revision is now accepted and ready to land.Mon, Sep 9, 3:22 PM
hallas added a comment.Mon, Sep 9, 5:19 PM

Thanks @hallas. I'm good with this now. For the future, I would like one of the following UI improvements:

  • Show no visible arrow at all (i.e. what web browsers do)
  • Make the downward-pointing arrow be really tiny and sit in the bottom-right corner of a square toolbutton, rather than making the toolbutton wider to accommodate it

Yes, let't find a solution for this, once this gets in :D

@elvisangelaccio - could you please re-review this patch to ensure that you are ok with it before merging?

ngraham edited the test plan for this revision. (Show Details)Mon, Sep 9, 5:21 PM

Cool! IMHO, I think the arrows should stay. With this feature being new to Dolphin, not everyone reads the release notes, and won't be aware that the navigation history is there in the next version. The arrows would give a hint. At least that's how I see it

elvisangelaccio requested changes to this revision.Sat, Sep 14, 2:49 PM

Haven't look at the whole diff yet, since the fix for the duplicates could be quite invasive

src/dolphinmainwindow.cpp
95–103

The problem is that the menu can become huge and fill the whole screen:

Another problem from this screenshot: if an URL is already in the history, don't add it again.

694

please call it urlNavigator

707

please call it urlNavigator

723

please call it urlNavigator

This revision now requires changes to proceed.Sat, Sep 14, 2:49 PM
hallas added inline comments.Sat, Sep 14, 3:49 PM
src/dolphinmainwindow.cpp
95–103

Thanks for the review feedback :)

In the original patch I had a limit on the number of entries in the menu, but this was removed on request from review feedback, but would that be a way to solve it in combination with not adding duplicates? I just checked with my browser (Chrome) and that actually allows duplicates in the history, but I haven't tested how many entries it limits. @elvisangelaccio @ngraham what are your thoughts on this?

We could make the menu scrollable rather than expand sideways for this use case. There's a QStyleHint for that IIRC. I think it's a somewhat niche scenario case anyway