@huoni Please review (including pressing the "Accept" button if both behaviour and code cannot be improved anymore ;) Let me know if you need tips on that.

In D12489#252864, @rkflx wrote:

@huoni Please review (including pressing the "Accept" button if both behaviour and code cannot be improved anymore ;) Let me know if you need tips on that.

Okay, I admit it: In the meantime I peeked at the patch a bit, and there might be a problem…

In D12489#253315, @rkflx wrote:

In D12489#252864, @rkflx wrote:

@huoni Please review (including pressing the "Accept" button if both behaviour and code cannot be improved anymore ;) Let me know if you need tips on that.

Okay, I admit it: In the meantime I peeked at the patch a bit, and there might be a problem…

I haven't had time to look at this properly yet (and probably won't for another 24 hours), but looking at the code on my phone. Is the problem that loading the config always shows the sidebar, even if it was hidden?

In D12489#253322, @huoni wrote:

In D12489#253315, @rkflx wrote:

In D12489#252864, @rkflx wrote:

@huoni Please review (including pressing the "Accept" button if both behaviour and code cannot be improved anymore ;) Let me know if you need tips on that.

Okay, I admit it: In the meantime I peeked at the patch a bit, and there might be a problem…

I haven't had time to look at this properly yet (and probably won't for another 24 hours), but looking at the code on my phone. Is the problem that loading the config always shows the sidebar, even if it was hidden?

If I hide the sidebar, it will stay hidden, so that's not the problem. Just wait until you have a chance to try the patch, but maybe you can also see it when looking critically at every line of the patch (i.e. typically you'd have to look for things that are either wrong, superfluous, or missing; and in particular look at edge cases).

Sorry for the cliffhanger, but we need more reviewers in Gwenview, so I just hope you are interested in learning how to review a bit ;)

In D12489#253332, @rkflx wrote:

In D12489#253322, @huoni wrote:

In D12489#253315, @rkflx wrote:

In D12489#252864, @rkflx wrote:

@huoni Please review (including pressing the "Accept" button if both behaviour and code cannot be improved anymore ;) Let me know if you need tips on that.

Okay, I admit it: In the meantime I peeked at the patch a bit, and there might be a problem…

I haven't had time to look at this properly yet (and probably won't for another 24 hours), but looking at the code on my phone. Is the problem that loading the config always shows the sidebar, even if it was hidden?

If I hide the sidebar, it will stay hidden, so that's not the problem. Just wait until you have a chance to try the patch, but maybe you can also see it when looking critically at every line of the patch (i.e. typically you'd have to look for things that are either wrong, superfluous, or missing; and in particular look at edge cases).

Sorry for the cliffhanger, but we need more reviewers in Gwenview, so I just hope you are interested in learning how to review a bit ;)

Definitely interested and appreciate the guidance/mentoring.
I'll hold off making any more guesses until I can take a look on an actual computer :)

Well, I've had a play around, and found the following problems:

1. If you switch to the Start Page before quitting, the sizes aren't saved.
2. There's a pre-existing issue where collapsing the sidebar by dragging the splitter to the left doesn't toggle the sidebar off, and further toggling shows and hides a narrow sliver of the splitter. This patch makes this issue worse by saving the collapsed splitter state to config. I think the best solution to this would be to detect the collapse and 1) toggle the sidebar off, and 2) make sure the collapse splitter sizes aren't saved.

In D12489#253984, @huoni wrote:

Well, I've had a play around, and found the following problems:

1. If you switch to the Start Page before quitting, the sizes aren't saved.

Ahh, always this special page. ;)

2. There's a pre-existing issue where collapsing the sidebar by dragging the splitter to the left doesn't toggle the sidebar off, and further toggling shows and hides a narrow sliver of the splitter. This patch makes this issue worse by saving the collapsed splitter state to config. I think the best solution to this would be to detect the collapse and 1) toggle the sidebar off, and 2) make sure the collapse splitter sizes aren't saved.

I considered this while working on D10687 but I don't think we should hide the sidebar if a user collapses the left side because this would hide the splitter handle making it impossible to drag back. Wouldn't that a bit vexing to the user?
To not save the collapsed state is a good idea IMO.

In D12489#254103, @muhlenpfordt wrote:

2. There's a pre-existing issue where collapsing the sidebar by dragging the splitter to the left doesn't toggle the sidebar off, and further toggling shows and hides a narrow sliver of the splitter. This patch makes this issue worse by saving the collapsed splitter state to config. I think the best solution to this would be to detect the collapse and 1) toggle the sidebar off, and 2) make sure the collapse splitter sizes aren't saved.

I considered this while working on D10687 but I don't think we should hide the sidebar if a user collapses the left side because this would hide the splitter handle making it impossible to drag back. Wouldn't that a bit vexing to the user?
To not save the collapsed state is a good idea IMO.

I'm not the reviewer for this one, but nevertheless allow me to add my perspective:

