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

Convert the Convert Request/Response types to Proto, refactor Layout conversion code #1965

Merged

Conversation

timothyfroehlich
Copy link
Member

@timothyfroehlich timothyfroehlich commented Jan 9, 2025

Based on PR #1886

Chain of upstream PRs as of 2025-01-09

With this change we're able to fully remove serde-reflection from the code base.

This involved a decent amount of refactoring and cleanup of old work to get things right. My goal with the refactoring was to keep the packages cleanly separated and to work towards having the ability to build a DesignCompose app without including anything related to Live Update. Ultimately I could see the dc_jni crate being split into the layout and fetch portions, which would compile into their own separate libraries, and merged in with figma_import and layout.

Some of the messages related to both the doc fetching and layout processing were a bit scattered and were declared in crates that didn't actually use them. Before this change the dc_jni crate didn't have any of its own proto messages, it was pulling the ones it needed (Layout conversion related) from other crates (both dc_bundle and layout). The new Convert Request/Response messages would only be used by dc_jni, so it made sense to add a build.rs file to it so that the generated messages would be native to that crate, rather than being part of an unrelated crate.

While making that change it made sense to rearrange the proto messages related to the android interface, so I moved the layout related messages into a layout_interface proto package and put the new Convert messages into the live_update package.

In addition, the layout code had really only been half converted to proto, we were still parsing them and then converting them into the original Rust structs. That's all cleaned up now.

We had been manually generating a json string which was decoded into the ConvertRequest on the rust side, so I converted the ConvertRequest to Proto and removed the json code. It didn't make sense to use proto on one side and not the other. It'd probably be cleaner to change the jniFetchDoc method to just send a single proto message instead of the ConvertRequest, doc_id, version_id and proxy configuration, but that can be cleaned up another day.

I also decided not to convert the image_session from json to proto because we don't actually do anything with it on the Kotlin side other than writing it into the .dcf. I'm simply including the json in the proto message and removing the redundant storage of the session's bytes in the docContent class.

Admittedly this PR is bigger than it strictly needed to be. I can split it into some smaller PRs if requested. I'm sure there's some additional cleanup that could be done as well, including simplifying the jniFetchDoc function and turning ProxyConfig into a proto struct.

@timothyfroehlich timothyfroehlich self-assigned this Jan 9, 2025
@timothyfroehlich timothyfroehlich changed the title Convert the Rust side of Convert Request/Response to Proto Convert the Convert Request/Response types to Proto Jan 9, 2025
Copy link

github-actions bot commented Jan 9, 2025

Snapshot diff report vs base branch: feature/protoconv
Last updated: Mon Jan 13 09:43:07 PST 2025, Sha: cd4102a
No differences detected

@timothyfroehlich timothyfroehlich force-pushed the wb/froeht/protolize-convertresponse branch from df68f7c to ac219a2 Compare January 9, 2025 19:47
@timothyfroehlich timothyfroehlich changed the title Convert the Convert Request/Response types to Proto Convert the Convert Request/Response types to Proto, refactor Layout conversion code Jan 9, 2025
@timothyfroehlich timothyfroehlich force-pushed the wb/froeht/protolize-convertresponse branch 2 times, most recently from dfa3d89 to b3a2559 Compare January 9, 2025 21:13
@timothyfroehlich timothyfroehlich marked this pull request as ready for review January 9, 2025 21:19
crates/figma_import/src/proxy_config.rs Outdated Show resolved Hide resolved
crates/layout/src/layout_manager.rs Outdated Show resolved Hide resolved
@timothyfroehlich timothyfroehlich force-pushed the wb/froeht/protolize-convertresponse branch 3 times, most recently from 7cbaad8 to 1d680f6 Compare January 10, 2025 18:51
@timothyfroehlich timothyfroehlich enabled auto-merge (squash) January 13, 2025 17:25
@timothyfroehlich timothyfroehlich force-pushed the wb/froeht/protolize-convertresponse branch from 1d680f6 to cd4102a Compare January 13, 2025 17:25
@timothyfroehlich timothyfroehlich merged commit 5a70cd0 into feature/protoconv Jan 13, 2025
21 checks passed
@timothyfroehlich timothyfroehlich deleted the wb/froeht/protolize-convertresponse branch January 13, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants