Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tile: avoid rendering beyond document boundary #10983

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lpranam
Copy link
Member

@lpranam lpranam commented Jan 22, 2025

problem:
i.e: if calc sheet has most of row hidden and remaining rows don't cover the canvas area then

  1. canvas center was incorrectly calculated based on available area instead the required doc area
  2. grids were drawn beyond the last visible raw

Change-Id: I1f43e37e104c693b8f070271dac620385c0a2dd0

  • Target version: master

Checklist

  • I have run make prettier-write and formatted the code.
  • All commits have Change-Id
  • I have run tests with make check
  • I have issued make run and manually verified that everything looks okay
  • Documentation (manuals or wiki) has been updated or is not required

@lpranam
Copy link
Member Author

lpranam commented Jan 22, 2025

before:
Screenshot_20250122_054530

after:
Screenshot_20250122_054659

Copy link
Contributor

@gokaysatir gokaysatir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really good approach to the issue.

I have 2 thoughts on this:

  • Can you also shrink the size of the canvas when there aren't enough rows or columns to show? Map and canvas elements should have the same size. We are planning the remove map element but until then, we'll need to keep them aligned.
    Also, we need to check shape operations, text selections and the like, after shrinking the elements' sizes. Because the UI may not comply with the changing sizes.

  • Indeed, maybe we can even play with the (id="document-container") element's size. Since map and canvas will probably resize accordingly, everything may "just work". We can do so by adding margins to the "document-container" element.

These are initial thoughts and of course need trying. But changing the size of the elements looks to me like a good approach.

Then i think we can get this in, in the red code.

Thank you.

@gokaysatir
Copy link
Contributor

By the way, this resizing method may be tricky when user zooms in/out the document. If this method proves to be not useful, we may need to work on SheetGeometry handling. My first thought was making the data handling better. That would require to check ColumnHeader.ts, RowHeader.ts and other related files. That way, we may not need to modify the map's and canvas's sizes.

@eszkadev
Copy link
Contributor

could you add cypress test too?
if only way is visual comparison: we have that, examples: cypress_test/integration_tests/desktop/writer/jsdialog_widgets_spec.js

@lpranam
Copy link
Member Author

lpranam commented Jan 23, 2025

  1. Map and canvas usually have different sizes in calc.
  2. Tried "document-container" resizing but it did not work
  3. Added a new commit to take care of zooming and syncing tiles in that case.

I'll added cypress too

browser/src/map/Map.js Fixed Show fixed Hide fixed
browser/src/map/Map.js Fixed Show fixed Hide fixed
@gokaysatir
Copy link
Contributor

Yes i remember now, canvas has column and row headers so that makes a small difference. But we still need to align the canvas's size. Map will be removed. It means this issue will regress when we remove it. I think we need to set also canvas's size. We are mirroring events from map to canvas. Dividing them would look like a workaround rather than a solution.

I also tested previous version and if you move mouse cursor where map doesn't exist but canvas does, row headers rise JS errors. This JS errors is not the main reason for this comment, main reason is canvas and map should stay coupled.

Thanks for checking the zoom issue :) I will check & test this again.

problem:
i.e: if calc sheet has most of row hidden and remaining
rows don't cover the canvas area then
1. canvas center was incorrectly calculated based on
available area instead the required doc area
2. grids were drawn beyond the last visible raw

Signed-off-by: Pranam Lashkari <[email protected]>
Change-Id: I1f43e37e104c693b8f070271dac620385c0a2dd0
problem:
when document size was small as compared to canvas
and we rendered only small area which had document
we need to update container sizes when user changes zoom
or else zoomed document will be displayed in same
small area even when there is enough space to display

Signed-off-by: Pranam Lashkari <[email protected]>
Change-Id: I8cae51f93f5ac591514fea713907a08c243728c2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To Review
Development

Successfully merging this pull request may close these issues.

3 participants