• Huon has a point in that some users might accidentally move too far to the left and can never recover, as the small handle is hard to see.
• Allowing to drag to the left, hiding the splitter automatically and then not allowing to undo (perhaps it was just an accidental drag) by dragging right is bad too, IMO Peter is right on that.
• However, I don't agree with the proposed solution, because I think it is a bit inconsistent: When we allow (almost) hiding the sidebar by dragging to the far left, we should also remember that width. If we don't save the zero-width, we should also disallow setting such a width, i.e. set childrenCollapsible to false (IIRC). Everything else will just confuse users.

As we already provide the separate View → Sidebar option, the built-in functionality from QSplitter is not really needed IMO, so I'd tend to go with simply disallowing a zero width.

Yay/Nay?

In D12489#254156, @rkflx wrote:

As we already provide the separate View → Sidebar option, the built-in functionality from QSplitter is not really needed IMO, so I'd tend to go with simply disallowing a zero width.

I like it this way (setting all child widgets to non collapsible). It doesn't really make sense to hide the view or thumbnail area by collapsing and the side bar has it's buttons/shortcuts.
Btw. dolphin prevents collapsing too. ;)
Maybe we should adapt this (or whatever comes out) to the thumbnail bar afterwards.

In D12489#254182, @muhlenpfordt wrote:

Maybe we should adapt this (or whatever comes out) to the thumbnail bar afterwards.

If you also mean with that the "Gwenview forgets the thumbnailbar height upon switching to the start page" part, I'm all for it (might be worth checking what else is not remembered in that situation…).

Also I noticed that dragging to the far right would be prevented too, which never made much sense to me, as the image was hidden completely.

Hadn't considered disabling the collapse behaviour entirely, and agree this is the best decision. The functionality doesn't make sense in this situation.

In that case, we can revert "Prevent save if one splitter side is collapsed", and put the above in a dependent patch?

In D12489#254235, @huoni wrote:

In that case, we can revert "Prevent save if one splitter side is collapsed", and put the above in a dependent patch?

Sounds good to me. That's how I do it.

Weirdly I'm now getting the cherry pick error when using arc patch. Something about dependent patches seems to confuse Arcanist.

$ arc patch D12489
INFO  Base commit is not in local repository; trying to fetch.
Branch name arcpatch-D12489 already exists; trying a new name.
Branch name arcpatch-D12489_1 already exists; trying a new name.
Created and checked out branch arcpatch-D12489_2.
Branch name arcpatch-D12561 already exists; trying a new name.
Created and checked out branch arcpatch-D12561_1.
Checking patch app/mainwindow.cpp...
Applied patch app/mainwindow.cpp cleanly.

Cherry Pick Failed!
Exception 
Command failed with error #1!
COMMAND
git cherry-pick 'arcpatch-D12561_1'

STDOUT
On branch arcpatch-D12489_2
You are currently cherry-picking commit 64ea7ce0.

nothing to commit, working tree clean


STDERR
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git reset'

(Run with `--trace` for a full exception trace.)

Anyway, patch looks good now.
Thanks for being a practice reviewee :)

In D12489#254971, @huoni wrote:

Weirdly I'm now getting the cherry pick error when using arc patch. Something about dependent patches seems to confuse Arcanist.

From which branch did you issue arc patch from? For patches with dependencies, you'll probably have to git checkout master first, see my analysis in T8506. I guess you reviewed D12561 first and were still on this branch?

In D12489#254981, @rkflx wrote:

In D12489#254971, @huoni wrote:

Weirdly I'm now getting the cherry pick error when using arc patch. Something about dependent patches seems to confuse Arcanist.

From which branch did you issue arc patch from? For patches with dependencies, you'll probably have to git checkout master first, see my analysis in T8506. I guess you reviewed D12561 first and were still on this branch?

Ah, yes that was it. Interesting discussion.

In D12489#254971, @huoni wrote:

Anyway, patch looks good now.
Thanks for being a practice reviewee :)

Anytime! 👍

In D12489#254981, @rkflx wrote:

From which branch did you issue arc patch from? For patches with dependencies, you'll probably have to git checkout master first, see my analysis in T8506. I guess you reviewed D12561 first and were still on this branch?

I tried a couple of constellations in my normal git copy which includes both source branches - I don't get any errors using arc patch for this patch, no matter what branch I checked out before. Only if I clone a new copy I can reproduce the error.

In D12489#254981, @rkflx wrote:

From which branch did you issue arc patch from? For patches with dependencies, you'll probably have to git checkout master first, see my analysis in T8506. I guess you reviewed D12561 first and were still on this branch?

I tried a couple of constellations in my normal git copy which includes both source branches - I don't get any errors using arc patch for this patch, no matter what branch I checked out before. Only if I clone a new copy I can reproduce the error.

This further supports my theory on how this works: For dependent revisions, baseRevision references a commit which is only available on your local branch (as the parent Diff has not landed yet, and thus has no commit sha in the remote repo yet). Your "normal" git copy has that local commit sha, so you get no error. However, a new clone does not have that commit sha, so you get the errror. Anyway, using git checkout master should be good enough of a workaround for now.