Bug 366796 - Ctrl-click should never, ever deselect all pieces
Summary: Ctrl-click should never, ever deselect all pieces
Status: CONFIRMED
Alias: None
Product: palapeli
Classification: Applications
Component: general (show other bugs)
Version: 2.0
Platform: Fedora RPMs Linux
: NOR normal
Target Milestone: ---
Assignee: Stefan Majewsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-15 17:30 UTC by Matthew Woehlke
Modified: 2016-08-21 07:01 UTC (History)
3 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 Matthew Woehlke 2016-08-15 17:30:13 UTC
It is very, very frustrating when I am trying to select a bunch of pieces by clicking on them individually while holding ctrl that palapeli drops my entire selection if the mouse is over the wrong pixel when I click.

Multi-select behavior should work like it does in every other application. Specifically:

- A ctrl-click on empty space DOES NOTHING. It absolutely, positively should not deselect every piece I've worked hard to select.
- A ctrl-click-and-drag should ADD to the selection, not replace it.

(It would also help if there was a little fudge factor for clicking on pieces, but that probably deserves a separate report.)

Reproducible: Always

Steps to Reproduce:
1. Select a piece.
2. Ctrl-click on an empty area.

Actual Results:  
Selected piece is deselected.

Expected Results:  
Nothing (selection unchanged).

I'm not familiar enough with how palapeli is implemented, but it occurs to me the problem might be in the QGV frameworks? (In which case I'm happy to punt this to Qt...)
Comment 1 Ian Wadham 2016-08-16 03:49:13 UTC
(In reply to Matthew Woehlke from comment #0)
> It is very, very frustrating when I am trying to select a bunch of pieces by
> clicking on them individually while holding ctrl that palapeli drops my
> entire selection if the mouse is over the wrong pixel when I click.
> 
I agree, but perhaps not with a second "very"... :-)  Also the procedures for
deselecting a whole group of selected pieces or just one piece from such a
group should be clearer and more intuitive.

> Multi-select behavior should work like it does in every other application.
> Specifically:
> 
> - A ctrl-click on empty space DOES NOTHING. It absolutely, positively should
> not deselect every piece I've worked hard to select.
> - A ctrl-click-and-drag should ADD to the selection, not replace it.
> 
Is there a standard on this?  If so, Palapeli should follow it.

> (It would also help if there was a little fudge factor for clicking on
> pieces, but that probably deserves a separate report.)
> 
Agreed, but the little hand that appears is a reliable indicator, even
for pieces that are currently too small to see properly.
Comment 2 Ian Wadham 2016-08-16 04:13:54 UTC
(In reply to Matthew Woehlke from comment #0)
> I'm not familiar enough with how palapeli is implemented, but it occurs to
> me the problem might be in the QGV frameworks? (In which case I'm happy to
> punt this to Qt...)

I have not checked the code, but I am pretty sure this behavior is nothing to do
with Qt or QGraphicsView.

There are some other mouse operations I am not happy about in Palapeli, such
as the possibility of crashing Palapeli when dragging and joining pieces. 

Unfortunately I am not in a position to do any serious maintenance, due to my
advanced age and the fact that I am on an Apple computer and do not yet have
KF5.  I last worked on Palapeli about a year ago.

If you are interested in fixing Palapeli, I could help you find the pieces of code
that need work and perhaps suggest some patches.

The position of maintainer is also vacant and porting to Frameworks is incomplete.
Comment 3 Matthew Woehlke 2016-08-17 14:45:11 UTC
(In reply to Ian Wadham from comment #1)
> (In reply to Matthew Woehlke from comment #0)
> > Multi-select behavior should work like it does in every other application.
> 
> Is there a standard on this?  If so, Palapeli should follow it.

Certainly there is the way it works in QAbstractItemView and derived classes. AFAIK this is consistent (most Qt applications at least are relying on this for selection behavior, and I think also most of the major toolkits agree on this selection behavior). Anyway, it makes sense to follow what QAbstractItemView does.

Note of course I am talking about the ExtendedSelection mode, which seems to be the most commonly used, at least for instances when you think about selecting items (e.g. not systemsettings, configuration dialogs). Compare for example the behavior in gwenview or dolphin.

(In reply to Ian Wadham from comment #2)
> I have not checked the code, but I am pretty sure this behavior is nothing
> to do with Qt or QGraphicsView.

Sure; looking over the documentation, it seems QGraphicsView has some helpers for selecting things, but isn't primarily responsible for selection behavior. Previously, I didn't know if it was or wasn't.

> There are some other mouse operations I am not happy about in Palapeli, such
> as the possibility of crashing Palapeli when dragging and joining pieces. 

Ah... yeah, noticed that too :-).
Comment 4 Christoph Feck 2016-08-18 23:09:03 UTC
Ian, while Palapeli has a KF5 port lingering at the frameworks branch, the released version is still KDE4 based. So hack ahead ;)
Comment 5 Ian Wadham 2016-08-19 12:57:23 UTC
(In reply to Christoph Feck from comment #4)
> Ian, while Palapeli has a KF5 port lingering at the frameworks branch, the
> released version is still KDE4 based. So hack ahead ;)
Thanks, I know that, but I have retired from KDE programming, permanently.
Comment 6 Ian Wadham 2016-08-19 13:19:30 UTC
@Matthew: I had a brief look at how Palapeli is handling selects. It is mostly home-grown and could be changed, but I won't be fixing it.  See my reply to Christoph.

QAbstractItemView is part of the model/view classes (lists, tables and trees). I suppose it would apply to selections in the view of the puzzle collection (a list view), but it does not apply to pieces on a puzzle table.

Do you wish to make the changes and commits to fix this bug? If so, I can help you get started.
Comment 7 Stefan Majewsky 2016-08-19 13:24:45 UTC
(In reply to Ian Wadham from comment #6)
> @Matthew: I had a brief look at how Palapeli is handling selects. It is
> mostly home-grown and could be changed, but I won't be fixing it.  See my
> reply to Christoph.

Yeah, that was a result of my naive over-engineering back in the day. It was on my laundry list to rip out that monstrosity and move to simple hard-coded mouse actions, see e.g. [1]. I don't have had the time for quite a while to hack on KDE (and if I had, i would push the KF5 port through first), but if anyone wants to cleanup this mess, then by all means go ahead. And CC me on the review request if you like some extra pair of eyes that's familiar with the codebase (or at least was 4 years ago).

[1] https://majewsky.wordpress.com/2012/02/13/is-anyone-using-custom-mouse-actions-in-palapeli/
Comment 8 Ian Wadham 2016-08-21 07:01:35 UTC
@Stefan: There are now two more configurable mouse actions: teleport pieces and switch to close-up or distant view.  I followed the existing scheme, with interactors, etc. because it seemed simpler to leave well enough alone.  Also the config dialog is a handy lookup-table for users even if they never change the default mouse actions.

One very nice thing about the current setup was that the functionality of Palapeli's view and scene classes came for free with every piece holder window, making it possible to do sub-assemblies within large puzzles in any window --- main puzzle table or piece holder.

There is just one problem with the interactors as far as I am aware: they can run on after a piece object has disappeared and cause a crash, e.g. in some circumstances when merging pieces and always if you do Restart Puzzle (as a keyboard shortcut) in the middle of a drag (which makes the crashes reproducible).