-
-
Notifications
You must be signed in to change notification settings - Fork 263
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
Implement auto discovery #126
Conversation
I love this, thank you so much! The only criticism I could spot in a quick review would probably be the standard AlertDialog with the newlines, but for now that's absolutely fine and in the future I may replace it with another UI anyways. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the correct approach in adding server discovery. In my opinion it should always start when the server selection screen is shown and display the results in the activity instead of using an dialog.
@@ -2,6 +2,7 @@ | |||
xmlns:tools="http://schemas.android.com/tools" | |||
package="org.jellyfin.mobile"> | |||
|
|||
<uses-permission android:name="android.permission.ACCESS_WIFI_STATE" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this permission required for discovery to work? If so I'll add it to the apiclient for the next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, is required, also is already documented in the source code by ... you (?) https://github.com/jellyfin/jellyfin-apiclient-java/blame/1565f286a3b676cdf85093b0ee99c4b940b99660/android/src/main/kotlin/org/jellyfin/apiclient/discovery/AndroidBroadcastAddressesProvider.kt#L18
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's funny how I knew it was required but forgot to actually add it to the manifest lol
Unfortunately for me the discovery of the cordova client never worked so, I don't know how it used to be the discovery UI, for me the UI is the login screen with a "Cancel" button that redirects to a no-op selectServer.html bundled in the web client (notice the height change in the title bar), but you are free to reimplement the UI to follow the original behavior / flow. |
f139eab
to
85c693b
Compare
Immediately show server chooser after the first server has been discovered, replace withContext() with flowOn()
85c693b
to
bc6ea9b
Compare
Button only visible when at least one server is found
Closes #115 👀