Differential D8196

Try to keep ContextManager in sync with viewed files in MainWindow
ClosedPublic
Actions

Authored by valeriymalov on Oct 8 2017, 1:02 PM.

Details

Reviewers
gateau
rkflx
Group Reviewers
KDE Applications
Commits
R260:b55420b2ac3d: Try to keep ContextManager in sync with viewed files in MainWindow
Summary

ContextManager now is responsible for switching to the directory
containing requested URL and selecting it. However, if it is not
possible, URL is still kept (in case of remote URLs), while selection is
cleared (to avoid dragging in local files)

MainWindow now relies on ContextManager's selection and/or
selectedFileItemList instead of ThumbnailView selection. If selection &
currentUrl are empty, refuse to open View tab, otherwise display
selected items.

This should prevent (reduce?) the amount of mismatches between which
files user sees, and which files are being operated upon
(e.g. by FileOpsContextManagerItem)

BUG: 355493
BUG: 275807
BUG: 326190
BUG: 306835

Test Plan

Tried playing around to make sure it doesn't break any old behaviour
Tried deleting all image files while in View mode, to make sure we back out when we run out of images
Tried opening an http url and check that operations apply to it unless we select something in browse tab
And then remote image should be unloaded from the View tab since our actions will now affect user-selected items

Tests pass but they don't seem to cover this?

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.
valeriymalov created this revision.Oct 8 2017, 1:02 PM
rkflx added a reviewer: gateau.Oct 15 2017, 8:14 AM
rkflx added subscribers: gateau, rkflx.

Thanks for the patch, Valeriy.

Does this also solve https://bugs.kde.org/show_bug.cgi?id=275807 or https://bugs.kde.org/show_bug.cgi?id=326190?

Tests pass but they don't seem to cover this?

It would be marvelous if you could add autotests, specifically for these cases:

  • Selecting an image, then pressing Del on startpage
  • In a folder with subfolders, deleting all images in view mode and pressing Del again

As the code is somewhat complex, this would ensure it won't regress again in the future.

@gateau I'm not too familiar with the code in question. Could you have a look? If you are busy (or in addition), I could give this a try myself, but next weekend at the earliest. It is a non-trivial change, so it needs thorough testing and consideration.

valeriymalov added a comment.Oct 19 2017, 3:17 AM
In D8196#155563, @rkflx wrote:

Does this also solve https://bugs.kde.org/show_bug.cgi?id=275807 or https://bugs.kde.org/show_bug.cgi?id=326190?

Yes.

It would be marvelous if you could add autotests, specifically for these cases:

  • Selecting an image, then pressing Del on startpage
  • In a folder with subfolders, deleting all images in view mode and pressing Del again

I'm not really sure how to approach that, most of the current autotests cover the code in gwenviewlib and just link with the library. MainWindow doesn't belong to that library and actually pulls in pretty much everything in the app/, so I'll have to duplicate gwenview build target for that test. Unless I'm missing something.

rkflx requested changes to this revision.Oct 22 2017, 9:43 PM

Thanks again for your patch, Valeriy, and sorry it takes so long to review.

> Does this also solve https://bugs.kde.org/show_bug.cgi?id=275807 or https://bugs.kde.org/show_bug.cgi?id=326190?
Yes.

While those bugs are all solved by your patch, they are not really duplicates. Instead, could you just add them here in the summary as BUG:s?

most of the current autotests cover the code in gwenviewlib and just link with the library

Well, in this case it would be even more important to provide more test coverage for MainWindow :)

However, it is not fair to burden you with writing them as a condition for accepting this patch. I'd say we can accept your patch (pending review) without an autotest, and if you feel motivated or have a good grasp on the code you are welcome to submit an additional patch.

Note: I already made a list of things to test, will do that in the next days and also look at the code.

This revision now requires changes to proceed.Oct 22 2017, 9:43 PM
valeriymalov updated this revision to Diff 21194.Oct 23 2017, 6:09 PM

reverted back incorrect select call, my bad

select with (ClearAndSelect | Current) doesn't seem to do work for setting both selection and current item.
In fact, original code worked as intended, e.g. current with (ClearAndSelect) sets both current item and selection.

valeriymalov edited the summary of this revision. (Show Details)Oct 23 2017, 6:10 PM
valeriymalov added a comment.Oct 23 2017, 6:46 PM

