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.

Sign up for GitHub

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

fix: use proper LocalPairingState #17328

Merged
merged 1 commit into from
Feb 20, 2025
Merged

fix: use proper LocalPairingState #17328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/app/modules/onboarding/module.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ method onNodeLogin*[T](self: Module[T], err: string, account: AccountDto, settin

method onLocalPairingStatusUpdate*[T](self: Module[T], status: LocalPairingStatus) =
self.localPairingStatus = status
self.view.setSyncState(status.state.int)
self.view.setSyncState(status.state)

method onKeycardStateUpdated*[T](self: Module[T], keycardEvent: KeycardEventDto) =
self.view.setKeycardEvent(keycardEvent)
Expand Down
7 changes: 4 additions & 3 deletions src/app/modules/onboarding/view.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import NimQml
import io_interface, states
from app_service/service/keycardV2/dto import KeycardEventDto
from app_service/service/devices/dto/local_pairing_status import LocalPairingState

# TODO move these files to this module when we remove the old onboarding
import ../startup/models/login_account_model as login_acc_model
Expand All @@ -11,7 +12,7 @@ QtObject:
View* = ref object of QObject
delegate: io_interface.AccessInterface
keycardEvent: KeycardEventDto
syncState: int
syncState: LocalPairingState
Copy link
Member

Choose a reason for hiding this comment

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

Right, I think it makes sense to fix the below types to, to prevent future errors like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but we agreed that we should group other keycard command states, so I decided not to touch them here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but why can't they use the proper (same) type, instead of a plain int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They can and should, I just don't like mixing fixes in a single pr

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, eventhough a separate commit would do for me :)

addKeyPairState: int
pinSettingState: int
authorizationState: AuthorizationState
Expand Down Expand Up @@ -40,11 +41,11 @@ QtObject:

proc syncStateChanged*(self: View) {.signal.}
proc getSyncState(self: View): int {.slot.} =
return self.syncState
return self.syncState.int
QtProperty[int] syncState:
read = getSyncState
notify = syncStateChanged
proc setSyncState*(self: View, syncState: int) =
proc setSyncState*(self: View, syncState: LocalPairingState) =
self.syncState = syncState
self.syncStateChanged()

Expand Down
2 changes: 1 addition & 1 deletion storybook/pages/OnboardingLayoutPage.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ SplitView {
}

Repeater {
model: Onboarding.getModelFromEnum("ProgressState")
model: Onboarding.getModelFromEnum("LocalPairingState")

RoundButton {
text: modelData.name
Expand Down
9 changes: 9 additions & 0 deletions ui/StatusQ/src/onboarding/enums.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,20 @@ class OnboardingEnums: public QObject
Error,
};

// Keep in sync with LocalPairingState in src/app_service/service/devices/dto/local_pairing_status.nim
enum class LocalPairingState {
Idle,
Transferring,
Error,
Finished,
};

private:
Q_ENUM(PrimaryFlow)
Q_ENUM(OnboardingFlow)
Q_ENUM(LoginMethod)
Q_ENUM(KeycardState)
Q_ENUM(ProgressState)
Q_ENUM(AuthorizationState)
Q_ENUM(LocalPairingState)
};
8 changes: 4 additions & 4 deletions ui/app/AppLayouts/Onboarding2/pages/SyncProgressPage.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. Once the backend enum is changed, the UI component must be updated even if visually nothing changed (SRP)
  2. 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.


signal loginToAppRequested()
signal restartSyncRequested()
Expand All @@ -21,7 +21,7 @@ OnboardingPage {
states: [
State {
name: "inprogress"
when: root.syncState === Onboarding.ProgressState.InProgress || root.syncState === Onboarding.ProgressState.Idle
when: root.syncState === Onboarding.LocalPairingState.Transferring || root.syncState === Onboarding.LocalPairingState.Idle
PropertyChanges {
target: root
title: qsTr("Profile sync in progress...")
Expand All @@ -46,7 +46,7 @@ OnboardingPage {
},
State {
name: "success"
when: root.syncState === Onboarding.ProgressState.Success
when: root.syncState === Onboarding.LocalPairingState.Finished
PropertyChanges {
target: root
title: qsTr("Profile synced")
Expand All @@ -70,7 +70,7 @@ OnboardingPage {
},
State {
name: "failed"
when: root.syncState === Onboarding.ProgressState.Failed
when: root.syncState === Onboarding.LocalPairingState.Error
PropertyChanges {
target: root
title: "<font color='%1'>".arg(Theme.palette.dangerColor1) + qsTr("Failed to pair devices") + "</font>"
Expand Down