show snapshots side-by-side

[dterrahe]

Adds a button next to "take snapshot" to change to a side-by-side (or over-under) view of snapshot next to the current image.

This completely depends on both sides being exactly the same dimensions, so don't even think about asking if the dividing line can be made moveable. Clicking anywhere is interpreted as clicking on the rotate button. If you want to move (after zooming in) you need to keep the A key pressed (as you normally would, with an overlayed snapshot) while dragging.

In all likelihood this PR will break stuff. Especially stuff that depends on the "center" view being directly contained within the "center_base" overlay. This PR adds an extra layer in between (a box that holds two views). Lighttable does funky stuff with "center_base" as does Maps. I won't pretend I understand any of it or that I did any testing. At all.

fixes #18270

[TurboGit]

Works good, two issues:

• In side-by-side when you zoom you cannot pan. Try to click an pan the picture change the split orientation and create some strange shadow effect over one image from time to time.
• In side-by-side, when zoomed in and after trying to pan it seems that we have a process recomputing in loop, we have the label "working..." appearing.

[dterrahe]

• Try to click an pan the picture change the split orientation

Did you read the PR? I did not change the pan behavior in overlay; it always required the "force pan/zoom/rotate with mouse" (a) button to be held. And since you cannot move the divider, any click is interpreted as rotate instead.

It makes sense to revisit the decision to make the canvas not sensitive when in snapshot mode. When side-by-side, we could just have masks drawing etc functional in the "current" side. Even in the overlay snapshot mode, we could have it functional on the "uncovered" side and only move the divider line when clicked on/near it and dragged. This will take a bit longer to get right carefully. In the mean time I don't want to waste too much time making the intermediate solution perfect.

create some strange shadow effect over one image from time to time

Is this a regression or the same thing that happens in normal snapshot mode? For the "current" side we fill in the not-yet computed zoomed+panned area from the "preview" (full image top left). For the snapshot we don't calculate a separate preview image, so we use the same preview as the normal image, which might look very different. If you are like me you test snapshots by creating one and then changing exposure, so filling in the snapshot from the darker preview image creates a "shadow". Or are talking about something else?

• we have a process recomputing in loop

Yes, I see that sometimes. I can't think of a reason why this PR would cause that (except that the viewport tends to be narrow). I've checked that it is not caused by one side of the window being one pixel wider than the other. So maybe this is another case of this kind of behavior that I've seen reported previously (can't find it now) and that might be rounding errors of position calculation.

[TurboGit]

Did you read the PR?

It seems no, and more importantly this is a common dt feature (pressing A) so I should have remembered. Let's put that on the tiredness, I'm doing too many things.

[TurboGit]

It makes sense to revisit the decision to make the canvas not sensitive when in snapshot mode.

No I don't think we need to revisit that.

So after a second testing, it works ok on my side.

The issue I was talking about is something else, the snapshot area when returning to the "overlay" mode do not get redrawn and so may be wrong... The fix is simple, we need to invalidate the snapshot, my proposal:

diff --git a/src/libs/snapshots.c b/src/libs/snapshots.c
index 7f59c17304..79c24c4a41 100644
--- a/src/libs/snapshots.c
+++ b/src/libs/snapshots.c
@@ -734,7 +734,9 @@ static void _sidebyside_button_clicked(GtkWidget *widget, dt_lib_module_t *self)
   dt_lib_snapshots_t *d = self->data;
 
   d->sidebyside = !d->sidebyside;
-  gtk_widget_set_visible(dt_ui_snapshot(darktable.gui->ui), d->sidebyside && d->selected >= 0);
+  d->snap_requested = TRUE;
+  gtk_widget_set_visible(dt_ui_snapshot(darktable.gui->ui),
+                         d->sidebyside && d->selected >= 0);
 }

[dterrahe]

I improved the way a redraw of the snapshot "window" gets triggered. Sometimes it wouldn't when it should. Would you please see if that solves the issue as well? There should be no need to always force a recalc separately; the resize should do that if needed.

Also removed a level of double buffering that's been there forever and that nobody has ever looked at since (presumably). Since gtk2.0 double buffering is already done in the toolkit so we should not do it again. I think when I rejigged the whole zoom/pan thing I also removed a double buffer, so originally we quadruple buffered! One of those "for good measure" and "let's assume (based on nothing) that they knew what they were doing and not fix anything".

But of course if there was a good reason for additional triple buffering, I'll drop the second commit.

Since now gtk receives back the same cr it handed us, it notices if we messed up the transformation matrix, which apparently happened when no image had finished loading yet. Some extra checks solved the resulting error messages.

[TurboGit]

Still an issue, take a picture, just added an history to have the image B&W to better see the issue.

• Snapshot on Sigmoid
• Last history step on color balance RGB (B&W)

• Enable snapshot by clicking on it
• Enable side-by-side view
• Zoom-in
• Pan the image
• Disable side-by-side view

See the right part of the image (the snapshot) gets part color (which is good) and part in B&W (which is wrong):

[TurboGit]

And using my patch above I cannot reproduce.

[dterrahe]

I see what you are saying, but the "part b&w part color" snapshot is only temporary, while the snapshot is being recalculated. Both before and after your patch it corrects (to all color) after a few moments (for me). So snap_requested seems to get triggered in either case, which is what I would expect. If that doesn't happen for you (i.e. being more patient doesn't solve the issue, but adding the line and no additional patience does) then it would be better to understand why then just papering over it, but as long as I cannot reproduce it is hard to further investigate. So let's add the line if you need it.

The only drawback I see is that IF someone in future makes the snap_requested trigger smarter (for example by checking if the currently available area is already at least as large as what we need, rather than exactly what we need, which happens when zooming from 100% to 200%. That would avoid an unnecessary recalc and make panning around the 4x extra available area much quicker; a change that was made for the normal view but not snapshots) then having that improvement cancelled when switching to/from sbs would be a pity and someone might see this extra line that does it and be afraid to remove it.

BTW the other issue, flickering "working..." which I sometimes get when rotating while zoomed does cause pieces of the snapshot to not get updated. So I've now added a snap_requested there too.

Any comments on the double buffering removal?

[jenshannoschwalm]

Also removed a level of double buffering that's been there forever and that nobody has ever looked at since (presumably). Since gtk2.0 double buffering is already done in the toolkit so we should not do it again. I think when I rejigged the whole zoom/pan thing I also removed a double buffer, so originally we quadruple buffered! One of those "for good measure" and "let's assume (based on nothing) that they knew what they were doing and not fix anything".

But of course if there was a good reason for additional triple buffering, I'll drop the second commit.

I think this is all good (and removes code that could be duplicated :-)

So maybe this is another case of this kind of behavior that I've seen reported previously (can't find it now) and that might be rounding errors of position calculation.

Or could some code part fight against main canvas size?

[dterrahe]

removes code that could be duplicated

Not just that; it allocated a chunk of memory and copies needlessly. So should be (unnoticeably) faster now.

Or could some code part fight against main canvas size?

Maybe. I'd have to dig in and understand the snapshot generation code again. Anyway, seems to be solved by my last commit...

[TurboGit]

Any comments on the double buffering removal?

No, seems to be ok on my testing. Hopefully this is ok also on macOS and Windows.

[TurboGit]

Working fine for me, thanks a lot!

Need a release note entry for the "big one" section.

[dterrahe]

Release note:

• view snapshots side-by-side with the current image, instead of as a partial overlay, by clicking the button next to "take snapshot". The center viewport is divided in two equal panels and can be zoomed and panned (by dragging while holding the "a" key) as before. The dividing line can not be moved and any click without "a" rotates the layout.