In some cases the titlebar information (filename, size, zoom) is lost after an image has been modified.
This patch fixes the caption update and thereby also the missing * on modified images.
BUG: 345980
Details
Details
- Reviewers
rkflx - Commits
- R260:5a7d105ae9de: Update titlebar after image modification
Case 1:
- Open image in view mode, which fits in window (100% zoom)
- Scale (Shift-R) or crop (Shift-C) image
-> Titlebar info is lost
Case 2:
- Open big image in view mode, which is zoomed to fit in window
- Scale image with Shift-R (do not save)
- Go to another big image (zoomed to fit)
- Scale image with Shift-R
-> Titlebar info is lost
Diff Detail
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.
Comment Actions
Case 1 can be fixed by calling updateCaption in BirdEyeView::slotZoomOrSizeChanged().
In the second case the titlebar is cleared out inside MainWindow::slotModifiedDocumentListChanged().
I could not find any situation where this setCaption call is needed or the caption argument is anything else than an empty string.
It's triggered from some actions in DocumentFactory class like loading and saving an image, but the title is updated via other paths.
Maybe I missed the meaning of setting the caption in slotZoomOrSizeChanged()?
Anyway, removing the setCaption call, seems to solve the second case.
Comment Actions
Already your fourth patch and uploaded with arc too, I like the direction this is heading…
I could not spend too much time on the review yet (or rather understanding the title update mechanisms), but here are some initial comments / thoughts:
- Adding some debug messages to DocumentViewPrivate::updateCaption, your patch increases the number of calls to this from 3 to 9 when simply advancing to the next image. Ideally this would be 1, of course, so there is already something odd going on.
- With your patch this is called now very often for everything involving animations, e.g. when showing the Crop interface.
- setCaption is used for marking the window title with a star for modified images. This is currently broken, and I started on researching a fix for this a couple of days ago when I noticed The window title does not contain a '[*]' placeholder being logged when rotating. Not done yet, because reviewing some other patches took a lot of time… I'm not sure you should remove this, though.
- Why is the fix in the BirdEyeView? This should not be related to the window title.
- I currently don't understand the connection to the zoom levels, and having two cases in the test plan seems odd on first sight. A window title change on image modification should not depend on the previous navigation history and the zoom settings. If the bug is only triggered in those cases, this might mean the original update calls are in the wrong place.
To summarize: Your patch works, but it does feel like it just adds stuff on top of something more fundamentally broken. Ideally we could understand how this all works, identify a root cause and put a fix at the right place.
Taking a step back and not looking at the code, the window title should be updated for changes to:
- Filename → Navigating between images, renaming, switching view modes.
- Zoom level → Zooming (either manually or when changes in the UI like showing the crop bar cause a resize).
- Modified status → Any "Image Operation", including undo/redo and in every view mode.
- Image size → Cropping, resizing, rotating. (But those are already included in the point above.)
Maybe rip out all code related to this and start from scratch? Hmm…
For now, I'm setting this to "Request Changes" regarding the removal of setCaption, not sure about the rest yet.
Comment Actions
Ok, that's what I missed... ;)
- I currently don't understand the connection to the zoom levels, and having two cases in the test plan seems odd on first sight. A window title change on image modification should not depend on the previous navigation history and the zoom settings. If the bug is only triggered in those cases, this might mean the original update calls are in the wrong place.
A zoom level change triggers the title update and this happens after crop/resize in most cases. So the bug does not occur.
But you're right, this patch does not seem to be a nice solution. I will try to get behind the update mechanism.
Comment Actions
I think the hint with the missing '*' was a push to the right direction. ;)
If setting the window title in MainWindow also updates mCaption, this seems to fix the bug.
Checking the current url in the modified document list, adds the '*' for modified images even while browsing through the list.
(While testing I found another bug with my patch D8934. After modification and saving under a different name, the view jumps to the first item instead of the new file.)
Comment Actions
Amazing, this looks much more straightforward and at the same time fixes the * problem too. The flickering of the title I sometimes got is also gone :)
Could you update the summary regarding the fixed modification marker?
Taking a step back and not looking at the code, the window title should be updated for changes to: […]
Tested all those, worked great. We are nearly there regarding the patch.
Sidenote:
I think now I understood where The window title does not contain a '[*]' placeholder was coming from: Normally, you can mark the place where Qt should insert the star like so: Before star [*] After star, and if you have no explicit star it is assumed to be at the end. Now if we do setCaption("", true), there is no "end" of an empty string per se, and Qt complains.
| app/mainwindow.cpp | ||
|---|---|---|
| 847 | Add const, please. | |
| 853–854 | The comment is a bit redundant, given the code is so simple now. | |
| app/mainwindow.h | ||
| 94 | Giving the parameter a name would make this more readable. | |