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

Allow changing download network preference #1331

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

taitsmith
Copy link
Contributor

Changes
Adds a section in the settings screen which gives users the option of changing their preferred download network. Previously, users were unable to allow or restrict mobile data downloads after making the initial choice. Additionally updates the message displayed when the initial dialog is displayed to inform users their choice can be updated.

ss_dialog_update
ss_allowed_dialog

Issues
Fixes #1326

@jellyfin-bot jellyfin-bot added this to the v2.7.0 milestone Apr 4, 2024
@taitsmith
Copy link
Contributor Author

The issue I ran into is that singleChoice() expects a string key, and DownloadMethod is stored as an Int. As it is, the PR creates items with string keys and separately handles the updating of the download preference in defaultOnSelectionChange, which obviously is not ideal given that some amount of unnecessary work is being done and unneeded preferences are being created. Potential other solutions were updating the library to create something like singleChoiceInt() which seems like quite a bit of work for this one issue, or providing multiple check boxes like the player options currently provide.

Maxr1998
Maxr1998 previously approved these changes Apr 7, 2024
@Maxr1998
Copy link
Member

Maxr1998 commented Apr 7, 2024

The issue I ran into is that singleChoice() expects a string key, and DownloadMethod is stored as an Int.

True, that's definitely a shortcoming of the library. Not a huge problem though and we can change this later when the library gets updated to directly support ints, I believe.. the only thing that is a bit of a pain is that this new preference will store the setting twice (as int in pref_download_method and as string in download_method, the latter being unused), and currently doesn't restore the current preference value from the int preference. Could you look into fixing the second point? Should be relatively simple.

Otherwise, this looks pretty good.

Edit: the new 2.4.0-beta2 of the preferences libraries will support singleChoice with int keys, so we can also use that. I'll update to that version once the distribution is complete, and you can rebase on top of the latest master branch and make the required changes.

Edit 2: done, you can now proceed.

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Apr 7, 2024
@taitsmith
Copy link
Contributor Author

awesome thanks, was actually going to sit down today and look into updating the library so i'll get this wrapped up

@Maxr1998 Maxr1998 force-pushed the update_download_preferences_1326 branch from f055712 to a850479 Compare April 23, 2024 13:49
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Apr 23, 2024
@Maxr1998
Copy link
Member

Rebased your PR onto the latest master branch and fixed the conflicts. Thanks for your contribution!

@Maxr1998 Maxr1998 enabled auto-merge (squash) April 23, 2024 13:50
@Maxr1998 Maxr1998 disabled auto-merge April 23, 2024 13:50
@Maxr1998 Maxr1998 changed the title Allow changing network download preferences Allow changing download network preferences Apr 23, 2024
@Maxr1998 Maxr1998 changed the title Allow changing download network preferences Allow changing download network preference Apr 23, 2024
@Maxr1998 Maxr1998 enabled auto-merge (squash) April 23, 2024 13:51
@Maxr1998 Maxr1998 merged commit 7644358 into jellyfin:master Apr 23, 2024
7 checks passed
@Maxr1998 Maxr1998 added the enhancement New feature or request label Apr 23, 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.

Option to change Download Preference from WiFi to Mobile Data not available after initial selection
3 participants