Bug 327412 - Additional items are selected when using "Forward" and "Back" mouse buttons
Summary: Additional items are selected when using "Forward" and "Back" mouse buttons
Status: RESOLVED FIXED
Alias: None
Product: dolphin
Classification: Applications
Component: view-engine: general (show other bugs)
Version: 4.11.3
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: Dolphin Bug Assignee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-10 14:33 UTC by David Rosca
Modified: 2013-11-14 08:12 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 4.11.4


Attachments
Don't propagate those events (638 bytes, patch)
2013-11-10 14:34 UTC, David Rosca
Details
Screenshot describing the issue (153.58 KB, image/png)
2013-11-10 14:35 UTC, David Rosca
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Rosca 2013-11-10 14:33:15 UTC
When using the "Forward" and "Back" mouse buttons to go back/forward in history, and this view in history has been scrolled down, additional unwanted items will be selected.
It is because the mouse button press was propagated as a normal left/right click.

Reproducible: Always

Steps to Reproduce:
1. Open a folder which contains a lot of other folders
2. Scroll down and open one of the folders
3. Press "Back" on mouse (move the mouse at the bottom of the view) and see that additional items were selected
Actual Results:  
Additional items gets selected

Expected Results:  
Nothing unwanted gets selected
Comment 1 David Rosca 2013-11-10 14:34:10 UTC
Created attachment 83475 [details]
Don't propagate those events
Comment 2 David Rosca 2013-11-10 14:35:41 UTC
Created attachment 83476 [details]
Screenshot describing the issue
Comment 3 Emmanuel Pescosta 2013-11-10 19:23:27 UTC
Thanks for the bug report + patch!!

>  if (event->buttons() & (Qt::XButton1 | Qt::XButton2) && m_pressedIndex < 0) {
Why do you check for m_pressedIndex < 0 here?

This means:
When you press the back or forward button above an item, the code in your if statement will not be called, because the m_pressedIndex is equal or greater than 0 (and not less than 0), so the item will be selected instead.

You can simply fix this behavior, by removing the m_pressedIndex < 0 from the if condition.

Btw. you can move this button check before "m_pressedMousePos = transform.map(event->pos());" and add "emit mouseButtonPressed(-1, event->buttons());" to the if statement before "return true". 
The -1 means pressed above the viewport.
Comment 4 David Rosca 2013-11-10 20:33:59 UTC
> When you press the back or forward button above an item, the code in your if statement will not be called, because the m_pressedIndex is equal or greater than 0 (and not less than 0), so the item will be selected instead.

This is correct, I just copied the check (for m_pressedIndex) from src/views/dolphinview.cpp DolphinView::slotMouseButtonPressed. 
There is a comment explaining that pressing those buttons on items should just focus them.

"// Trigger the history navigation only when clicking on the viewport:   
 // Above an item the XButtons provide a simple way to select items in   
 // the singleClick mode."


Actually, the whole idea of this patch is simply ignore the mouse press when going back / forward in history.
Comment 5 Emmanuel Pescosta 2013-11-11 09:16:18 UTC
> There is a comment explaining that pressing those buttons on items should just focus them.
Oh sorry, I didn't know that.

In my opinion, we should change this behavior to the default back/forward mouse button behavior: always trigger the history navigation on back/forward button click. 

This is what the user expects from these buttons.

@Frank, @you
What do you think?
Comment 6 David Rosca 2013-11-11 09:20:15 UTC
> In my opinion, we should change this behavior to the default back/forward mouse button behavior: always trigger the history navigation on back/forward button click. 

There already is a bug exactly for this: https://bugs.kde.org/show_bug.cgi?id=310288

For me, I don't mind it to be either way (I get used to press those buttons only outside items, so it won't make difference).
Comment 7 Frank Reininghaus 2013-11-12 10:32:31 UTC
The patch looks good to me - thanks! Maybe a comment wouldn't hurt though, to explain the point of the m_pressedIndex check (and that it should be revisited if the "select items when pressing bakc/forward button" behavior is ever changed).

Do  you have a git account, or shall we push the commit for you?

(In reply to comment #5)
> @Frank, @you
> What do you think?

Well, I don't have such a mouse and have never used one, so I'm probably not the best person to ask ;-)

But anyway, on the one hand, I understand that the point of this behavior is to make selecting items in single-click mode more convenient. On the other hand, we already have the selection markers, Ctrl+click, and rubberband selection for that, and I'm not sure if we really need one more way to select items, in particular if we consider that quite a few users expect that the back/forward buttons are only used for navigating back/forward, which is understandable.

In any case, any additional comments about this issue should be added to bug 310288.
Comment 8 David Rosca 2013-11-13 16:02:20 UTC
> Do you have a git account, or shall we push the commit for you?

I don't have account at KDE git, can you please push it for me? Thanks
Comment 9 Frank Reininghaus 2013-11-14 08:12:46 UTC
Git commit 41ece8e93da5058a14b2431d730f055a0653b44d by Frank Reininghaus, on behalf of David Rosca.
Committed on 14/11/2013 at 08:08.
Pushed by freininghaus into branch 'KDE/4.11'.

Do not select items when navigating back/forward with the mouse

If we detect that the user pressed the back/forward buttons while
hovering the empty space of the view, such that
DolphinView::slotMouseButtonPressed(int, Qt::MouseButtons) will request
that Dolphin navigates back/forward in the history, handling the mouse
press event should stop. This prevents the possible unexpected selection
of items in the new directory.
FIXED-IN: 4.11.4

M  +8    -0    dolphin/src/kitemviews/kitemlistcontroller.cpp

http://commits.kde.org/kde-baseapps/41ece8e93da5058a14b2431d730f055a0653b44d