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

Binary File Parsing in read_file_from_workspace #4977

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

Conversation

toastx
Copy link

@toastx toastx commented Sep 25, 2024

☕️ Reasoning

Remove the bail and added a base64 engine to parse binary into a base64 string, which can be displayed in frontend.

🧢 Changes

The function no longer bail at binary, and now returns a b64 string of the image

Fixes: #4957

Copy link

vercel bot commented Sep 25, 2024

@toastx is attempting to deploy a commit to the GitButler Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added the rust Pull requests that update Rust code label Sep 25, 2024
Copy link
Collaborator

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for contributing!

With this function I think we have now reached the point where it needs to change more fundamentally. What's proposed here returns a String by default which assumes an encoding, something that can fail in many ways.

Then there is another branch that encodes the content as base64 and returns it in place of the actual content. Now the frontend wouldn't know anymore what to do with it.

To me, it would be ideal if everything is treated as binary and transmitted to the frontend as bytes. There it could decide what to do.
Alternatively, or if this is too cumbersome, encode everything as base64 and handle it in the frontend.

Something that might be better is to return a structure, though, that informs about the assumed kind of data, maybe with a little help of the infer crate. That could then drive the frontend to indicate what to display.

@toastx
Copy link
Author

toastx commented Sep 25, 2024

Following your method, I have a suggestion:
Instead of string, we can output a tuple in the format of ("content", type), and let the front-end handle it respectively.

So it would output as (content, text), or (base64, binary).

Thoughts/Suggestions?

@Zamoca42
Copy link
Contributor

In my opinion, how about adjusting the format in Rust so that it can be received as a Uint8Array in JavaScript?
I think using Vec<u8> type would make it easier to convert to Uint8Array on the frontend.

@toastx
Copy link
Author

toastx commented Sep 25, 2024

I can try that as well, but I feel using base64/uint8array won't make a difference tbh... And converting the b64 into its original content would be easier no?

One more thing is that we can directly convert the binary into a b64 using a one-liner

@Zamoca42
Copy link
Contributor

You’re right as well. My perspective is that, considering the use of Tauri’s invoke, I understand that GitButler is not communicating through standard web protocols but rather using Tauri’s own IPC (Inter-Process Communication) mechanism.
So, I believe aligning the format with Uint8Array has several advantages.

@toastx
Copy link
Author

toastx commented Sep 25, 2024

You’re right as well. My perspective is that, considering the use of Tauri’s invoke, I understand that GitButler is not communicating through standard web protocols but rather using Tauri’s own IPC (Inter-Process Communication) mechanism. So, I believe aligning the format with Uint8Array has several advantages.

can u link me the ipc stuff, ill give it a read for understanding

@Byron
Copy link
Collaborator

Byron commented Sep 25, 2024

It's worth noting that tauri IPC relies on JSON serialisation, and Vec<u8> serializes to an array of numbers, which is probably less efficient than base64 on the wire. However, I don't know if it matters and we'd probably want to choose what's faster and has less latency.

Regarding the return type, I'd prefer returning a struct so there are field-names - they probably bode better in the frontend and make for more readable code.

@Zamoca42
Copy link
Contributor

https://v2.tauri.app/concept/inter-process-communication/#_top

I think this explains the IPC well.

@toastx
Copy link
Author

toastx commented Sep 26, 2024

After reading about the IPC, and the output structure serialization stuff,

Serialization of Uint8array would be faster imo, as it is in the raw binary form when it is in Vec<u8>, so it would be more performance oriented. Base64 will give us better transportability and visibility , but also encoding/decoding overhead, and more storage space.

Like @Zamoca42 said, handling it in uint8array will be more performance oriented.

@Byron
Copy link
Collaborator

Byron commented Sep 26, 2024

How is a uint8array represented in JSON - the message format of tauri IPC?

@toastx
Copy link
Author

toastx commented Sep 26, 2024

Smaller uint8arrays can be wrapped as array, and passed through it.

Larger has to either be parsed as b64, or tauri supports the datatype called arraybuffer, but it requires manual serialization and deserialization.

