Thanks for the patch, this gets rid of a huge annoyance (in my case mostly when reviewing Gwenview patches :) Change works as expected, and the place you added the function call to is probably fine.

I changed your summary a bit, because CCBUG in the other Diff was more of an exception, it was just to notify an already fixed bug. To actually close a bug you'd use BUG, this is detailed here. (The process for new contributors to discover these workflows is not optimal, I know…)

Do you think we should select the first item when entering a folder or when starting Gwenview passing a folder? 256418c7371f did this for the Recent Folders, and in Dolphin the first item also gets focused instead of having to use → every time. (Sadly Gwenview currently combines focus and selection in an unfortunate way, which together with other problems makes me wonder whether a port to Dolphin's folder/thumbnail view should be considered.)

I also filed a wish, but this is probably out-of-scope for this patch: https://bugs.kde.org/show_bug.cgi?id=387229

In D8934#170989, @rkflx wrote:

Do you think we should select the first item when entering a folder or when starting Gwenview passing a folder?

Yes, that seems like a good idea. ;)
I've solved it by selecting the first thumbnail anytime after a dir listing completed and nothing else is selected.

I also filed a wish, but this is probably out-of-scope for this patch: https://bugs.kde.org/show_bug.cgi?id=387229

Personally I would prefer this, too - but maybe this should be configurable. I don't know if everyone will be happy with this behavior. Perhaps selecting a folder with space and selection+focussing with return is a solution?
Anyway, I think we should not mix that with this patch.

In D8934#171534, @muhlenpfordt wrote:

I've solved it by selecting the first thumbnail anytime after a dir listing completed and nothing else is selected.

Thanks for the update. Works for me in general, but I could break it with Add Filter → Filter by Rating, where index.isValid() == false (not sure why, maybe wrong source model?) and nothing gets selected. Interestingly, Filter by Name works fine. Could you have a look?

If it is too complicated for now, we can also commit as is and report this as a bug, but first we should at least try not to introduce incomplete or buggy behaviour.

I also filed a wish, but this is probably out-of-scope for this patch: https://bugs.kde.org/show_bug.cgi?id=387229

Let's continue discussing this in the bug report (I added my thoughts there).

In D8934#172016, @rkflx wrote:

Works for me in general, but I could break it with Add Filter → Filter by Rating, where index.isValid() == false (not sure why, maybe wrong source model?) and nothing gets selected. Interestingly, Filter by Name works fine. Could you have a look?

The problem seems to be the delayed retrieveSemanticInfoForIndex in SortedDirModel::filterAcceptsRow(). (This is needed for rating, but not for name filters.) After dir listing is complete, there are no items in model. Only after retrieving the semantic info, the list is populated.
I can't see a simple way to get a signal after this second task ended. Any ideas?

In D8934#173157, @rkflx wrote:

For now, I'm accepting the patch. The problem is also present in 17.08.3, so not really a problem with your patch. Nevertheless, please open a new bug and post the link here for reference.

The bugreport can be found here: https://bugs.kde.org/show_bug.cgi?id=387573

There's another bug caused by this patch - after saving an image under another name, the view should not jump to the first item.
I have a fix, using a flag, which is set when current dir changes.
Should I create a completely new diff for this or is there something special I should respect?

In D8934#179416, @muhlenpfordt wrote:

There's another bug caused by this patch - after saving an image under another name, the view should not jump to the first item.

Not sure what you mean by "saving under another name", just referring to Button or Menu entry functionality would make reproducing easier ;)

I have a fix, using a flag, which is set when current dir changes.

Yay, bug reports with patches included are the best ;) Can't really comment on it without seeing it, though.

Should I create a completely new diff for this or is there something special I should respect?

Best practice is to not touch the code of a revision which already landed in the git repo, otherwise it gets confusing what was reviewed when and what was actually committed. Just open a new Diff for new code please.

That said, it should be perfectly fine to add a comment on a review to notify the author of some problems. If it is a larger thing, you could open a bug report and post the link here, or only refer to a new Diff. Note that Phab makes this very easy: Just open a new Diff and mention the old (e.g. "This fixes problem X introduced by Dyyy"), then the old Diff will get a "mention" referring to the new one added at the bottom (no notification though, in this case add a comment manually).

In D8934#179681, @rkflx wrote:

In D8934#179416, @muhlenpfordt wrote:

There's another bug caused by this patch - after saving an image under another name, the view should not jump to the first item.

Not sure what you mean by "saving under another name", just referring to Button or Menu entry functionality would make reproducing easier ;)

Ah, got it. Reading D9293#179042 you probably mean something like Rotate → Save As. (Click on "View Remarkup" behind the arrow on the comment to see how I wrote this.)