Update filename in fullscreen view on rename
ClosedPublic

Authored by slenz on Feb 22 2018, 1:55 PM.

Details

Summary

When a file was renamed in fullscreen view the filename displayed in InformationLabel was not updated until you navigated to another image and back. This was caused by the current url not being updated, which also resulted in edits and saves affecting the wrong image. This patch fixes the missing url update. Also makes full screen thumbnailBar scroll to the new file similar to other thumbnailviews.

BUG: 390332
Closes T8071

Test Plan

Case 1:

  1. Open image in fullscreen view.
  2. Rename image using F2 or right click menu.
  3. Verify that all visible filenames are updated, and that navigation between images still work.

Case 2:

  1. Open image in normal view.
  2. Rename image using F2 or Operations sidebar.
  3. Verify that all visible filenames are updated, and that navigation between images still work.

Case 3:

  1. Select image thumbnail in browse view.
  2. Rename image using F2 or right click menu.
  3. Verify that all visible filenames are updated, and that the image is still selected.

Diff Detail

Repository
R260 Gwenview
Lint
Lint Skipped
Unit
Unit Tests Skipped
slenz requested review of this revision.Feb 22 2018, 1:55 PM
slenz created this revision.

Thanks for working on that bug. :)
Although this approach works at first sight, it looks at least somewhat unusual to me calling a signal function from outside the class.
I'll take a closer look at it.

slenz added a comment.Feb 22 2018, 7:13 PM

Thanks for working on that bug. :)
Although this approach works at first sight, it looks at least somewhat unusual to me calling a signal function from outside the class.
I'll take a closer look at it.

First time i touch any major Qt/C++ project, so I may very well be doing something dumb. :)
Thanks for taking a look!

rkflx added a subscriber: rkflx.Feb 22 2018, 8:05 PM

No time to look at the code right now, but IIRC in Fullscreen View with the sidebar shown via F4, the filename in the Information panel was being updated, as well as the window title if you peeked at it via Alt+.

I would suspect the problem or rather the fix should be somewhere in the top bar, because only that part is broken?

rkflx added a comment.Feb 22 2018, 8:05 PM

And BTW, @slenz thanks for working on Gwenview ;)

muhlenpfordt added a comment.EditedFeb 23 2018, 8:27 AM

... in Fullscreen View with the sidebar shown via F4, the filename in the Information panel was being updated, as well as the window title if you peeked at it via Alt+.

That's a good hint. ;)
ContextManager already 'knows' something has changed and emits both selectionChanged and selectionDataChanged signals. The sidebar information widget InfoContextManagerItem connects exactly these two to update its content. I think it would be a more general way to use this here too than just look at the rename case.

... in Fullscreen View with the sidebar shown via F4, the filename in the Information panel was being updated, as well as the window title if you peeked at it via Alt+.

That's a good hint. ;)
ContextManager already 'knows' something has changed and emits both selectionChanged and selectionDataChanged signals. The sidebar information widget InfoContextManagerItem connects exactly these two to update its content. I think it would be a more general way to use this here too than just look at the rename case.

Well this turned out to be more complicated than expected...
So adding connections to those signals does trigger an "update" after being renamed, but mCurrentDocument is still the old document, thus "updating" to the old filename. The reason the sidebar works is that it uses the selected files (which are correct). But I can't use that for the label, as that wold break in compare mode (and would just be a workaround anyways). (One of) the root cause(s) seems to be that currentUrlChanged is never triggered on a rename, so mCurrentDocument is not updated. There is an identical bug for the save bar, where the url is also updated using a line like this:

connect(mContextManager, SIGNAL(currentUrlChanged(QUrl)), mSaveBar, SLOT(setCurrentUrl(QUrl)));

It can be triggered in this way:

  1. Open image a.jpg
  2. Rename image to b.jpg
  3. Edit it (which by the way results in another bug, the preview is not updated)
  4. The save bar now pops up, click save.
  5. You will now have both a.jpg and b.jpg

May be the same as bug: 104056 (not the crash, but the duplicate files).
The slideshow is updated in the same way, so I guess there is a similar bug there.

So tecnically the filename displayed in full screen is correct, we are actually still viewing the old image...

