fix(calc): Improve zoom view-jumping #10997
Conversation
Minion3665
force-pushed
the
private/skyler/push-ponoroluvzmz
branch
3 times, most recently
from
0ebd4f9 to
2053685
Compare
| newCenter = newTopLeftP.add(paneBounds.getSize().divideBy(2)).multiplyBy(app.dpiScale); | ||
| } else { | ||
| const newPaneCenter = new L.Point( | ||
| (docTopLeft.x - splitPos.x + (paneSize.x + splitPos.x) * 0.5 / scale), |
There was a problem hiding this comment.
splitPos is used only in Calc, so no sense to have it in this branch probably? (I understand you don't want to break anything and it's just "old behavior" ?)
Also I don't see anything wrt split panes in the Calc branch. Did you test with freezed rows and columns?
|
would be good to add some cypress test too, existing one for inspiration: https://github.com/CollaboraOnline/online/blob/master/cypress_test/integration_tests/desktop/calc/cell_cursor_spec.js#L63 |
Minion3665
force-pushed
the
private/skyler/push-ponoroluvzmz
branch
2 times, most recently
from
3a79fbc to
db7bfc1
Compare
|
While writing tests for this, I noticed that in its current incarnation it loses a lot of precision sometimes, leading to it being worse than the current zoom algorithm at keeping where it's zooming around (the current zoom algorithm will get to ~400 cells out in a single zoom step, but that's the most it'll do and it'll often jump back to stay around the zoom center whereas this one will tend to drift) ... certainly I'm not happy with this OTOH, it never gets that far out in single, regular-sized zoom steps |
|
Anyway, this leads to the fun effect that it's somewhat difficult to write large-zoom tests that are sensible for it, because they will tend to pass on the previous incarnation if they pass on this one ... |
Previously, we got significant view jumps in calc by virtue of an incorrect center position. To fix this, I've melded together the old formula (from before commit d6f375c) with the new formula. I've gated this to calc as the old formula is calc specific. More testing will be needed to determine if there is any jumping in writer/impress. There are still minor jumps from the end of the animation to the final zoom pos, which is particularly noticable in split view, but I think this may actually be *better* than the animated-to position, so instead of further iterating on this, I'd like to look and see if we should change that instead. This code still doesn't affect tablet view. Signed-off-by: Skyler Grey <[email protected]> Change-Id: I7127d8f0a3156ed9dfb04bdd5fb801c318531269
In calc, splitPos is used for frozen cells. As those can't exist in other apps it makes no sense to keep the splitPos terms in the calculations: they will always be zero Signed-off-by: Skyler Grey <[email protected]> Change-Id: I41b7135408402035773141b4af174e6c3f645ded
Minion3665
force-pushed
the
private/skyler/push-ponoroluvzmz
branch
from
db7bfc1 to
7c9879c
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /* global describe it cy beforeEach expect require */ | ||
|
|
||
| var helper = require('../../common/helper'); | ||
| var calcHelper = require('../../common/calc_helper'); |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
|
A new day, a new patch. This one is much cleaner, although it still jumps when going between some zoom levels, particularly at weird dpiScales (at dpiScale=4 120->150 causes a jump, for example)... this leads to drifting again |
Co-Authored-By: Chris Lord <[email protected]> Change-Id: Ia575bebb64af49e8ee2c9b91b171aeb00ebcaa5c DO-NOT-MERGE: This test does not pass, as the zoom feature has some drifting left
Minion3665
force-pushed
the
private/skyler/push-ponoroluvzmz
branch
from
6de181e to
1aec250
Compare
Previously, we got significant view jumps in calc by virtue of an incorrect center position. To fix this, I've melded together the old formula (from before commit d6f375c) with the new formula. I've gated this to calc as the old formula is calc specific. More testing will be needed to determine if there is any jumping in writer/impress.
This old formula had some pitfalls, as it was made to deal with zooming from the top left of the screen rather than an arbitrary point. Particularly notably, the old formula doesn't deal with anything that is not on the edge of a cell. Therefore, there is still some view jumping, there's just likely to be less-of-it and it'll be a bit more consistent rather than having huge jumps every time on higher positions.
This code still doesn't work in some cases, which I will continue to fix:
Change-Id: I7127d8f0a3156ed9dfb04bdd5fb801c318531269
Summary
TODO
Checklist
make prettier-writeand formatted the code.make checkmake runand manually verified that everything looks okay