No worries, patch is rather long, I'm not very familiar with gwenview's code, already found an error (I broke next/previous buttons when opening a file via command line or open file menu, should be fixed now), and app/ code has no tests. So I suppose it'd be better to have it reviewed/tested thoroughly.

rkflx added a comment.Oct 26 2017, 7:57 PM

Regarding the behaviour, I can confirm this fixes the linked bugs – no more severe data loss. Specifically, I tested the following:

  • Select all pictures in Browse mode, open Start page, Del → pictures not deleted anymore
  • Prepare folder with a single image and a subfolder, view image, press Del twice → image not shown anymore after deletion, subfolder only deleted after being visible first
  • Select all pictures in Browse mode, File → Open a remote URL, Del → pictures not deleted anymore
  • View image, open remote URL, Browse mode, view first image again → bug gone (where remote image instead of local image was shown), but see below
  • Open image attachment from KMail, press Del twice → image not shown anymore after first deletion
  • Open image via command line or via File → Open from different folder than Browse mode is in, press Space → navigation still possible
  • Open multiple images from the command line → first image not selected anymore (I don't count this as a regression, because I actually like this behaviour more…)
  • Misc (sorting, thumbnails in sync, recents folder, undo) → still works

In terms of regressions, I found the following:

  • After showing a remote (https) image, Browse mode tries to show a directory listing of the server but only shows an empty view (without the patch we would get the last used local directory) → Could we just disable Browse mode in this case (without breaking for example fish:/, where it is working fine)?
  • No more so far :) (but a lot of old bugs…)

@valeriymalov Thoughts on how to handle the regression? Now/later/never?

As for the code, I could not find any immediate issues and just some minor nits (see below), but as I said I am not too familiar with the inner workings of Gwenview. In general your approach reads reasonable, and also some details you changed show you thought about this quite a bit. Let's just hope we did not miss something important in the details.

Since the patch changes the code quite a bit, I would propose to target this for master (17.12) and not the stable branch unless someone else can provide a more detailed code review. Even though this fixes a severe bug, we should not rush this or risk the stability of the bug fix release (17.08.3), after all the bug is several years old anyway.

@valeriymalov Would this be okay for you? How is your developer account application going? :)

app/mainwindow.cpp
938–939

add const

1113

add const

1123

This now gets called under different conditions (??) than before, i.e. not for the degenerated case (empty list) anymore. Is this intentional? (Your are probably right and I just don't see what you did there, but better safe than sorry, so please explain…)

valeriymalov updated this revision to Diff 21488.Oct 28 2017, 1:23 PM
valeriymalov marked 2 inline comments as done.
  • add const where needed
valeriymalov added a comment.Oct 28 2017, 1:57 PM
In D8196#160524, @rkflx wrote:

Thoughts on how to handle the regression? Now/later/never?

No idea right now, according to KProtocolManager::supportsListing "http" is listable and I suppose listing it just fails in most cases, but don't see an easy way to catch that to lock the user out of browse section. As far as understand that requires overriding KDirLister's error handling, so maybe later since it would add quite a bit of a code. If there's an easier way I'll probably add it into this patch.

Since the patch changes the code quite a bit, I would propose to target this for master (17.12) and not the stable branch unless someone else can provide a more detailed code review. Even though this fixes a severe bug, we should not rush this or risk the stability of the bug fix release (17.08.3), after all the bug is several years old anyway.

@valeriymalov Would this be okay for you? How is your developer account application going? :)

Yes, it's good, I wouldn't mind it getting reviewed more or getting more testing. Application is in progress :)

app/mainwindow.cpp
1123

I am not really sure which list do you mean

If we have an empty selection, then selectedFileItemList is populated by currentUrl, which is set by contextManager::slotCurrentChanged() if we have a valid currentIndex

Now the thing, I've messed around with undo a bit and I'm thinking of either reverting it or forcing currentIndex to be in sync with selection

It seems that at least Undo and Rotation both rely on currentIndex and not selection (makes sense), however ThumbnailView doesn't seem to render current index at all? At least for me with my theme settings, I can't see it. and currentIndex and selection do not always stay in sync. Example:

  • Open a folder with two images
  • Navigate to one image using arrow keys
  • Navigate to an adjacent image using CTRL+arrow key
  • Click on a rotate, I'd expect selected image to be rotated but actually another one is rotated, because one is selected and another is current.