My patch only seems to fix the issue partially. In the save scenario this happens:

  1. Open image a.jpg
  2. Rename image to b.jpg
  3. Edit it (preview still does not update)
  4. The save bar now does notice that it is another image that was modified and says "One image modified | go to it".

So now everything that subscribes to currentUrlChanged seems updated correctly, but editing an image still edits the old one.

I'll try to find a fix later, now i need to stare at something else for a while :D

Maybe we could use ContextManager::setUrlToSelect() (or via MainWindow::goToUrl() as GvCore::saveAs() does) and just switch to the new image? The image is loaded anyway as soon as it appears so I think it does not cost more.
But to get this working we have to handle/remove this part first - same problem as discussed in D9342.
@rkflx What do you think? Do you see any other way?

rkflx added a comment.EditedFeb 24 2018, 5:56 PM

Maybe we could use ContextManager::setUrlToSelect() (or via MainWindow::goToUrl() as GvCore::saveAs() does) and just switch to the new image? The image is loaded anyway as soon as it appears so I think it does not cost more.

In general Rename should be quite similar to Save As in terms of Gwenview's internal state (ignoring the extra copy on disk of the old file), so this makes sense from a high level. However, before we fix D9342 and D10745, the following problem worries me a bit more, because if we are unlucky this might be a much bigger issue than just the preview not updating (thanks @slenz for finding this!):

It can be triggered in this way:

  1. Open image a.jpg
  2. Rename image to b.jpg
  3. Edit it (which by the way results in another bug, the preview is not updated)
  4. The save bar now pops up, click save.
  5. You will now have both a.jpg and b.jpg

I cannot reproduce with 17.08.3. Could you git bisect to find the culprit? You can use -DBUILD_TESTING=OFF for CMake and trigger with F2 and Ctrl+L to save time. If you find something, please open a new task on Gwenview's workboard, so we don't clutter this Diff too much.


But to get this working we have to handle/remove this part first - same problem as discussed in D9342.

Did not forget about this, it's on my list. But I don't want to let the recent influx of new (and not-so-new-anymore ;) contributors waiting for reviews either, so the situation is a bit unfortunate…

BTW, just uploaded my WIP patch to P170, still needs some thinking about consequences though. Sadly does not help directly yet.

@rkflx What do you think? Do you see any other way?

Sorry to leave you hanging here, but until I looked deeper into it (which I shouldn't, because then my backlog will grow forever…) I have no good idea. Let's solve the regression first, then we'll see what happens.

slenz updated this revision to Diff 27954.Feb 24 2018, 8:36 PM

Switch from signal currentUrlChanged to function setCurrentUrl, which solves the same issue, but also sets the new url, solving T8071.

Looks much better. :)
Just found a little issue with the cached image:
Rename a.jpg to b.jpg -> Edit -> Save -> Rename b.jpg back to a.jpg -> the unedited image is viewed.
This could be fixed with a DocumentFactory::instance()->forget(oldUrl) as in delOrTrash() above - but leads to loosing unsaved changes when renaming.
Personally, that would not bother me. ;)

app/fileopscontextmanageritem.cpp
354

Should we move this inside rename()?

rkflx added inline comments.Feb 25 2018, 9:51 AM
app/fileopscontextmanageritem.cpp
354

My plan was to do the opposite: Move everything contextManager related to fileopscontextmanageritem.cpp, because I think D6379 muddled the separation a bit.

I'd leave it as is for this patch, and once this one is in I'll send a follow-up fixing it everywhere (if you all agree).

muhlenpfordt added inline comments.Feb 25 2018, 10:00 AM
app/fileopscontextmanageritem.cpp
354

I don't care about the place, my intention was just to keep it in one place - agreed. ;)

Just found a little issue with the cached image:
Rename a.jpg to b.jpg -> Edit -> Save -> Rename b.jpg back to a.jpg -> the unedited image is viewed.
This could be fixed with a DocumentFactory::instance()->forget(oldUrl) as in delOrTrash() above - but leads to loosing unsaved changes when renaming.
Personally, that would not bother me. ;)

Can we show the "Unsaved changes" warning dialog for Rename and Move To (possibly in a follow-up patch)?

