Dialogs app file browser wrapper

[mogenson]

Add a safe Rust wrapper for the dialog file browser methods and the
DialogsFileBrowserOptions type. Rename the DialogsApp::show() method
to show_message() and create new show_file_browser() method. Add a
getter method to FuriString to provide access to the inner
sys::FuriString via a raw pointer because a sys::FuriString is needed by
a few of the Furi C API functions. Extend the storage example to open a
file browser and have the user select the file that was just written to
the file system.

Note: The Furi C struct is named DialogsFileBrowserOptions, but all
other dialog types and functions are singular instead of plural. The
Rust type is named DialogFileBrowserOptions.

The DialogFileBrowserOptions type uses setter methods that return Self
so they can be chained together in the builder pattern. The
DialogFileBrowserOptions includes a Rust unit type phantom data member.
The type or size does not really matter, but PhantomData does require a
sized type. We just need one struct member to carry the lifetime
parameter so that CStr and Icon references are not dropped until the
struct goes out of scope.

[mogenson]

@JarvisCraft are you ok with merging this now?

[JarvisCraft]

I actually am not the one with push permissions, so I summon @dcoles and @str4d for this purpose :)

[dcoles]

Hi @mogenson,

Thanks for the PR! This looks great.

Most of my feedback is just minor style things, though I'd say the as_ptr returning a *mut sys::FuriString is important to address before I can merge this.

[dcoles]

This should take &mut self and be called as_mut_ptr since this effectively allows you mutable access to the string data.

edit: as_mut → as_mut_ptr

[JarvisCraft]

I would disagree. core and std types do not require mut on self since the raw pointer itself cannot be used in any bad way, while its dereferenceing is anyway unsafe.

Also our existing APIs use &self, for example UnsafeRecord::as_ptr(&self)

[mogenson]

I was a bit unsure about this method because it's just a wrapper for the as_ptr() method of the core NonNull type. NonNull<T>::as_ptr() takes self and returns an *mut T.

[str4d]

This method absolutely needs to take &mut self. AFAICT the only place this PR uses this method is sys::dialog_file_browser_show, which mutates the provided sys::FuriString to store the selected path. From the Rust perspective, this is mutating the FuriString and therefore requires exclusive access.

I also intentionally did not add this method to the public API, because I do not want it to exist there; it enables violating the safety requirements relied on by the FuriString struct. We should instead be crafting safe APIs that wrap the FFI methods taking *mut sys::FuriString, inside which we can call this in a crate-private setting where we know the safety requirements are satisfied.

Finally, we should name this as_mut_ptr to match equivalent APIs in the Rust stdlib (slice::as_mut_ptr, Vec::as_mut_ptr, str::as_mut_ptr). I'd personally also prefer that this method be unsafe, though technically obtaining this kind of reference is safe Rust (it's just using said reference that is unsafe), so I'm not going to require that. Note that the only stdlib methods named as_ptr that return *mut T are the ones on atomic types (where obtaining a mutable pointer through a shared reference is safe) and NonNull (because it specifically represents *mut T, not *const T).

Suggested change

	pub fn as_ptr(&self) -> *mut sys::FuriString {
	pub(crate) fn as_mut_ptr(&mut self) -> *mut sys::FuriString {

[dcoles]

@JarvisCraft As far as I'm aware methods like str::as_mut_ptr all need &mut self (or self so they consume the original object).

NonNull::as_ptr is a little odd naming wise since it gives a *mut T, but since it consumes the NonNull its safe to do so.

[JarvisCraft]

From the other side, I like the crate-local access modifier so I generally like @str4d's suggestion.

[mogenson]

I understand why &mut self makes sense because the *mut sys::FuriString could be mutated. For this PR, we'd also need a mutable reference for the preselected file path variable in show_file_browser(). Maybe we can include a note in the comment doc that although this method needs a mutable reference, the starting file path will not be modified. It would be better if the sys::dialog_show_file_browser() C function took a *const FuriString for path instead of *FuriString.

[JarvisCraft]

Actually, you can propose this change to flipperdevices/flipperzero-firmware. I've done similar job with adding const-qualifiers and the maintainers were fine with merging them.

[dcoles]

Actually, NonNull is impl Copy so it does not effectively consume it.

Oh! I did not realize that was the case. OK. That makes more sense. Also I see NonNull<T> is defined as being the same as *mut T, so there's no need for a mut self, because self is a pointer-to-mut-T.

In my opinion, adding &mut is not the right choice here.

I see this is analogous to String::as_ptr(&self) -> *const u8 and String::as_mut_ptr(&mut self) -> *mut u8. Sure, you can freely cast back and forth between *const and *mut (since Rust won't let you actually do anything with these pointers in safe code). However if you (the user) violate mutability rules (e.g. by creating a mut* to an immutable object and then mutatating it in unsafe code), then that is not Rust's fault. You, the user, are responsible for upholding Rust's mutability rules in unsafe code.

const* doesn't really mean all that much in Rust (or C). It's essentially type-checked documentation. About the only thing it does in Rust is prevents you from performing mutable operations when you dereference it.

Is it useless? No, it's still helpful to have the compiler act as a safety. The furi_string_ methods are all const-correct and will stop you if you try to mutate something through a const*. If you take off the safety and point it at your foot, well you're in unsafe code, so neither Rust or C is going to stop you.

This would all be far less confusing if we could have done in-place/#[repr(transparent)] allocation of FuriString (flipperdevices/flipperzero-firmware#2461). Then it's super clear that you can't get a *FuriString that you can legally write through without having a mut self first.

From the other side, I like the crate-local access modifier so I generally like @str4d's suggestion.

OK, I have no problem with keeping this pub(crate).

[dcoles]

Maybe we can include a note in the comment doc that although this method needs a mutable reference, the starting file path will not be modified. It would be better if the sys::dialog_show_file_browser() C function took a *const FuriString for path instead of *FuriString.

This is one totally legitimate case for *const T as *mut T. It's fine to call FuriString::as_ptr() -> *const sys::FuriString and then cast it to a *mut sys::FuriString so long as you don't modify memory through this pointer.

Ultimately what matters (for correct compilation) is "does sys::dialog_show_file_browser() actually mutate the data behind the path pointer?", not whether the function declaration says const *FuriString or *FuriString.

[mogenson]

After inspecting the flipperzero-firmware code, dialog_show_file_browser() will not modify the preselected file path FuriString, but some common storage processing functions will replace /ext, /app, /any prefixes depending on the context. I think pub(crate) fn as_mut_ptr(&mut self) -> *mut sys::FuriString make sense, at least for the show_file_browser() usage. Is everyone in agreement?

[mogenson]

Also, is there anything I need to do with this PR to get the Clippy (MSRV) lints CI to succeed?

[mogenson]

@dcoles @JarvisCraft @str4d Are there any remaining issues or is this PR able to be merged?

[dcoles]

Also, is there anything I need to do with this PR to get the Clippy (MSRV) lints CI to succeed?

@str4d Would you be able to take a look at this? I'm not quite sure why the beta is fine, but MSRV is throwing a Unable to run @augu/clippy-action: HttpError: Resource not accessible by integration.

[dcoles]

@dcoles @JarvisCraft @str4d Are there any remaining issues or is this PR able to be merged?

Yes, let's get this merged. Thanks very much for your patience and for the PR!

[mogenson]

Cheers, thanks!