-
Notifications
You must be signed in to change notification settings - Fork 129
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
Settings Sync: Fix Up Next settings #1615
Conversation
5635445
to
ab00f17
Compare
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.
@bjtitus this seems to not be working for me:
not_working_1080.mov
If I change the flag to false
, the correct values return.
addToUpNext = false | ||
addToUpNext = newValue.enabled | ||
if let position = newValue.position { | ||
addToUpNextPosition = position |
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.
Not sure if it matters, but shouldn't addToUpNextPosition
be optional? In case it has no position, is nil.
As it stands it will be the previous value that had a position
.
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 can't be optional in the Protobuf settings so I was thinking we shouldn't make it optional in our local model.
var enabled: Bool { | ||
get { | ||
return self != .off | ||
} | ||
} |
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.
var enabled: Bool { | |
get { | |
return self != .off | |
} | |
} | |
var enabled: Bool { | |
self != .off | |
} |
577c6cc
to
47487ee
Compare
Fixes a minor issue when importing the Up Next setting. Previously this passed through the new update mechanism which would set the
ModifiedDate
to the current date, so this setting would be overwritten by the last device to import, which was not the original guidelines. We want any imported values to be assigned an epoch date (done in #1616), not the current date, so they are clearly delineated from changed settings.To test
shouldEnableSyncedSettings
inFeatureFlag.swift
to falsenewSettingsStorage
andsettingsSync
feature flagsChecklist
CHANGELOG.md
if necessary.