The window title now updates with the currently selected image when the user changes the selection in comparison mode.
BUG: 314724
Before:
After:
ngraham | |
rkflx |
Gwenview |
The window title now updates with the currently selected image when the user changes the selection in comparison mode.
BUG: 314724
Before:
After:
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.
No Linters Available |
No Unit Test Coverage |
Thanks for working on this ;)
Note there is currently a bit of a review backlog for Gwenview, so it might take a while…
Nice focused change. The test plan works for me, and some quick spot-checks of existing focus-related functionality all still work:
Lets let @rkflx take a look too before we land it.
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.
@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.
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.
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?
Could not reproduce the jumping to 'f.jpg', but the deselection issue should now be solved.
@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?
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 | ||
---|---|---|
727 | Please put the { back where it belongs ;) (But perhaps you won't need to touch this function at all, see other comment…) |
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.
lib/documentview/documentview.cpp | ||
---|---|---|
687 | Are you sure that we need to call this for value == false? |