Bug 292250 - While holding Ctrl, clicking an item to unselect it clears the selection
Summary: While holding Ctrl, clicking an item to unselect it clears the selection
Status: RESOLVED FIXED
Alias: None
Product: dolphin
Classification: Applications
Component: general (show other bugs)
Version: 16.12.2
Platform: Unlisted Binaries Linux
: NOR normal
Target Milestone: ---
Assignee: Peter Penz
URL:
Keywords: reproducible
: 292459 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-01-23 04:57 UTC by Ahmad Samir
Modified: 2012-02-07 08:00 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ahmad Samir 2012-01-23 04:57:04 UTC
Version:           unspecified
OS:                Linux

While holding Ctrl, clicking an item to unselect it clears the selection; while Ctrl is held down the selection shouldn't be cleared, IIUC.

Reproducible: Always

Steps to Reproduce:
- Select multiple files/folders, e.g. 1,2,3
- Hold Ctrl and click e.g. 2 to unselect it

Actual Results:  
The other items become unselected and only 2 is selected

Expected Results:  
4 becomes unselected and the selection isn't cleared.
Comment 1 Ahmad Samir 2012-01-23 04:58:53 UTC
Sorry, s/4/2/ in the Expected results.
Comment 2 Ahmad Samir 2012-01-23 06:09:36 UTC
(Forgot to say, this is with current git master head).
Comment 3 Frank Reininghaus 2012-01-23 08:24:02 UTC
Thanks, I can reproduce that. I think that this used to work correctly some time ago, it looks like it has been broken again by some other bug fix.

I'll try to fix this ASAP, but I'm afraid it's too late for KDE 4.8.0 already.

And then I'll work on a unit test for KItemListController to make sure that this kind of regression does not happen again.
Comment 4 Frank Reininghaus 2012-01-23 13:48:24 UTC
I think that this regression has been caused by my commit

https://projects.kde.org/projects/kde/kde-baseapps/repository/revisions/9f711b5f2e1d1fd856cd6b033e6adb96f9b46d8a

If I'm right, one way to fix it to replace the line (which has been added to KItemListControllerr::mousePressEvent() in that commit)

 } else if (pressedItemAlreadySelected && (event->buttons() & Qt::LeftButton)) {

by

 } else if (pressedItemAlreadySelected && !shiftOrControlPressed && (event->buttons() & Qt::LeftButton)) {

I can't test or even commit that atm though because I'm not at home. I'll do that tonight and ask the release team if this can still be included in the final KDE 4.8.0 tarballs.
Comment 5 Ahmad Samir 2012-01-23 14:41:26 UTC
(In reply to comment #4)
> I think that this regression has been caused by my commit
> 
> https://projects.kde.org/projects/kde/kde-baseapps/repository/revisions/9f711b5f2e1d1fd856cd6b033e6adb96f9b46d8a
> 
> If I'm right, one way to fix it to replace the line (which has been added to
> KItemListControllerr::mousePressEvent() in that commit)
> 
>  } else if (pressedItemAlreadySelected && (event->buttons() & Qt::LeftButton))
> {
> 
> by
> 
>  } else if (pressedItemAlreadySelected && !shiftOrControlPressed &&
> (event->buttons() & Qt::LeftButton)) {
> 

Changing that line seems to work as expected here.
Comment 6 Frank Reininghaus 2012-01-23 18:30:31 UTC
Git commit 84a9cc4bf6e9decc4c102102c4b04162369eb0fe by Frank Reininghaus.
Committed on 23/01/2012 at 19:28.
Pushed by freininghaus into branch 'KDE/4.8'.

Make sure that Control+click toggles the selection state

This commit fixes a regression caused by the recent commit
9f711b5f2e1d1fd856cd6b033e6adb96f9b46d8a.

M  +1    -1    dolphin/src/kitemviews/kitemlistcontroller.cpp

http://commits.kde.org/kde-baseapps/84a9cc4bf6e9decc4c102102c4b04162369eb0fe
Comment 7 Frank Reininghaus 2012-01-23 18:39:07 UTC
Git commit e8bfc8724b441b70e440cad05983134975facc8b by Frank Reininghaus.
Committed on 23/01/2012 at 19:28.
Pushed by freininghaus into branch 'master'.

Make sure that Control+click toggles the selection state

This commit fixes a regression caused by the recent commit
9f711b5f2e1d1fd856cd6b033e6adb96f9b46d8a.
(cherry picked from commit 84a9cc4bf6e9decc4c102102c4b04162369eb0fe)

M  +1    -1    dolphin/src/kitemviews/kitemlistcontroller.cpp

http://commits.kde.org/kde-baseapps/e8bfc8724b441b70e440cad05983134975facc8b
Comment 8 Frank Reininghaus 2012-01-23 18:46:56 UTC
(In reply to comment #5)
> Changing that line seems to work as expected here.

Thanks for testing! I pushed the fix to the 4.8 and master branches and asked the release team to include it in the final 4.8.0 tarballs. I'm not sure if it's already too late for that though.

Anyway, thanks for your help and sorry for the trouble!

@Peter: The code which is affected by the regression and the fix can be written in a nicer way, but wanted to change as little as possible now to avoid further trouble. I'll change that when I've got a unit test for KItemListController ;-)
Comment 9 Peter Penz 2012-01-23 20:22:28 UTC
Thanks Frank for investigating, sounds like a good plan :-)
Comment 10 Jekyll Wu 2012-01-26 13:23:25 UTC
*** Bug 292459 has been marked as a duplicate of this bug. ***
Comment 11 Paul van Erk 2012-02-07 07:50:31 UTC
Glad to see this is fixed :) I'm running 4.8 and it's still there so I reckon I'll have to wait to get the right behaviour in 4.8.1?
Comment 12 Ahmad Samir 2012-02-07 08:00:18 UTC
(In reply to comment #11)
> Glad to see this is fixed :) I'm running 4.8 and it's still there so I reckon
> I'll have to wait to get the right behaviour in 4.8.1?

(Or ask your distribution package maintainers to include the patch from comment#6).