After an image is Saved As... in View Mode, the current image
and selection in thumbnail bar are not updated. As a side effect the
view jumps to the first item in directory instead of showing the newly
saved image. This is caused by D8934.
This patch makes sure, the new image is selected in ContextManager
and the window title is updated correctly.
Details
- Reviewers
rkflx - Group Reviewers
Gwenview - Commits
- R260:b19e0229b0d2: Make sure, a newly saved image is selected in ContextManager
- Open an image in View Mode
- Use File → Save As and save image as e.g. "filename2.jpg"
- Image "filename2.jpg" should be viewed and window title updated
- Check that File → Save As also works correctly after image modifications (resize, crop, etc.)
- Check with option Keep same zoom and position and 100% or a custom zoom factor active
- Check current selection while toggling between View/Browse Mode after File → Save As
- Check that Go back to the original works
- (Some more special tests in D9342)
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.
You should become Gwenview's main tester, as you seem to have uncovered another regression caused by D8196 if I did my testing correctly ;)
My issue with the patch is mainly that your fix is only of cosmetic nature, i.e. while it displays the correct filename, the actual selected image (currentIndex??) is still not set correctly. This can be seen here:
- Del deletes the wrong image.
- The thumbnail strip does not show which image is selected.
- ⎋ (← that's the ISO symbol for Esc) switches to Browse mode, but does not highlight the correct image like it normally would.
Looking at D8196 might give you some hints how to approach a fix. My gut feeling is that after that problem is fixed, you won't have to do your mSelectFirstItemWhenDirListerCompleted workaround (which is a bit ugly…), because hasSelection will be true.
Also, Gwenview has File → Save As / Ctrl+⇧+S, which is still broken with your patch. I suspect it has the same root cause and hopefully the same fix.
Ok, your're right again - you just have the better overview. ;)
The problem is the delayed appearence of the new file in the dirmodel. Since the initial KDirLister is finished, ContextManager::selectUrlToSelect() fails to update the selection.
Forcing a KDirLister update and waiting for completion, seems to fix this. This also solves the initial problem, jumping to the first index.
Also, it looks like the newly saved image is never loaded from file as stated ("You are now viewing the new document"). Ok, the image should be nearly the same as before saving.
Doing a reload() in Document::slotSaveResult updates image and title bar.
I don't know, if this is really wanted or the display should by updated on another way?
Works now as it should, but I'm still not feeling 100% confident (but a lot better than with the previous iteration :)
After playing around a bit, we essentially try to solve two issues (see inline comments for details):
- mDirListerFinished clearing the selection (introduced in D8196)
- caption not updating
lib/contextmanager.cpp | ||
---|---|---|
181 | setCurrentDirUrl can be called in all sort of situations and I'm not sure the fix (even if guarded by the if) is needed in all of these, e.g. when going up to the parent directory. When debugging you might end up in selectUrlToSelect, where for Save As the new filename is in mUrlToSelect as expected. However, at this point mDirModel does not seem to have a correct index for us yet as you noted. Should we update mDirModel somewhere else, then? Turns out we don't have to: Without your patch, simply disabling the else if clause in selectUrlToSelect is enough to fix the problem, because eventually the selection will be updated after all (it is just cleared too eagerly in the else if). We could try to get rid of mDirListerFinished completely (it is a bit ugly, anyway). In D8196#160524 I have a long list of test cases, and there is a list of bugs in the summary as well. If you give me a couple of days, I can test those again and ask Valeriy what he thinks about this. | |
lib/document/document.cpp | ||
325 ↗ | (On Diff #24096) | I agree that to reload completely (again) seems like overkill. In DocumentFactory::slotSaved we emit some signals, however according to gammaray only in DocumentInfoProvider::DocumentInfoProvider there is a corresponding connect. In my testing, it is solely the caption that would need an update. What if we added another connection reacting to said signal at the right place to something updating the caption? (BTW, it's the same old reason we don't see the bug for Resize: Here the caption is updated by the animation hiding the white toolbar.) |
I wait until the first point is clarified, before updating the diff. No hurry. ;)
Or should I separate the second issue into a new diff?
lib/contextmanager.cpp | ||
---|---|---|
181 | I did not dare to change this behaviour in selectUrlToSelect(), since I do not understand what could be the problem here with "dragging files". | |
lib/document/document.cpp | ||
325 ↗ | (On Diff #24096) | I can't find any loading of the new image from file.? But ok, reloading is really not neccesary in normal operation. ;) |
I don't mind really regarding the second issue. Either here or in a new Diff (with Depends on D9342 added).
But no hurry indeed, other reviews and patches waiting in my queue anyway and VLC is also showing funny icons these days…
Short status update: I checked the list of test cases, those still seem to work (found https://bugs.kde.org/show_bug.cgi?id=306835#c3, though). I'll continue in a bit.
Please consider to bring this patch upstream soon "as is" even if if it can be possibly improved.
I was asked by nervous colleges to solve this problem for them. It occurred for them in the context of cropping/save as.
This patch will do the job.
Thanks Peter for the finding, answering my "unrelated" questions and pointing me here!
Bernhard
@bschiffner Thanks for your concerns. Please note that the ContextManager is quite delicate to work on, as can be seen by the many implications D8196 had after it landed.
I can try to work on this topic first (if Peter does not mind other patches falling behind), but surely you'll understand that pressure from comments on Phab is not a good reason to rush things in.
Tested https://phabricator.kde.org/P170 + https://phabricator.kde.org/differential/diff/33278/ and it works for me.
(Experience tells me that it is better to act in a timely manner, if I get something told "expressis verbis" from early adaptors ...)
It's not about whether it works for your case, but about whether it does not break other cases. Checking the latter is the hard part, because the interactions involved here are quite unexpected in some cases…
Just a quick heads-up on what I worked on all weekend:
P170 is a dead-end, because it breaks showing the error message for non-existing documents. Sadly I discovered this only very late in my validation efforts. First I thought I found a way to fix this, but in the end there were even more problems… From a high level, showing the error and jumping to the new image seem like conflicting goals with the current implementation.
I still think the concept of mDirListerFinished is a bit fishy, so I wanna give searching for a better solution one more go. Sorry for the wait :/
Short report on this issue: I worked pretty hard on this the last weekend and also the weeks before. Every time I think I got it, there is one minor detail breaking everything again. There must be a way to eliminate mDirListerFinished (and meanwhile I also suspect mUrlToSelect can and should go), however I still have not found it :/
Nevertheless, by sheer luck I found that picking a single change from P170 is already enough to solve at least the issue here. The only thing left was to update the caption. After wasting lots of time trying to fix this, I remembered my analysis in D9342#inline-42793, and Peter's tip in D9342#inline-42827 was spot on – thank you so much!
Fingers crossed we don't find any remaining issues this time! I'll come up with a test plan, do more testing and post a Diff later, hopefully in time for the RC (sorry for the cliffhanger :).
Okay, here's my patch (don't laugh, I know it looks fishy):
Also, let's just reuse your Diff, given that you did the initial investigation and should probably get most of the credit. I'll accept once you updated, tested and are happy with the patch.
Here's what I tested and can confirm to work fine:
- In View mode, Save As a modified as well as an unmodified image (to rule out that the save bar animation updates the title for Fit):
- selected thumbnail in thumbnail bar is correct
- modification state (e.g. orientation for Rotate) of old/new thumbnail is correct
- window title changed
- filename in Information sidebar changed
- name and thumbnail in fullscreen thumbnail bar are correct
- reload works
- Go back to original image works
- Modify image, navigate away (different folder), check that clicking on "Go to it" works, and the folder in the sidebar is expanded correspondingly.
- gwenview ~/image.png selects correct image in thumbnail bar.
- test cases in D8196#160524
- display of error message "image" works:
- gwenview ~/non-existing-image.png
- gwenview ~/unsupported-file.type
- Open, type non-existing name (tested from both View and Browse modes)
- Selecting the first image when entering a folder still works.
- Switching between Browse and View by clicking on an entry in the Folders sidebar works (also in combination with the previous point).
- Save As in Browse mode unaffected.
- Press ⎋ to switch to Browse, correct image is selected.
- Rename an image in View mode.
- Check Keep same zoom, zoom to 100% and navigate around.
I guess it's not really worth to add all of that to the test plan (it's more a reminder of things to look at in case we find a way to remove mDirListerFinished), but please try to check for anything I might have missed to test.
Main point is - it works. Many thanks for the deep investigations! :)
I needed some time to find a situation where the update on DocumentFactory::documentChanged signal is needed. This seems to be the case only if the zoom level is 100% or custom and Keep same zoom is active. Maybe we should add this to your list (if I did not miss it somewhere).
Oops, I think there was a misunderstanding. P250 was against the stable branch, not against your patch. The point of my investigations was to find a way so that your call to updateDirectory would not be needed, because as far as I can see the model will actually update itself eventually. As for the title, I thought the connect would be better than the full-blown reload.
(Looking at it this way, you'll find that the connect will be needed in a lot more situations ;)
Thanks a lot to all of you for finding and fixing this bug!
It is working in Gwenview as of today's KDE neon unstable edition.
I own you a beer (or some other drink of your choice). Do you come to Wien next week?
Bernhard