Update window title when changing selection in comparison mode
ClosedPublic

Authored by slenz on Mar 6 2018, 10:45 PM.

Details

Summary

The window title now updates with the currently selected image when the user changes the selection in comparison mode.

BUG: 314724

Before:

After:

Test Plan

View and select different images with and without comparison mode and verify the title.

Resize images by resizing the window or selecting/deselecting images while in comparison mode and verify the title.

View and switch between images in normal mode to verify it still works, including title updates.

Diff Detail

Repository
R260 Gwenview
Branch
fullscreencaptionupdate (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
slenz requested review of this revision.Mar 6 2018, 10:45 PM
slenz created this revision.
slenz retitled this revision from Update window title when changing selection in comparision mode to Update window title when changing selection in comparison mode.Mar 6 2018, 10:48 PM
slenz edited the summary of this revision. (Show Details)
slenz edited the test plan for this revision. (Show Details)
slenz added a reviewer: Gwenview.
slenz added a project: Gwenview.
slenz added a subscriber: Gwenview.
rkflx added a subscriber: rkflx.Mar 6 2018, 10:51 PM

Thanks for working on this ;)

Note there is currently a bit of a review backlog for Gwenview, so it might take a while…

slenz updated this revision to Diff 28876.Mar 6 2018, 10:53 PM
  • Remove unwanted blank line
ngraham accepted this revision.Mar 10 2018, 12:25 AM
ngraham added a subscriber: ngraham.

Nice focused change. The test plan works for me, and some quick spot-checks of existing focus-related functionality all still work:

  • Switch between images
  • Rename an image
  • Copy and navigate to copied image

Lets let @rkflx take a look too before we land it.

This revision is now accepted and ready to land.Mar 10 2018, 12:25 AM

The title updates correctly when selecting the image by mouse click.
But when adding or removing images to the multi view or while resizing the window (zoom factor change) the title does not always stick to the selected image.

The title updates correctly when selecting the image by mouse click.
But when adding or removing images to the multi view or while resizing the window (zoom factor change) the title does not always stick to the selected image.

I must have gotten "lucky"/selected bad test cases when testing this, tried both removing and resizing, but you are totally right. I'll investigate.

rkflx added a comment.Mar 30 2018, 5:41 AM

@slenz Let us know if you are stuck and need some tips.

One thing I noticed is that when adding an image to the light table, e.g. via a hover button in the thumbnail bar, the window title flickers through all filenames in rapid succession. This was already the case before your patch, but it might indicate a deeper problem which might need fixing.

slenz updated this revision to Diff 31213.Apr 3 2018, 1:02 PM
  • Merge branch 'master' of git://anongit.kde.org/gwenview into fullscreencaptionupdate
  • Only update caption when the selected image is resized and not when others are
slenz updated this revision to Diff 31214.Apr 3 2018, 1:15 PM
  • Only update caption when the selected image completed loading and not when others have
slenz added a comment.EditedApr 3 2018, 1:34 PM

Got stuck on a whole bunch of assignments and then a vacation, sorry about the delay :)

This first commit fixes the flickering and wrong title when resizing or adding/removing. Still changes the zoom percentage many times due to the animation, but at least the filename is consistent.

The second fixes an issue where the window title would be reset to the last selected image when exiting and reopening (pressing Esc and then Enter) and not kept on the currently selected image.

One question: Should the rest of slotCompleted also be inside that new if? I have a feeling that the current behavior should have generated a bug where the zoom values where based on the wrong image just like the caption. But it seems to work correctly, so maybe I'm just missing something.

Edit: Just noticed that the title sometimes is kept when removing the currently selected image. No idea when or why, but happened in about 1/20 cases.

slenz edited the test plan for this revision. (Show Details)Apr 3 2018, 1:40 PM

Sorry to say - there is still a problem. When adding the 6th image, the caption jumps from 'a.jpg' to 'f.jpg' and after deselecting it again the caption still shows 'f.jpg'.

I didn't look closer to the whole update code, so I don't know what's really possible. But maybe the update-caption function should know which image is selected and use only this to create the caption?

slenz updated this revision to Diff 31218.Apr 3 2018, 2:37 PM
  • Move around caption update to catch more cases
slenz added a comment.Apr 3 2018, 2:41 PM

Sorry to say - there is still a problem. When adding the 6th image, the caption jumps from 'a.jpg' to 'f.jpg' and after deselecting it again the caption still shows 'f.jpg'.

Could not reproduce the jumping to 'f.jpg', but the deselection issue should now be solved.

Works perfect now, I can't find any more issues. Just a little style hint (see inline comment).
I hand over to @rkflx for further inspection. ;)

lib/documentview/documentview.cpp
220

Space missing. Maybe just return if !mCurrent?

slenz updated this revision to Diff 31263.Apr 4 2018, 8:15 AM
  • Style change
rkflx requested changes to this revision.Apr 4 2018, 7:27 PM

@muhlenpfordt Thanks for the review!

@slenz Thanks for the updates, and don't worry about the delay ;)

When adding qDebug() to each call site of updateCaption(), I still get a bunch of useless updates of the window title. Would it be enough to only add the function call to a single place, i.e. inside the if in setCurrent?


One question: Should the rest of slotCompleted also be inside that new if? I have a feeling that the current behavior should have generated a bug where the zoom values where based on the wrong image just like the caption. But it seems to work correctly, so maybe I'm just missing something.

Not sure what you mean by that, but in general it would be a great idea for a follow-up patch to investigate whether there are more useless calls to updateCaption or other redundant function calls.

Another idea (in case you want to keep working on the light table) would be to implement https://bugs.kde.org/show_bug.cgi?id=391895.

lib/documentview/documentview.cpp
723

Please put the { back where it belongs ;)

(But perhaps you won't need to touch this function at all, see other comment…)

This revision now requires changes to proceed.Apr 4 2018, 7:27 PM
slenz updated this revision to Diff 31324.Apr 4 2018, 7:48 PM
  • Revert to original emitFocused
slenz added a comment.Apr 4 2018, 8:08 PM

You have a point (as usual) :D

And yes, it seems like two out of four calls to updateCaption could be removed, but lets leave that for another patch.

rkflx added a comment.Apr 4 2018, 11:05 PM

inside the if in setCurrent?

lib/documentview/documentview.cpp
685

Are you sure that we need to call this for value == false?

slenz updated this revision to Diff 31378.Apr 5 2018, 11:43 AM
  • Avoid one more unneeded updateCaption call
rkflx accepted this revision.Apr 5 2018, 5:49 PM

Cool, LGTM now.

This revision is now accepted and ready to land.Apr 5 2018, 5:49 PM
This revision was automatically updated to reflect the committed changes.