Happens to me with both patched an unpatched gwenview unless I miss something

This is probably not in the scope of this patch, though, since maybe it's just better to fix ThumbnailView rendering current item. If it is not, then I should revert this one change back to using currentIndex.

rkflx accepted this revision.EditedOct 29 2017, 2:26 PM
In D8196#161209, @valeriymalov wrote:

Thoughts on how to handle the regression? Now/later/never?

No idea right now, according to KProtocolManager::supportsListing "http" is listable and I suppose listing it just fails in most cases, but don't see an easy way to catch that to lock the user out of browse section. As far as understand that requires overriding KDirLister's error handling, so maybe later since it would add quite a bit of a code. If there's an easier way I'll probably add it into this patch.

One more thing I noticed, in case it is related: Before, the remote image would only appear in Recent Files, now its folder appears in Recent Folders, too. Also, trying to open the folder directly via the URL bar is already blocked by Gwenview (maybe a similar check could be used for Browse mode?). But I agree, this is not super important compared to the problem below and could be fixed later.

I wouldn't mind it getting reviewed more or getting more testing. Application is in progress :)

I'm afraid there are not that many reviewers familiar with Gwenview's internal architecture (generic code quality is not the issue here). All we can do really is to put this in the hand of testers. I really would like to see your patch in 17.12, as it prevents potential data loss while we cannot seem to find anything wrong in terms of regressions for Gwenview's main use cases so far.

Looking at the schedule, this should be committed around November 12th (weekend in two weeks time) to make the Beta. Let's use the next two weeks for final improvements and reviews. If we do not find any blockers, I'd suggest to land this patch. I can do so if you haven't got commit access until then. Adding more fixes on top is less risky and could also be done before the RC or in a later stable release.

(Edit: Oops, we have one more week.)

app/mainwindow.cpp
1123

I am not really sure which list do you mean

Let me be more specific regarding the conditions for setActiveStack(0) to be called:

  • Before: !index.isValid(), giving item.isNull(), so else is entered
  • After: !list.isEmpty(), only then item.isNull() is checked and else is entered

IOW, you moved this inside the outer if. If list is empty (I guess this corresponds to an invalid index?), the active stack is not zeroed anymore, while before it was.

Not sure what's the effect of this is or how to test, but I wanted to mention this at least.

1123

I'm thinking of either reverting it or forcing currentIndex to be in sync with selection
[...] another one is rotated [...]
Happens to me with both patched an unpatched gwenview unless I miss something

Can confirm this is an old issue, but don't know enough about currentIndex to give you any hint. In general it sounds reasonable to be in sync with the selection, but can't the selection also include multiple images? As for reverting, I cannot see a reason to do so if there is no regression.

Unfortunately, there seem to be more issues regarding the selection which are no regression either. When selecting with the mouse, sidebar and toolbar often do not allow to actually perform image operations on the image. This definitely should be solved at some point.

I'd suggest to do one step at a time, i.e. a followup patch with your sync idea trying to solve the selection issues would be great.

This revision is now accepted and ready to land.Oct 29 2017, 2:26 PM
valeriymalov updated this revision to Diff 21594.Oct 30 2017, 10:33 PM

revert back undo stack selection

currentIndex handling should be untangled in a different patch

rkflx added a comment.Nov 4 2017, 12:19 PM

Congrats on getting your commit access approved :)

according to KProtocolManager::supportsListing "http" is listable and I suppose listing it just fails in most cases, but don't see an easy way to catch that to lock the user out of browse section.

Note that for the cases I observed the issue in, Gwenview prints Found no root index. Would it be feasible to just add FolderViewContextManagerItem::findRootIndex to the check?

revert back undo stack selection
currentIndex handling should be untangled in a different patch

Ok, great. Tested briefly again, seemingly the rest still works.

I also stumbled upon one more non-duplicate bug your patch solves, please add it to the summary if you can confirm: https://bugs.kde.org/show_bug.cgi?id=306835

