Make sure, a newly saved image is selected in ContextManager
ClosedPublic

Authored by muhlenpfordt on Dec 15 2017, 8:48 AM.

Details

Summary

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.

Test Plan
  1. Open an image in View Mode
  2. Use FileSave As and save image as e.g. "filename2.jpg"
  3. Image "filename2.jpg" should be viewed and window title updated
  • Check that FileSave 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 FileSave 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.
muhlenpfordt requested review of this revision.Dec 15 2017, 8:48 AM
muhlenpfordt created this revision.
rkflx requested changes to this revision.Dec 15 2017, 8:58 PM

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 FileSave As / Ctrl++S, which is still broken with your patch. I suspect it has the same root cause and hopefully the same fix.

This revision now requires changes to proceed.Dec 15 2017, 8:58 PM

Rewrite to address the base problem.

muhlenpfordt retitled this revision from Select first item in directory only after directory changes to Make sure, a newly saved image is selected in ContextManager.Dec 19 2017, 8:53 AM
muhlenpfordt edited the summary of this revision. (Show Details)
muhlenpfordt edited the test plan for this revision. (Show Details)

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".
Removing the else if solves this issue. I was a little bit worried about the url to select could never appear and this will run endless, but I can't see any situation where this could happen.

lib/document/document.cpp
325 ↗(On Diff #24096)

I can't find any loading of the new image from file.?
If I block QImageWriter.write() in DocumentLoadedImpl::saveInternal() and save as png this results in a 0-byte file, but no error occurs and the image is still displayed. So, I think it's not the new image displaying.

But ok, reloading is really not neccesary in normal operation. ;)
Connecting to documentChanged() in DocumentView::DocumentView() to a slot that just updates the caption will do it.

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?

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…

rkflx added a comment.Jan 7 2018, 11:54 PM
In D9342#181752, @rkflx wrote:

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.

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.

In D9342#255369, @rkflx wrote:

@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.

No problem for me. Since this issue comes up from time to time I think it's worth taking a look again.
The latest combination of your patch P170 and the title update should be this preview.

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 ...)

rkflx added a comment.Apr 29 2018, 8:55 AM

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…

rkflx added a comment.May 13 2018, 9:58 PM

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 :/

rkflx added a comment.Jul 30 2018, 7:43 PM

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 :).

rkflx added a comment.EditedAug 1 2018, 4:57 AM

Okay, here's my patch (don't laugh, I know it looks fishy):

1diff --git a/lib/contextmanager.cpp b/lib/contextmanager.cpp
2index b04e4026..2243ea99 100644
3--- a/lib/contextmanager.cpp
4+++ b/lib/contextmanager.cpp
5@@ -348,7 +348,6 @@ void ContextManager::selectUrlToSelect()
6 // and manually set current URL
7 d->mSelectionModel->clearSelection();
8 setCurrentUrl(d->mUrlToSelect);
9- d->mUrlToSelect.clear();
10 }
11 }
12
13diff --git a/lib/documentview/documentview.cpp b/lib/documentview/documentview.cpp
14index fa9d3f5b..ee8dd85e 100644
15--- a/lib/documentview/documentview.cpp
16+++ b/lib/documentview/documentview.cpp
17@@ -464,6 +464,10 @@ DocumentView::DocumentView(QGraphicsScene* scene)
18 d->setCurrentAdapter(new EmptyAdapter);
19
20 setAcceptDrops(true);
21+
22+ connect(DocumentFactory::instance(), &DocumentFactory::documentChanged, this, [this]() {
23+ d->updateCaption();
24+ });
25 }
26
27 DocumentView::~DocumentView()

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.

muhlenpfordt edited the summary of this revision. (Show Details)
muhlenpfordt edited the test plan for this revision. (Show Details)

Added modifications from P250 by rkflx

In D9342#301657, @rkflx wrote:

Okay, here's my patch (don't laugh, I know it looks fishy): P250

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).

rkflx added a comment.EditedAug 1 2018, 4:01 PM

Added modifications from P250 by rkflx

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 ;)

In D9342#301838, @rkflx wrote:

Oops, I think there was a misunderstanding. P250 was against the stable branch, not against your patch.

Ah, indeed - the old parts are not needed anymore and it works perfect without them.

Removed unneeded parts from old diff

rkflx accepted this revision.Aug 1 2018, 9:34 PM

Let's hope for the best ;)

(I also added the fix to the release notes etherpad.)

This revision is now accepted and ready to land.Aug 1 2018, 9:34 PM

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

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?

You're welcome! Glad it works. :)
Thanks for the offer but I won't be there.