CollaboraOnline / online

Collabora Online is a collaborative online office suite based on LibreOffice technology. This is also the source for the Collabora Office apps for iOS and Android.
https://collaboraonline.com
Other
1.85k stars 702 forks source link

Calc - multiple invalidations seen on single change with sessions on different zoom levels #8792

Open caolanm opened 6 months ago

caolanm commented 6 months ago

Describe the Bug

"extra" invalidations seen when changes takes place at how row/col positions in calc with multiple users

Steps to Reproduce

  1. open hello-world.ods in two browser tabs
  2. change zoom to 85% in browser tab A (one zoom in click)
  3. change zoom to 120% in browser tab B (one zoom out click)
  4. Go to J200 in both browser tabs, center that cell in browser area
  5. ctrl+shift+alt+d and enable tile invalidations in both browsers
  6. type "A" in browser tab A

Actual Behavior

In browser tab A there are two invalidation rectangles, one at the expected location, and another unexpected one higher up In browser Tab B, again two invalidations, the unexpected one lower than the expected one

Expected Behavior

One invalidation seen

Screenshots

invalidation.webm

The problem as I see it is that calc, unlike writer, isn't wysiwyg/stable between different zoom levels. e.g. a 200% zoomed sheet has less than 1/4 the number of rows as a 50% zoomed sheet. And the same cells/area invalidated by one view, even when mapped to twips, is different than the same cells/area in another view.

Right now every view broadcasts separate, and different at higher row/col positions, invalidations for the same change. And for cell editing care is taken by the active view to invalidate the other views with their understanding of the area affected. Each browser view at a given zoom level gets both invalidation's, etc.

These zoom-variable "screen twips" have a stable "print twips" counterpart and can be mapped by core and online to the other.

One possibility is to always invalidate in "print twips" instead of "screen twips" and get one set of invalidations. The browser-side can map those to "screen twips" for that side of things, but to generate deltas etc online would have to do something like intercept SheetGeometryData to know the same geometry as the browser-side does in order to map those print twips to screen twips to see if they intersect the client area, etc.

Another I guess is to propagate the zoom/view-id in invalidations, treat it like a "part" and invalidate only the clients with a matching zoom/shared-canonical id and different zoom levels are considered as different as different dark/light modes are.

caolanm commented 6 months ago

@mcecchetti @mmeeks let me know if my understanding is totally out of whack here on how calc+zoom+online works

mmeeks commented 6 months ago

Hmm; so - my hope is that the tile-cache we invalidate has in its key the tile width & height - which - is ultimately an expensive way of storing the zoom level ;-) as well as the normalized-view-id. So we should be able to invalidate just one view or another.

My expectation is that when we invalidate, we calculate the rectangle per canonical-view and then send to each real view co-ordinates in its own view co-ordinate system - so we get this right.

Of course - inevitably we're probably not doing that right; as this shows - but I believe we have the pieces in-hand to do that.

mmeeks commented 6 months ago

I had a really nice ODT write-up of the co-ordinate systems and their complexities including pictures ;-), perhaps I gave it to @mcecchetti or @dennisfrancis - but I havn't seen it recently and can't find it locally which is annoying.

mcecchetti commented 6 months ago

@caolanm The double invalidation is not limited to cell editing, it occurs even if you try to insert a shape. Every invalidation is forwarded to all views, the rectangle is resized according to the view zoom. I'm not sure where, in the code, this event multiplexing occurs. Anyway, in Calc this strategy doesn't work for well known reason, that you explained. Here the need to send correct invalidation as it is done in ScGridWindow::InvalidateByForeignEditView (see https://gerrit.libreoffice.org/c/core/+/160303). The problem is that if you have N different zoom levels each view gets N invalidation rectangles, which is weird. The second solution you propose looks more simple than the first.