Regarding the multitude of selections issues mentioned in various comments above, it occurred to me that it's probably helpful not to confuse selection and focus, which Gwenview currently does conceptually (see also https://bugs.kde.org/show_bug.cgi?id=295411). In general Gwenview should be modified to behave similar to Dolphin:

  • All operations should only happen on the "selection", i.e. what is clearly marked with a blue frame.
  • Moving around via Ctrl+arrow_key should not change or discard the selection like it currently does (without even indicating visually). It should only move the focus, which only a single item can have at any time.
  • There needs to be a visual represention of "focus" distinct from "selection", e.g. an underline.
valeriymalov edited the summary of this revision. (Show Details)Nov 4 2017, 5:18 PM
valeriymalov added a comment.Nov 4 2017, 5:51 PM
In D8196#164237, @rkflx wrote:

Note that for the cases I observed the issue in, Gwenview prints Found no root index. Would it be feasible to just add FolderViewContextManagerItem::findRootIndex to the check?

findRootIndex is private, also as far as I understand it's related to folder view sidebar & it's model, so it will fail for some paths (e.g. it fails for browsable sftp paths for me) that aren't listed in "Places" or whatever is used to populate folder view sidebar.

We rely on KCoreDirLister's completed signal, but I see no error signals, which probably would help here.

I also stumbled upon one more non-duplicate bug your patch solves, please add it to the summary if you can confirm: https://bugs.kde.org/show_bug.cgi?id=306835

Seems like it fixes it too, added

rkflx added a comment.Nov 4 2017, 6:15 PM

That's a pity, but it was a wild guess anyway and as you may have noticed, currently I just lack the time to look too deep into the code. Any questions regarding KCoreDirLister are probably best asked on kde-devel (IRC or mailing list), or tag someone like dfaure on Phabricator.

I'm happy with the current state of the patch, and so far no other reviewer came along. If you don't find any last minute bugs, I'd say you can arc land it now or next week.

As already said, it would be great if you could try to fix some of the open issues in a followup patch, and I'd be happy to review again (just subscribe me to the Diff).

Thanks again!

gateau accepted this revision.Nov 5 2017, 8:06 PM

Small nitpick on a coding style issue, but the code looks good, thanks for your work!

lib/contextmanager.cpp
173

nitpick: opening brace should be on the same line as the if

valeriymalov updated this revision to Diff 22038.Nov 7 2017, 4:44 PM
  • fix code style
valeriymalov marked an inline comment as done.Nov 7 2017, 4:44 PM
ngraham added a comment.Nov 8 2017, 8:06 PM

Great work! Can we land this?

rkflx added a comment.Nov 8 2017, 8:08 PM

I guess Valeriy will do so in time for the Beta ;)

rkflx added a comment.Nov 8 2017, 11:24 PM

One more bug which might be related to the overall topic: https://bugs.kde.org/show_bug.cgi?id=299748. Unfortunately, this bug is not fixed by the Diff, this is just for reference.

(I wish for easier crossreferencing and organizing we could move all Bugzillas to Phab like Wikimedia did…).

ngraham added a comment.Nov 9 2017, 3:51 AM

@valeriymalov, can you land this?

Closed by commit R260:b55420b2ac3d: Try to keep ContextManager in sync with viewed files in MainWindow (authored by valeriymalov). · Explain WhyNov 9 2017, 2:53 PM
This revision was automatically updated to reflect the committed changes.
rkflx added a comment.Nov 28 2017, 10:51 PM

And here we go, first post-review regression discovered: Before, entering a folder containing images and subfolders would select the first image. Now, nothing is selected. Luckily, D8934: Initial focus to thumbnails in browse mode will fix this in time for the RC. The only difference is that now the first item is always selected regardless of type, which in this case is the folder.

I think this is even an improvement, just wanted to mention it in case anyone feels different. I'm committing the other patch for now so it makes the RC, but this does not mean it cannot be changed later on.

rkflx mentioned this in D9342: Make sure, a newly saved image is selected in ContextManager.Dec 15 2017, 8:58 PM

Just for completeness, another regression (there's just too much functionality in Gwenview…).

No selection for:

  • File → Save As
  • Rotate → Save As

More details in D9342#179947.

Revision Contents
Changeset List

Diff 22122

View Options

app/fileopscontextmanageritem.cpp

Loading...
View Options

app/mainwindow.cpp

Loading...
View Options

lib/contextmanager.h

Loading...
View Options

lib/contextmanager.cpp

Loading...