Another idea: Not sure how the "edited" state is handled internally, but would it be possible to move it along with any renames, i.e. attach this state to the actual/current filename instead of the original filename?

muhlenpfordt accepted this revision.Feb 25 2018, 11:13 AM

Can we show the "Unsaved changes" warning dialog for Rename and Move To (possibly in a follow-up patch)?

Another idea: Not sure how the "edited" state is handled internally, but would it be possible to move it along with any renames, i.e. attach this state to the actual/current filename instead of the original filename?

I think we keep this patch as is and preserve that for another one. Ok?

This revision is now accepted and ready to land.Feb 25 2018, 11:13 AM

Haha, was just about to submit the same comment:

Okay, pondered about this some more. I think we should land this patch as is to stable in order to fix 390332 and T8071. I'm not really comfortable with doing more invasive changes with only a week left for 17.12.3 (which is the last in this series).

After this, we'll see about the remaining issues for master:

  • Handling of Rename and Move To wrt. to unsaved changes. I guess the warning dialog idea is not all that great, but DocumentFactory sounds interesting. Would DocumentFactory::slotSaved(const QUrl &oldUrl, const QUrl& newUrl) already cover what we are looking for (perhaps with the appropriate selection and currentUrl handling)?
  • D9342: Make sure, a newly saved image is selected in ContextManager, possibly solvable in conjunction with P170, but maybe slotSaved alone will be enough?

Let me do some final tests, then you can land.

Damn, just discovered...
Renaming a file in browse mode and switching to fullscreen view mode - the filename is the old one.

Renaming a file in browse mode and switching to fullscreen view mode - the filename is the old one.

Perhaps because of the else in FileOpsContextManagerItem::rename? I guess for Browse mode we'd also need to call setCurrentUrl. Where to get newUrl from, though?


@slenz Apart from that it worked really well for me. Only one thing found for a later patch: In normal mode, renaming causes the Thumbnail Bar to scroll to the new entry, while it does not yet in fullscreen mode.

Also, please let Peter know your email address, so he can land on your behalf (you did not use Arcanist yet to upload the patch, which would have included this).

Only one thing found for a later patch: In normal mode, renaming causes the Thumbnail Bar to scroll to the new entry, while it does not yet in fullscreen mode.

I have a fix for that. Should i update the current patch or upload a new one?

Also, please let Peter know your email address, so he can land on your behalf (you did not use Arcanist yet to upload the patch, which would have included this).

silas.lenz@gmail.com

Only one thing found for a later patch: In normal mode, renaming causes the Thumbnail Bar to scroll to the new entry, while it does not yet in fullscreen mode.

I have a fix for that. Should i update the current patch or upload a new one?

That was quick ;)

If it's a simple one-liner, add it here as it is slightly related. If it is more complicated, better open a new Diff to keep the changes easier to understand when looking at them in the future. If the change depends on this Diff, see here.

slenz updated this revision to Diff 28012.Feb 25 2018, 12:16 PM
slenz edited the summary of this revision. (Show Details)

Add missing scrollToSelectedIndex() for full screen thumbnailBar.

slenz updated this revision to Diff 28014.Feb 25 2018, 12:19 PM

Remove missed debug line.

muhlenpfordt accepted this revision.Feb 25 2018, 12:52 PM

Everything looks good to me - thanks. :)
@rkflx Landing to stable & master?

rkflx accepted this revision.Feb 25 2018, 1:08 PM

Everything looks good to me - thanks. :)

Thanks everyone. Let's land this, and worry about the rest later.

@rkflx Landing to stable & master?

Please land to stable and merge to master afterwards – as usual ;)

This revision was automatically updated to reflect the committed changes.

Perhaps because of the else in FileOpsContextManagerItem::rename? I guess for Browse mode we'd also need to call setCurrentUrl. Where to get newUrl from, though?

The rename in browse mode is triggered by setting the new name in PreviewItemDelegate::setModelData(). The rename itself is done inside KDirModel::SetData().
I played a bit with the KDirLister::refreshItems() signal. It provides an old and a new KFileItem so we could detect a rename in ContextManager without an external trigger. Maybe this helps this when cleaning up the ContextManager stuff?