fix: use proper LocalPairingState #17328
Conversation
| @@ -11,7 +12,7 @@ QtObject: | |||
| View* = ref object of QObject | |||
| delegate: io_interface.AccessInterface | |||
| keycardEvent: KeycardEventDto | |||
| syncState: int | |||
| syncState: LocalPairingState | |||
There was a problem hiding this comment.
Right, I think it makes sense to fix the below types to, to prevent future errors like this
There was a problem hiding this comment.
Yes, but we agreed that we should group other keycard command states, so I decided not to touch them here.
There was a problem hiding this comment.
Yeah but why can't they use the proper (same) type, instead of a plain int?
There was a problem hiding this comment.
They can and should, I just don't like mixing fixes in a single pr
There was a problem hiding this comment.
Fair enough, eventhough a separate commit would do for me :)
Jenkins Builds
|
| @@ -12,7 +12,7 @@ import AppLayouts.Onboarding.enums 1.0 | |||
| OnboardingPage { | |||
| id: root | |||
|
|
|||
| required property int syncState // Onboarding.ProgressState.xxx | |||
| required property int syncState // Onboarding.LocalPairingState.xxx | |||
There was a problem hiding this comment.
Not a problem of that pr, but in general using nim side counterparts of enums in pure UI components is not optimal in general for couple reasons:
- Once the backend enum is changed, the UI component must be updated even if visually nothing changed (SRP)
- The states of the external enum may not reflect the need of the UI component itself, like in that case where we need to do mapping from two states into one, deeply in ui component.
Ideally the states of the UI component should be defined by the component itself, and mapped to nim/backend states in upper level, responsible for that mapping. The drawback is maintaining mapping itself, but when gathered in single place responsible exclusively for that it should result with better and cleaner design.
As mentioned, nothing to change in that pr but raising problem for further consideration.
igor-sirotin
force-pushed
the
fix/17283-local-pairing
branch
from
2b8d487 to
4db2e02
Compare
Fixes #17283
Description
ProgressStatewas used instead of theLocalPairingState. Fixed that.Screenshot
Screen.Recording.2025-02-18.at.18.13.01.mov