Fix snapping zoom to minimum when scrolling

[afontenot]

This code is designed to prevent zooming past the minimum or maximum, but it doesn't work properly in one important case. When zoom out (zoom levels < 1:1) is enabled via a modifier key, and zoom out has occurred (the current zoom is less than 0), and then the modifier key is released, further attempts to zoom out will instead zoom in as the zoom is snapped to the reduced maximum.

This is observable if you scroll by mistake, but the bigger issue is with high resolution touchpads, when generate numerous scroll events and usually have "momentum", meaning they continue generating scroll events after the user's fingers are off the touchpad. This means the user can do everything "right", and release the modifier key after they stop scrolling, but the momentum scroll events will result in an annoying snap-in zoom.

This commit works around the issue without side effects (I think):

• We only snap the zoom level in when the user is zooming out.

• In case the previous zoom level is lower than the current minimum zoom level, we keep the previous zoom level instead of snapping.

Also fixes a typo.

[EtlamGit]

The current behavior is intended to have a fast way to come back to 1:1 zoom.

But after reading your explanation and code several times I might start to understand your use case. I will try to find a device with touch pad to do own experiments. What device do you use?

In case I understand it correctly, your code changes seem to solve what you describe: When zoomed out successfully, that zoom level is preserved for additional zoom out events. And zoom in events will not jump to 1:1 but go step by step until they reach it..

[afontenot]

The current behavior is intended to have a fast way to come back to 1:1 zoom.

Hmm, I do see that that behavior would be impacted slightly. (Though zooming is so fast for me that zooming from -4 to 0 is negligible when I'm trying to do it - e.g. when I'm zooming in.) If you'd like to preserve that behavior, maybe this would work?

// snap the zoom level to bounds when the user scrolls too far
// do not force zoom-in when trying to zoom-out with allowZoomOut=false
if (zoomLevel < zoomMin) {
  if (steps < 0) {
    zoomLevel = std::min(zoomMin, oldZoomIndex);
  } else {
    zoomLevel = zoomMin;
  }
} else if (zoomLevel > zoomMax) {
  zoomLevel = zoomMax;
}

That way we activate the old behavior when attempting to zoom in (immediate snapping), but not when attempting to zoom out (which strictly makes no sense when you're at the maximum allowed zoom).

I will try to find a device with touch pad to do own experiments. What device do you use?

This is a Dell laptop with a Synaptics brand (or possibly rebranded) touchpad. Note that this behavior is software defined, the hardware is not generating bogus touch events. I have the option to turn this off in my system settings (with the xf86-input-synaptics driver):

Normally the coasting behavior is nice, though. Minutor triggers bad behavior in that there's this corner case where scrolling down/in will zoom you out instead, if you release the modifier key too soon.

[EtlamGit]

The comment about intended behavior just means that this was intended. It does not mean we could not change it. I just have to think about it and maybe try the options.

I always use a mouse (with extra buttons bound to the modifier keys) and there I can navigate through all zoom levels very precise. But these 4 zoom levels are also only 1cm movement of a finger, so I might get used to it.

[EtlamGit]

Today I was able to test on my laptop: I do not have such "momentum" settings even if it is some Synaptics touch pad.

What I observed is that the modifier key is ignored completely when left at default setting. It was necessary to set it to e.g. "Alt" to get the intended behavior. So there is some additional bug in modifier key detection at least on my laptop. Maybe I have time during summer break to look into that.

When testing zooming with/without modifier key I also get a straight jump to 1:1 at the moment when touching the touch pad with two fingers. So your changes will probable make it better for touch pad users.

I'm not sure if we have to preserve the old behavior, (I will think about that some more time).

[EtlamGit]

My laptop just burned up, so no further testing . . .

I was testing with your branch and a normal mouse with scroll wheel: your behavior change is fine.

But I observed another bug we still have: When zoomed OUT to maximum possible, the next step will be restricted due to lack of memory. When you then scroll one step in, you get no reaction. Expected would be the second zoom level. You need to scroll two clicks to get one reaction. At maximum zoom IN everything behaves fine.

Maybe you can fix this also with your PullRequest? It is just one instruction more around line 185:

    if ((1.2 * chunks) <= maxchunks)
      restrictZoom = false; // everything matches with a low margin of 20%
    else {
      zoomIndex++;          // restrict zoom
      zoomLevel++;          // revert zoomLevel change
    }

the last line and the curly brackets

[afontenot]

I think I see your point. With a scroll wheel you don't get fractional scrolling increments, so e.g. one scroll will take zoomLevel and zoomIndex up one level. If it turns out that that level would take you past the memory limit, the index is forced back down (meaning that the visible zoom doesn't change). However, since zoomLevel stores the true value of the zoom, it will be stuck at one level higher than zoomIndex. So then scrolling forward will just take you back to where zoomIndex is. Your change keeps the two in sync when snapping for memory reasons. Is that right? If so, makes sense to me.

I'll add a commit with this change.

Edit: actually on second thought, I think it makes more sense to change zoomLevel directly to whatever zoomIndex is. That's because whatever fractional values you have stored in zoomLevel no longer make any sense when zoomIndex has been forced.

So for example... suppose you're at zoomLevel=0.5 and then you scroll to zoomLevel=0.4. Rounding means you get zoomIndex=0. So if memory limits force zoomIndex=1, you don't want to have zoomLevel=1.4, because then you're only a hair way (on a touchpad) from jumping immediately to 2, which feels weird. It's better to snap zoomLevel=1.0 in this case.

I've modified the commit to do that instead.