There is also a method called tauri-bindgen which avoids the IPC route, but not sure how it works.

tauri-apps/tauri#7706

tauri-apps/tauri#7706 (comment)

@Byron
Copy link
Collaborator

Byron commented Sep 26, 2024

That's wonderful! My takeaway from the issue is that this is still an unsolved problem when using the V1 IPC system. base64 encoding was endorsed as the better way to do it there.
Personally I'd not want to introduce or deal with tauri-bindgen for now.

@toastx
Copy link
Author

toastx commented Sep 26, 2024

All right, Ill begin on the base64 working then. Do let me know if u need anything in particular in the output struct, apart from content, type and name

@toastx
Copy link
Author

toastx commented Sep 28, 2024

So, I want to test the implementation, and I try following the steps from Development.md , but for some reason, I keep on getting error after error, as I keep resolving them. Is there any other guide or way to do it?

@Zamoca42
Copy link
Contributor

Could you show the logs to see what issues are occurring?
And have you tried this? https://tauri.app/ko/v1/guides/getting-started/prerequisites
If it’s a build-related error, it might be relevant.

@toastx
Copy link
Author

toastx commented Oct 1, 2024

Really sorry for the late reply,
image

i was getting different errors before, but I couldnt solve this one

Tried running the terminal is admin, didnt work,
reinstalled cmake, didnt work

@Byron
Copy link
Collaborator

Byron commented Oct 1, 2024

Did you install the Rust MSVC prerequisites?

Assuming that these are installed and working, I admit to have never seen a plain Access is denied. error, which probably indicates a lack of permission somewhere. But since it's already an admin shell, I wouldn't know what else there could be.

Sometimes it helps to run cargo clean, but that's all I've got.

It's notable that it fails trying to build libz-ng-sys, and with this patch, it shouldn't do that anymore.

diff --git a/crates/gitbutler-tauri/Cargo.toml b/crates/gitbutler-tauri/Cargo.toml
index 7f79734e9..0b1328102 100644
--- a/crates/gitbutler-tauri/Cargo.toml
+++ b/crates/gitbutler-tauri/Cargo.toml
@@ -28,7 +28,7 @@ console-subscriber = "0.4.0"
 dirs = "5.0.1"
 futures.workspace = true
 git2.workspace = true
-gix = { workspace = true, features = ["max-performance", "blocking-http-transport-curl", "worktree-mutation"] }
+gix = { workspace = true, features = ["blocking-http-transport-curl", "worktree-mutation"] }
 once_cell = "1.19"
 reqwest = { version = "0.12.4", features = ["json"] }
 serde.workspace = true

I hope that helps.

@toastx
Copy link
Author

toastx commented Oct 2, 2024

Yes bro, all the prerequisites have been installed. Have been using cmake for 2-3 projects as well. Not sure why this sudden access is denied

@Byron
Copy link
Collaborator

Byron commented Oct 2, 2024

What about the patch?

@toastx
Copy link
Author

toastx commented Oct 2, 2024

I am still getting the same access denied error, I'll try reinstalling Rust, MSVC and Cmake again tomorrow , and try it

@Byron
Copy link
Collaborator

Byron commented Oct 3, 2024

With the patch applied, libz-ng won't be compiled anymore, so any error would be a different one which would be good to share here.
Since this conversation is about getting it to compile on Windows and I have shared everything I know, I think it's best for me to focus on the PR itself and review it when ready.

@toastx
Copy link
Author

toastx commented Oct 6, 2024

still no luck man, i uninstalled and reinstalled everything. Idk why its happening tho. If someone else wants to take this over, Im cool with it.

@Zamoca42
Copy link
Contributor

Zamoca42 commented Oct 9, 2024

I was hoping that would be resolved, but it’s unfortunate. Would it be okay if I continue working on it?

@Byron
Copy link
Collaborator

Byron commented Oct 9, 2024

That would be nice, there certainly are no objections from my side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Req: Show binary files (like images) in file preview
3 participants