Skip to content

Remote ID Toolbar Indicator: code review findings #14221

@DonLakeFlyer

Description

@DonLakeFlyer

Code Review: Remote ID Toolbar Indicator

Full code review of RemoteIDIndicator.qml and RemoteIDIndicatorPage.qml.

Bugs

  1. Null dereference on showIndicator (RemoteIDIndicator.qml:15)
    showIndicator: remoteIDManager.available — when activeVehicle is null, remoteIDManager is null, and accessing .available produces a QML warning. Should be:

    showIndicator: remoteIDManager ? remoteIDManager.available : false
  2. remoteIDState is not reactive (RemoteIDIndicator.qml:26)
    property int remoteIDState: getRemoteIDState() — calls the function once at initialization. QML won't re-evaluate when commsFlag, armFlag, etc. change because imperative function calls don't create bindings on the properties read inside the function.

  3. Undefined remoteIDManager reference (RemoteIDIndicatorPage.qml:324)
    labelText: remoteIDManager.armStatusError — there is no remoteIDManager property in this file. Will produce a runtime ReferenceError. Should be:

    labelText: _activeVehicle ? _activeVehicle.remoteIDManager.armStatusError : ""
  4. Operator ID visibility — unqualified enum reference (RemoteIDIndicatorPage.qml:193)
    _regionOperation == RemoteIDIndicatorPage.EU — should be RemoteIDIndicatorPage.RegionOperation.EU for consistency with the fully qualified form used elsewhere.

Cleanup

  1. Redundant break after return (RemoteIDIndicator.qml:50,53,56,59)
    Each case in getRidColor() has return ... break. The break is unreachable.

  2. Typo in id: basicIDFlagIge (RemoteIDIndicatorPage.qml:159)
    Should be basicIDFlagImage.

  3. Duplicated RegionOperation enum
    Defined in both RemoteIDIndicator.qml and RemoteIDIndicatorPage.qml. Should be in one place.

  4. Unused enums (RemoteIDIndicatorPage.qml:41-49)
    ClassificationType is never referenced. LocationType is only used once in the Connections handler.

  5. visible: true is redundant (RemoteIDIndicatorPage.qml:237,248)
    Both emergencyDeclareLabel and emergencyButton specify the default value.

  6. Redundant null checks on flag properties (both files)
    activeVehicle && remoteIDManager ? — if remoteIDManager is non-null, activeVehicle is necessarily non-null. Could simplify with a local remoteIDManager property in the indicator page too.

Design Improvements

  1. Emergency button — fragile source string comparison (RemoteIDIndicatorPage.qml:258-273)
    Timer and press-and-hold toggle the image by comparing emergencyButton.source against path strings. Use a boolean state property instead.

  2. Emergency button — no visual press feedback
    User must press-and-hold for 800ms/3000ms with no indication the press is being registered until the hold completes.

  3. Expanded panel Connections uses deprecated signal syntax (RemoteIDIndicatorPage.qml:300)
    onRawValueChanged — Qt6 prefers function syntax.

  4. Expanded panel side-effects in Connections (RemoteIDIndicatorPage.qml:302-308)
    Changing sendOperatorIdFact.rawValue and locationTypeFact.value in response to region changes is business logic that should live in C++, not in a UI indicator panel.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Relationships

None yet

Development

No branches or pull requests

Issue actions