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

Navigation #239

Merged
merged 32 commits into from
Nov 26, 2024
Merged

Navigation #239

merged 32 commits into from
Nov 26, 2024

Conversation

mobile-bungalow
Copy link
Contributor

@mobile-bungalow mobile-bungalow commented Nov 18, 2024

To address #166 and tangentially #212 this PR aims to add a web like navigation API to the LiveSocket object.


  • Add tests to swift and jetpack client testing code.
  • Internal structure and testing.
  • FFI interface with connection error percolation.

/// Destination URL
pub to: NavHistoryEntry,
/// Additional user provided metadata handed to the event handler.
pub info: Option<Vec<u8>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this match the name state in the NavHistoryEntry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

info and state are two different things, the history entry contains persistent state which will always be included whenever it shows up in a navigation event, or when NavCtx::current accquires it. info is ephemeral and is passed to the event once.

I based this off of the web navigation api's naming scheme which is honestly not great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

event_specific_info might be more descriptive.

@mobile-bungalow mobile-bungalow marked this pull request as ready for review November 22, 2024 17:15

#[derive(uniffi::Object)]
pub struct LiveSocket {
pub socket: Mutex<Arc<Socket>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused. Why's socket a Mutex<Arc<Socket>>? Wouldn't the mutex be locked for the entire time that someone has a reference to the return of fn socket(&self)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we have a really bizarre API underlying this which returns an immutable object inside and Arc because the socket was expected to be orchestrated over FFI. So there is no way around the having the internal smart pointer.

When you lock it and clone the Arc you get an immutable reference to the internal smart pointer without the mutex. The only thing that holds a reference to these sockets is the LiveChannel struct, so it's beholden upon the consumer to drop the old dead LiveChannels, which are now disconnected, to avoid a memory leak.

Comment on lines +210 to +214
#[cfg(target_os = "android")]
const HOST: &str = "10.0.2.2:4001";

#[cfg(not(target_os = "android"))]
const HOST: &str = "127.0.0.1:4001";
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I think you can use super::HOST.

Copy link
Contributor

@simlay simlay left a comment

Choose a reason for hiding this comment

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

I think you've covered all my suggestions.

@mobile-bungalow mobile-bungalow merged commit 23d277c into liveview-native:main Nov 26, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants