Update thumbnail de-/select hover button on selection change
ClosedPublic

Authored by muhlenpfordt on May 21 2018, 8:22 AM.

Details

Summary

The de-/select (-/+) hover button of the thumbnails in Browse
Mode or thumbnail bar in View Mode does not reflect the actual
selection state sometimes.
This patch updates the button state when the selection changed.

BUG: 394406
FIXED-IN: 18.04.2

Test Plan
  1. Open Gwenview in Browse Mode or View Mode with visible thumbnail bar
  2. Use e.g. Ctrl+Click to de-/select images or move selection by keyboard
  3. -/+ button of item under mouse pointer should reflect the actual selection state

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
muhlenpfordt requested review of this revision.May 21 2018, 8:22 AM
muhlenpfordt created this revision.

(Not in the scope of this fix - I just noticed that PreviewItemDelegate and ThumbnailBarItemDelegate duplicates some code but it seems not that easy to merge.)

lib/thumbnailview/previewitemdelegate.cpp
886

Not needed here (and the other place) anymore, since the select() above leads to the call.

lib/thumbnailview/thumbnailbarview.cpp
516

The selectionChangedSignal is emitted only in the direct base class in ThumbnailView::selectionChanged().

rkflx added a reviewer: huoni.May 21 2018, 8:42 AM
rkflx added a subscriber: rkflx.EditedMay 21 2018, 9:09 PM

@huoni Here's how I would start with assessing whether the patch works correctly (apply before and after arc patch and compare output on startup and when testing both keyboard and mouse navigation):

1diff --git a/lib/thumbnailview/previewitemdelegate.cpp b/lib/thumbnailview/previewitemdelegate.cpp
2index ec72940b..7f2e4290 100644
3--- a/lib/thumbnailview/previewitemdelegate.cpp
4+++ b/lib/thumbnailview/previewitemdelegate.cpp
5@@ -540,6 +540,8 @@ struct PreviewItemDelegatePrivate
6
7 void updateToggleSelectionButton()
8 {
9+ static int i;
10+ qDebug() << "update from previewitemdelegate" << i++;
11 mToggleSelectionButton->setIcon(QIcon::fromTheme(
12 mView->selectionModel()->isSelected(mIndexUnderCursor) ? "list-remove" : "list-add"
13 ));
14diff --git a/lib/thumbnailview/thumbnailbarview.cpp b/lib/thumbnailview/thumbnailbarview.cpp
15index 260c1bd8..0a72c725 100644
16--- a/lib/thumbnailview/thumbnailbarview.cpp
17+++ b/lib/thumbnailview/thumbnailbarview.cpp
18@@ -140,6 +140,8 @@ struct ThumbnailBarItemDelegatePrivate
19
20 void updateToggleSelectionButton()
21 {
22+ static int i;
23+ qDebug() << "update from thumbnailbarview" << i++;
24 bool isSelected = mView->selectionModel()->isSelected(mIndexUnderCursor);
25 mToggleSelectionButton->setIcon(QIcon::fromTheme(isSelected ? "list-remove" : "list-add"));
26 }

Sometimes it's also helpful to monitor signals in GammaRay (not sure if applicable here).

huoni added a comment.EditedMay 23 2018, 6:13 AM

If I see this correctly, the only time the buttons weren't updated was when the hover did not change, but the selection did. E.g. when Ctrl + clicking, or + arrow. Oops, just realised this was cleared up in the Test Plan.
If so, then the patch LGTM.

That said, two very minor points:

  • ThumbnailBar::selectionChanged is always triggered twice when selection changes. Seems to me there's some sort of selection changed event duplication happening somewhere
  • Changing the bar selection in View mode seemingly triggers the Browse hover buttons to update, which is unnecessary

Thanks @rflx for the debug diff. I don't think the above is too important, it's probably fine as is - there aren't many unnecessary calls, and I'd guess it's an inexpensive operation.

  • ThumbnailBar::selectionChanged is always triggered twice when selection changes. Seems to me there's some sort of selection changed event duplication happening somewhere

There are two different instances of ThumbnailBarView created in ViewMainPage and FullScreenContent, so the slot is called for both thumbnail bars.

  • Changing the bar selection in View mode seemingly triggers the Browse hover buttons to update, which is unnecessary

The selection events are sent to visible and not visible thumbnail view/bars (with or without this patch). Maybe we could prevent this, but then have to update the selection on mode change. I think this could get tricky.

huoni accepted this revision.May 24 2018, 12:01 AM
  • ThumbnailBar::selectionChanged is always triggered twice when selection changes. Seems to me there's some sort of selection changed event duplication happening somewhere

There are two different instances of ThumbnailBarView created in ViewMainPage and FullScreenContent, so the slot is called for both thumbnail bars.

  • Changing the bar selection in View mode seemingly triggers the Browse hover buttons to update, which is unnecessary

The selection events are sent to visible and not visible thumbnail view/bars (with or without this patch). Maybe we could prevent this, but then have to update the selection on mode change. I think this could get tricky.

Ah, forgot about the fullscreen bar. Yeah I don't think it's worth filtering events, it's probably just unnecessary complication. Sorry forgot to accept the patch earlier.

This revision is now accepted and ready to land.May 24 2018, 12:01 AM
This revision was automatically updated to reflect the committed changes.

@huoni Thanks for reviewing 👍

(Regarding P220, I just realized it would have been helpful to also dump this to prevent confusion… Sorry for that.)

@huoni Thanks for reviewing 👍

(Regarding P220, I just realized it would have been helpful to also dump this to prevent confusion… Sorry for that.)

That does sound helpful, I'll keep that in mind for future :)