Toggle scenes/actions for watchOS App all at once #2437
Conversation
bgoncal
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2437 +/- ##
==========================================
- Coverage 28.07% 28.03% -0.05%
==========================================
Files 273 273
Lines 30209 30232 +23
==========================================
- Hits 8481 8475 -6
- Misses 21728 21757 +29
|
|
The real crux of the issue is the startup routine for HA causes the entire scene domain to have nothing in it, which makes the app remove all its local copies. Once it later populates the scenes, it recreates them and loses the disabled state. A good fix for this probably would involve not listening to state changes until core finishes loading all the way. I think some integrations may also work by deleting all then restoring them, so moving enabled state out of Realm might also be required. |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Indeed some clean up needs to be done in the startup flow, I'll put it on my to-do list to look into it since it may require more time for me that's not familiar with it yet. |
| @@ -454,7 +453,26 @@ class SettingsDetailViewController: HAFormViewController, TypedRowControllerType | |||
| $0.disabled = true | |||
| }, | |||
| ], getter: { | |||
There was a problem hiding this comment.
you'll need a [weak self] here too
There was a problem hiding this comment.
Not needed anymore after my last commit.
| @@ -454,7 +453,26 @@ class SettingsDetailViewController: HAFormViewController, TypedRowControllerType | |||
| $0.disabled = true | |||
| }, | |||
| ], getter: { | |||
| Self.getSceneRows($0) | |||
| var baseRows: [BaseRow] = [] | |||
| if $0 == scenes.first { | |||
There was a problem hiding this comment.
i think it would be better to put a new section at the top for this, or a new RealmSection watching it. this is sort of side-effect-y -- this 'value' needs to update live with contents other than the first one coming in/out, too, right?
There was a problem hiding this comment.
Good one, I forgot to reflect the state on manually changing the scene toggles. It's done now in my latest commit.
| toggleAllSwitch.title = L10n.SettingsDetails.Actions.Scenes.selectAll | ||
|
|
||
| scenesUpdateObserver = scenes.observe { _ in | ||
| // Access UISwitch directly to set state to avoid triggering "on value change" |
There was a problem hiding this comment.
what happens if you change the model instead, and that triggers a cell update?
There was a problem hiding this comment.
Any changes on scenes model should result in this block of code being executed and updating the "select all" toggle state, is this what you mean or did I misunderstand?
There was a problem hiding this comment.
Don't put this in one of the model update blocks. Create the model, set up this observer to edit the model, and set the callbacks on the model for when the user toggles it.
There was a problem hiding this comment.
I don't think "Select all" should be part of the model, it's just a visual representation that reacts according to the group of scenes state.
There was a problem hiding this comment.
I updated the PR based on our talk in discord, also I moved the cell a little bit:
bgoncal
force-pushed
the
toggle-actions
branch
from
e47f4a7 to
659b558
Compare
Summary
When you open your watchOS App for the first time you may encounter several scenes already active to be used in the watch App (specially if you have Philips Hue in HA), this change allows you to unselect all scenes at once (hide from watch app) if you want.
Screenshots
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-11-01.at.13.56.46.mp4
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#
Any other notes