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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow hiding complications when watch is locked #1253

Merged
merged 17 commits into from Nov 2, 2020
Merged

Conversation

TomBrien
Copy link
Member

Given that we essentially let users add anything from Home Assistant that can be represented as text as a complication I thought it would be a good idea to let people choose whether or not to display a complication when the watch is locked.

Tested on my watch and seems to be working 馃槸although I haven't tested the Realm migration yet. Apologies in advance, this is my first time trying something of this complexity (for me, appreciate it's actually pretty simple)

@TomBrien TomBrien changed the title All ow hiding complications on when watch is locked Allow hiding complications on when watch is locked Oct 29, 2020
@TomBrien
Copy link
Member Author

Hmm I'm getting:

Database Error
Migration is required due to the following errors:

  • Property 'WatchComplication.IsPrivate' hass been added.

Pop up. Not sure what I did differently to https://github.com/home-assistant/iOS/blob/master/Sources/Shared/Common/Extensions/Realm%2BInitialization.swift#L89-93

Forgot to commit this earlier
@TomBrien
Copy link
Member Author

Migration should be resolved now

@TomBrien TomBrien changed the title Allow hiding complications on when watch is locked Allow hiding complications when watch is locked Oct 29, 2020
Copy link
Member

@zacwest zacwest left a comment

Choose a reason for hiding this comment

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

overall nice job! just a couple suggestions.

@@ -184,6 +189,11 @@ class ComplicationEditViewController: FormViewController, TypedRowControllerType
cell.detailTextLabel?.text = row.value?.style
}

<<< SwitchRow("IsPrivate") {
$0.title = L10n.Watch.Configurator.Rows.IsPrivate.title
$0.value = self.config.IsPrivate
Copy link
Member

@zacwest zacwest Oct 31, 2020

Choose a reason for hiding this comment

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

I think it is worth flipping the way this is presented. "Show When Locked {default on}" is a little easier to mentally discern than the double-negative "Hide when locked {default off}".


let model: WatchComplication?

if #available(watchOS 7, *), complication.identifier != CLKDefaultComplicationIdentifier {
Copy link
Member

@zacwest zacwest Oct 31, 2020

Choose a reason for hiding this comment

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

given this is from another method in this class, i think it's worth writing a helper function like func complication(for complication: CLKComplication) -> WatchComplication?

@TomBrien
Copy link
Member Author

TomBrien commented Nov 1, 2020

Had a bit of issues getting complications to sync. Should note I've seen this since the restructuring of complications for watchOS 7. I think it may be a me thing but interested if can be reproduced by others. Complication privacy is still working and tested though. Bottom complications are both HA left is set with Show on When Locked true right has it false.
Unlockedimage
Locked image

Copy link
Member

@zacwest zacwest left a comment

Choose a reason for hiding this comment

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

just minor - let's try and keep to swift-isms for function names here

@@ -16,8 +16,8 @@ class ComplicationController: NSObject, CLKComplicationDataSource {
// https://github.com/LoopKit/Loop/issues/816
// https://crunchybagel.com/detecting-which-complication-was-tapped/

private func template(for complication: CLKComplication) -> CLKComplicationTemplate? {
Iconic.registerMaterialDesignIcons()
private func getComplicationModel(for complication: CLKComplication) -> WatchComplication? {
Copy link
Member

Choose a reason for hiding this comment

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

prefixing functions with 'get' is not really a swift-ism that works super well -- apple's been leaning on doing this only for asynchronous returns (e.g. it provides a block)


let model: WatchComplication?

model = getComplicationModel(for: complication)
Copy link
Member

Choose a reason for hiding this comment

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

if you pull this up into the variable declaration above you can avoid needing to declare the type, too

@zacwest zacwest merged commit 55d472c into master Nov 2, 2020
@zacwest zacwest deleted the complication-privacy branch November 2, 2020 17:21
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.

None yet

4 participants