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.

Sign up for GitHub

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

Jump to bottom

Private/gokay/impress grid #10147

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

Private/gokay/impress grid #10147

wants to merge 20 commits into from
+728 −559

Conversation

gokaysatir
Copy link
Contributor

  • Resolves: #
  • Target version: master

Summary

TODO

  • ...

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

@gokaysatir gokaysatir marked this pull request as draft September 30, 2024 12:29
@gokaysatir gokaysatir force-pushed the private/gokay/impress-grid branch 7 times, most recently from a91b92f to 2cef940 Compare October 5, 2024 12:34
@gokaysatir gokaysatir marked this pull request as ready for review October 5, 2024 12:34
@gokaysatir gokaysatir force-pushed the private/gokay/impress-grid branch 3 times, most recently from 7d5fb94 to 02d3f17 Compare October 7, 2024 14:55
eszkadev
eszkadev reviewed Oct 8, 2024
View reviewed changes
browser/src/docstatefunctions.js
@@ -150,3 +152,61 @@ app.isExperimentalMode = function () {
return app.socket.WSDServer.Options.indexOf('E') !== -1;
return false;
};

app.calc.isPartHidden = function (part) {
if (!app.map._docLayer || !app.map._docLayer._lastStatusJSON) return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

we can add helper: app.isLoaded = () => { return app.map._docLayer && app.map._docLayer._lastStatusJSON; } to simplify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't add it as "app.isLoaded" because it's Calc specific. But that's right, we need a better approach and i will plan to refactor this. Thank you.

@eszkadev
Copy link
Contributor

eszkadev commented Oct 8, 2024

I read few first commits. it's a big rework, I think it should go into RED CODE.
Would be also easier if it was posted in parts (eg 'status' rework first as separate PR so it's easier to review :) )

@gokaysatir
Copy link
Contributor Author

I read few first commits. it's a big rework, I think it should go into RED CODE. Would be also easier if it was posted in parts (eg 'status' rework first as separate PR so it's easier to review :) )

Thanks for checking :) You are right, i saw the refactoring needs while working on the subject, it was a bit uncomfortable to split them. Sorry i know it's a pain to review this large PR.

hfiguiere and others added 10 commits October 9, 2024 12:00
@hfiguiere @gokaysatir
draw: Added GridVisible and GridUse commands
1a1bb24 
Also in Impress

Set this._map._docLayer._snapToGrid in the Canvas.

Issue #7406

Signed-off-by: Hubert Figuière <[email protected]>
Change-Id: I582d4c4970ce2d92dd552425b65ee14138ff57df
@gokaysatir
Move show-grid and use-grid commands to view tab.
4cc4c67 
Their compact menu counterpart is already under view menu.
It looks like these commands belong to View.

Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: Ic8d61332c9ed599567388a84978a158fc499bbf6
@gokaysatir
Refactor documentStatus function.
4d4a643 
It gets the document status from core side as JSON string.
Then it deserializes and merges again.
This commit creates a std::unordered_map element and uses it for storing core side data.
Then it stringifies the JSON and sends to the client side.

This allows:
* Removing difficult-to-read parts.
* Adding more info on the core side when needed, without modifying the server side code, in most cases at least.
   Because the core side will only change the content of the JSON and server is not checking it. Server is a bridge.

Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I94652400046a8f9f4aeda36224bac5a0c9b4cc69
@gokaysatir
Add docstate functions for Calc.
76afef1 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I74478b07ee0fa325ff38796521c7d5bd16a9ac0f
@gokaysatir
Use more docstate functions in Calc.
e0f3e29 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I5b931d45af447b23e42732962c3d8f1ce72f1b96
@gokaysatir
Use status JSON instead of text.
9629af6 
Now the server side is sending status command as a JSON string.
Client side parses and uses it.

This commit also removes some unused functions.

Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I1af63c8e84962f46476f1a5c81b71c27526ac9b1
@gokaysatir
Fetch pagerectangles as array.
7357db9 
Or the JSON string is not valid.

Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: Ia9d06591294fc1b2c4d03aa01d6264b554fd5e1f
@gokaysatir
Writer: Use JSON instead of text message.
3926665 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I4b669ca4f35fc6e3a65a6dd46cc4d8373af0d948
@gokaysatir
Use statusJSON for Impress.
c96a8d3 
Most parts of it is sent from the core side readily and read on the client side.
Previous commits prepared this infra.

Use "slide" term for impress related functions.
Move impress related isSlideHidden etc functions into docstatefunctions.js.
Use statusJSON's properties for checking the visibilities and selected states of slides.
Remove unused selectedparts message handler.

Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I078e5c7ce7514984a183e87197243c3f63684c60
@gokaysatir
Read initial state of grid visibility and snapping to grid features.
ce04800 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I1ff4ab93cfffc91e0a161d3417c251e468cefcab
@gokaysatir
Visually show the snapped grid point when grid-snapping is enabled.
cffe785 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I94c8591fda0040e3b6cebfa5ac4a537d4a1def80
@gokaysatir
Various improvements.
86f1a0d 
Avoid JS error in app.calc.isRTL when the part is newly added and non-existing in the list.
Parse hash to int when adding a comment marker is being added.
Fix Cypress issues that was related to latest changes on Hide/Show sheets.
Remove debugging code from KitHelper.hpp.
Modify parseDocSize function in helpers.hpp to use JSON instead of text.

Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I65cc6ef433452da297616f56da8f0d651a23dec6
@gokaysatir
Modify UnitInsertDelete test to use json objects instead of text.
b242574 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I58f29c6d29a884efe659e6ad0454b042a0e4640b
@gokaysatir
Remove unnecessary update calls.
850e4cc 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I2b3d1078435dcb8c2b1c3aef38cf7fd53f30e82f
@gokaysatir
Write mode into document status message before sending.
26b2373 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: Iac43e063bde44e3717e4abe301faa2ed5964aae1
@gokaysatir
Refactor reading status and statusupdate message on ClientSession.
563d627 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I2dc3c0d01f40c76501b1b198fa0b74f83f520f8f
@gokaysatir
Update TileCacheTests for using Json object as status message.
2f21fed 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I2360052a258a1da85e68116913579165a75d47be
@gokaysatir
Modify UnitLoadTorture.cpp for JSON.
fe1ea1d 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: Ibf3588ab4f55d743901342768bfcb3c5b8e1bf3e
@gokaysatir
Impress mobile: Ensure to get correct element on drag start.
9aaa9ac 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: I99553aee5aab6b94e0738355e88bba06475c1242
@gokaysatir
Refactor snap-to-grid feature.
810d230 
Signed-off-by: Gökay Şatır <[email protected]>
Change-Id: Ie175d7e4a36ac3e715dff220ffaa733ed499bc72
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eszkadev eszkadev Awaiting requested review from eszkadev

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
Collabora Online
Status: To Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gokaysatir @eszkadev @hfiguiere