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

feat: make DialogFileBrowserOptions::new() use native initialization function #92

Merged

Conversation

JarvisCraft
Copy link
Contributor

@JarvisCraft JarvisCraft commented Jul 8, 2023

Description

In #80 safe bindings for dialogs app file browser were added by @mogenson. At this moment, the native initialization function was unsound since it forgot to initialize one of the fields so we decided to perform initialization on Rust side.

Now that this issues is fixed in flipperdevices/flipperzero-firmware#2756 with the future guarantee to always initialize all fields we can (and should) migrate to this approach which is better for us from backwards compatibility standpoint since we will no longer have to reflect the changes to this structure on our side for sake of safety.

Requirements

  • This PR requires the bindings to be updated to firmware version 0.86.1 which contains the required fix to the dialog_file_browser_set_basic_options.

Notes

I've also added a c_str macro which has magically become const-friendly with the recent stabilization of from_bytes_with_nul which will land to stable soon (at least, by the time we are ready to become stable-friendly; anyways, it is behind a macro so there is no explicit dependency on unstable toolchain as for now).

@JarvisCraft JarvisCraft force-pushed the native-file-browser-options-init branch from 7ab64b2 to 70bf1c6 Compare July 8, 2023 10:05
Copy link
Contributor

@mogenson mogenson left a comment

Choose a reason for hiding this comment

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

Lgtm!

crates/flipperzero/src/dialogs/mod.rs Outdated Show resolved Hide resolved
crates/sys/src/lib.rs Outdated Show resolved Hide resolved
JarvisCraft added a commit to JarvisCraft/flipperzero-rs that referenced this pull request Jul 11, 2023
JarvisCraft added a commit to JarvisCraft/flipperzero-rs that referenced this pull request Jul 11, 2023
@JarvisCraft JarvisCraft requested a review from dcoles July 11, 2023 14:01
JarvisCraft added a commit to JarvisCraft/flipperzero-rs that referenced this pull request Jul 11, 2023
@JarvisCraft JarvisCraft requested a review from str4d August 19, 2023 14:20
crates/sys/src/lib.rs Outdated Show resolved Hide resolved
crates/flipperzero/src/dialogs/mod.rs Show resolved Hide resolved
crates/flipperzero/src/dialogs/mod.rs Show resolved Hide resolved
crates/flipperzero/src/dialogs/mod.rs Show resolved Hide resolved
JarvisCraft added a commit to JarvisCraft/flipperzero-rs that referenced this pull request Aug 20, 2023
crates/sys/src/lib.rs Outdated Show resolved Hide resolved
@JarvisCraft JarvisCraft added the enhancement New feature or request label Aug 27, 2023
@JarvisCraft JarvisCraft self-assigned this Aug 27, 2023
@JarvisCraft JarvisCraft added this to the v0.11.0 milestone Aug 27, 2023
@JarvisCraft JarvisCraft requested a review from str4d August 29, 2023 13:11
@str4d str4d mentioned this pull request Aug 30, 2023
@str4d
Copy link
Contributor

str4d commented Dec 2, 2023

Build is failing.

@JarvisCraft
Copy link
Contributor Author

JarvisCraft commented Dec 3, 2023

Build is failing.

Yup, I am going to fix that, but now that cstr-literals are stabilized in 1.76, I may rework this PR to omit the redundant dependency.

@JarvisCraft JarvisCraft marked this pull request as draft December 3, 2023 15:07
JarvisCraft added a commit to JarvisCraft/flipperzero-rs that referenced this pull request Dec 3, 2023
@JarvisCraft JarvisCraft force-pushed the native-file-browser-options-init branch 3 times, most recently from deebdac to 6ec32b2 Compare December 3, 2023 18:54
@JarvisCraft
Copy link
Contributor Author

JarvisCraft commented Dec 3, 2023

Important

Since this PR effectively depends on #120, Clippy should be fixed by the latter.

@JarvisCraft JarvisCraft force-pushed the native-file-browser-options-init branch from 6ec32b2 to 14ef1b9 Compare September 11, 2024 17:34
@JarvisCraft JarvisCraft marked this pull request as ready for review September 11, 2024 17:39
@JarvisCraft JarvisCraft force-pushed the native-file-browser-options-init branch 4 times, most recently from 7582078 to b0ef9f9 Compare September 12, 2024 10:59
@JarvisCraft JarvisCraft force-pushed the native-file-browser-options-init branch from b0ef9f9 to 4b67f07 Compare September 12, 2024 11:00
@JarvisCraft JarvisCraft force-pushed the native-file-browser-options-init branch from 4b67f07 to bf865a5 Compare September 12, 2024 11:02
@JarvisCraft JarvisCraft merged commit f78fef3 into flipperzero-rs:main Sep 12, 2024
9 checks passed
@JarvisCraft JarvisCraft deleted the native-file-browser-options-init branch September 12, 2024 11:05
@JarvisCraft JarvisCraft modified the milestones: v0.11.0, v0.13.0 Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants