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

Merging UserSettings to main #1290

Merged
merged 22 commits into from
Aug 22, 2023
Merged

Merging UserSettings to main #1290

merged 22 commits into from
Aug 22, 2023

Conversation

mchowning
Copy link
Contributor

@mchowning mchowning commented Aug 18, 2023

Description

This is merging to main the refactoring of the to-be-synced settings to be UserSettings.

Testing Instructions

All of these changes have already been tested in separate PRs, so they shouldn't need to be retested individually.

The one exception to this is the settings on the Notification screen because I had to resolve some non-trivial merge conflicts when merging in main due to #1264 and #1244. It's worth giving those another quick test to just make sure there aren't any issues.

Warning
7.46 has been cut, so this is no longer relevant. This can be reviewed now, but let's not merge it until after the 7.46 release is cut since the number of changes in here creates a bit of a risk that there could be a regression, so it would be good to have as much time as possible to catch any issues.

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews

@mchowning mchowning requested a review from a team as a code owner August 18, 2023 18:41
@mchowning mchowning added this to the 7.47 milestone Aug 18, 2023
Copy link
Contributor

@ashiagr ashiagr left a comment

Choose a reason for hiding this comment

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

I smoke-tested a few settings, including notifications. They all work as expected.

Great job getting this done!

@mchowning
Copy link
Contributor Author

I pushed an update in 43c3eda to address the proguard issue in release builds. Let me know what you think.

@ashiagr
Copy link
Contributor

ashiagr commented Aug 22, 2023

LGTM 👍

@CookieyedCodes
Copy link

Now I know this project is makeing progress for a future release, and I know notification, add to upnext (including which upnext/uplast selection) & auto downloads & per podcast playback FX are unlikely to sync but if their is a chance you can make this more seemless alot of people wouldn't mind and with individual experimental advanced option to turn on sync for these (obviously with clear warnings if required

the switching device pain is one of the few complaints I regularly see about podcatchers (now should this be a patron level feature I'll leave you decide but it might erk people & you'd get good brownie points with alot of users if the options were at least their🤔) let me know tho some of the technical reasons why it would be difficult if it is tho id be very interested just so I can tell others as I see it popup 🤔 😅

@mchowning
Copy link
Contributor Author

I know notification, add to upnext (including which upnext/uplast selection) & auto downloads & per podcast playback FX are unlikely to sync

We're actually planning to sync all of these except for auto downloads (because we feel people might have different auto download preferences on different devices, not because of technical limitations).

@mchowning mchowning enabled auto-merge (squash) August 22, 2023 12:22
@CookieyedCodes
Copy link

Great stuff, I'll probably be the person who checks with the high numbers sync as I have 413 notifications, 335 set to auto download & 326 set to add to upnext 😅, so please be careful, I'll definitely let you know tho, I'll also note add to upnext might cause complications if archive/stared sync isn't timed right

I think I would agree tho I still think maby an advanced option would be appropriate to try cut down on list comparisons, I also think if you figure out how local playback effects sync is done it should apply to the webapp too as episodes switch which would add some added complexity 🤔

Looking forward to testing tho I will be cautious ❤️❤️

@mchowning mchowning merged commit c46d2c4 into main Aug 22, 2023
@mchowning mchowning deleted the feature/sync-settings-refactor branch August 22, 2023 12:56
@ashiagr ashiagr mentioned this pull request Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants