CollaboraOnline
Shows calc comment indicatior as a triangle.
Comment indicator is changed in core d395b42cdf3ef517d215dee905e5ccf2ae73b2b8 We draw that indicator on online side separately. Now we see the same indicator on online too.
Change-Id: I2c640f4850473724e8a14e75fa7d3c00f76d4e9e
- Resolves: #7130
- Target version: master
Summary
TODO
- [ ] ...
Checklist
- [ ] I have run
make prettier-writeand formatted the code. - [ ] All commits have Change-Id
- [ ] I have run tests with
make check - [ ] I have issued
make runand manually verified that everything looks okay - [ ] Documentation (manuals or wiki) has been updated or is not required
Looks nice, but you need to also take into account zoom level I think:
I inserted comment in view with 100%, then I zoomed in and it is off.
@eszkadev @pedropintosilva Position problem is not related with this pr. I've updated core co-24.04 and online latest master. Without this PR it looks as video: Screencast from 2024-10-24 16-04-29.webm
Maybe we can merge this and fix the regression seperatly?
@GulsahKose good catch! but can we try to fix it too? It should be really easy. You do the calculations of the x, y coordinates. It is only required to correctly multiply by the factor based on zoom/dpi.
Some pointers to find numbers: https://github.com/CollaboraOnline/online/blob/master/browser/src/docstate.js#L20 You can look how it is used in other places. In my opinion this issue is serious and very visible.
so it seems Gulsah already tried that but it didn't work, probably because the cell coordinates are already being multiplied and so, I think we shouldn't need to multiple again (? as it seems the triangle is been drawn according to the cell).
@GulsahKose could you please add here your notes? and also your question around outgoing message clientVisibleArea
https://github.com/user-attachments/assets/0362db1f-d24c-486b-a75b-b0493a0fc7b9
@eszkadev I've tried to multiply with app.dpiScale but didn't fix the problem. Then I realized I'm using cell coordinates that also multiplied with scale. And the triangle is also grown when we zoom.
And found a good case. When we zoom in then scroll the sheet (with mouse wheel) the indicator position is fixed.
When i checked the console I see we sent clientvisiblearea message that contains visible area coordinates, width and height. I think after the zoom, this visible area coordinates is wrong so triangle position is wrong too. When the clientvisiblearea updated with scroll, triangle position is fixed too.
Maybe just a "refresh" is needed? @gokaysatir might know
I am checking the positioning issue now.
Pressed retry Cypress (desktop) as the failing test is unrelated "integration_tests/mobile/writer/annotation_spec.js "
@eszkadev @GulsahKose i think below commit solves the positioning issue:
- https://github.com/CollaboraOnline/online/pull/10328
Pressed the retry button on cypress mobile since writer/annotation_spec.js is unrelated
I think we could merge in red window (since it doesn't fix any particular bug) or we could merge this since it seems safe and better than the current situation, what's your opinion @eszkadev
Furthermore, I would like to see the 3 following enhancements/follow ups:
- The triangle not being drawn on top of the cell border (see the screenshot it is above the right cell border)
- The right edge of the triangle should have the same length as the top edge of the triangle
- The color should come from the palette or dark palette css instead (css var)
There is a typo in the commit message and MR title s/indicatior/indicator
@gokaysatir You only changed the PR title, I am more concerned about the commit message.
There is a typo in the commit message and MR title s/indicatior/indicator
@gokaysatir You only changed the PR title, I am more concerned about the commit message.
Too late to fix now.