Add navigation history to forward/back buttons
ClosedPublic

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

Details

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 16771
Build 16789: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

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.Sep 2 2019, 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.EditedSep 4 2019, 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.Sep 5 2019, 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.Sep 6 2019, 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.Sep 6 2019, 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.Sep 6 2019, 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.Sep 7 2019, 8:07 AM

Hmm, can you rebase it on master?

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

hallas added a comment.Sep 7 2019, 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.Sep 7 2019, 8:12 AM

Rebased

hallas edited the summary of this revision. (Show Details)Sep 7 2019, 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.Sep 9 2019, 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.Sep 9 2019, 3:22 PM
hallas added a comment.Sep 9 2019, 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)Sep 9 2019, 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.Sep 14 2019, 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.Sep 14 2019, 2:49 PM
hallas added inline comments.Sep 14 2019, 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

hallas added inline comments.Sep 17 2019, 11:13 AM
src/dolphinmainwindow.cpp
95–103

Actually, it seems like Chrome limits the navigation history to 12 entries, @ngraham would such a limit be an acceptable solution?

ngraham added inline comments.Sep 17 2019, 1:03 PM
src/dolphinmainwindow.cpp
95–103

I'd be okay with that, sure.

hallas updated this revision to Diff 66444.Sep 19 2019, 11:24 AM
hallas marked 7 inline comments as done.

Restrict the number of navigation entries to 12

ngraham requested changes to this revision.Sep 19 2019, 4:04 PM

Just noticed that Alt+left/right shortcuts for back and forward are now broken.

This revision now requires changes to proceed.Sep 19 2019, 4:04 PM
hallas updated this revision to Diff 66603.Sep 22 2019, 5:58 AM

Fix Back/Forward shortcuts

Just noticed that Alt+left/right shortcuts for back and forward are now broken.

Nice catch :) This is fixed in the latest revision, please give it a test again.

ngraham accepted this revision as: VDG, ngraham.Sep 24 2019, 5:20 PM

Thanks, that fixed it!

elvisangelaccio requested changes to this revision.Sep 24 2019, 8:10 PM
elvisangelaccio added inline comments.
src/dolphinmainwindow.cpp
96–103

This whole function can be replaced with urlNavigator->locationUrl(i).toString(QUrl::PreferLocalFile) ;)

708

numBack > 0 would be more clear, probably? Btw I'd just call this variable i...

737

Same here.

This revision now requires changes to proceed.Sep 24 2019, 8:10 PM
hallas marked 3 inline comments as done.Sep 27 2019, 7:02 PM
hallas updated this revision to Diff 66965.Sep 27 2019, 7:02 PM

Review comments

elvisangelaccio accepted this revision.Sep 29 2019, 11:07 AM

LGTM now, let's ship it so that we can move forward with the arrows patches.

This revision is now accepted and ready to land.Sep 29 2019, 11:07 AM
hallas closed this revision.Sep 29 2019, 12:16 PM

@hallas I noticed the following warning on dolphin start:

KXMLGUIFactoryPrivate::saveDefaultActionProperties(): Shortcut for action  "go_forward" "&Forward" set with QAction::setShortcut()! Use KActionCollection::setDefaultShortcut(s) instead.

Could you have a look? Maybe it's because we call actionCollection()->setDefaultShortcuts(m_backAction, backShortcuts); and we don't do the same for m_forwardAction ?

@hallas I noticed the following warning on dolphin start:

KXMLGUIFactoryPrivate::saveDefaultActionProperties(): Shortcut for action  "go_forward" "&Forward" set with QAction::setShortcut()! Use KActionCollection::setDefaultShortcut(s) instead.

Could you have a look? Maybe it's because we call actionCollection()->setDefaultShortcuts(m_backAction, backShortcuts); and we don't do the same for m_forwardAction ?

Yes I will take a look - thanks for the heads up :)