Rework UI architecture and introduce Jetpack Compose navigation#29
Rework UI architecture and introduce Jetpack Compose navigation#29angel-ayala wants to merge 1 commit intodevelopfrom
Conversation
- Implement `PrefsManager` for persistent configuration of IP addresses and theme modes.
- Refactor `MainActivity` to use `AppNavigation` and Jetpack Compose for the primary UI.
- Introduce a new navigation system with dedicated screens for Dashboard, Settings, and About.
- Add `SettingsViewModel` to manage application configuration state.
- Create a modular UI structure with specialized screens:
- `MainScreen`: Dashboard with status indicators, telemetry summary, and service controls.
- `AddressScreen`: Configuration for MAVLink and WebRTC IP addresses.
- `ThemeScreen`: Support for Light, Dark, and System theme modes.
- `AboutScreen`: Information about the project and core team.
- `KeyScreen`: Read-only view of the DJI API key.
- Enhance the theme engine with a custom color palette and improved Material3 integration.
- Update `build.gradle.kts` with dependencies for navigation and extended icons.
angel-ayala
left a comment
There was a problem hiding this comment.
I just did a review alongside the code and its already updated with develop and used the same state flow variables so far, apparently addressing all architectural changes. Major changes require to remove all DJI API KEY related code, reorganize the available settings, and set the services address from PrefManager. I found a KeyScreen view which is not required since part of the project core is the SDK and its API key, given for developers under a package name. Therefore, the apiKey value introduce in #22 and defined in the local.properties file seems the best fit for key management. Not sure if could show current value instead, for me a status message at main screen is be enough.
Another changes required are focused to the setting options. Initially, must offer settings for MAVLink, WebRTC, and WenuLink Interface. Each of them using a prefix to correctly identify its corresponding preference key, with both MAVLink and WebRTC storing an address instead of an IP, since additionally require a port. Future versions for MAVLink address, could deal with the pattern "PROTOCOL:IP:PORT" if other transport protocol is implemented, such as, TCP.
A final refactor could be rename and move the PrefManager to WenuLinkPreferences in the base package org.WenuLink.
| Triple("IP Addressing", "MAVLink and WebRTC IP Settings", Screen.ConfigIp), | ||
| Triple("DJI API KEY", "DJI Registration Details", Screen.ConfigDji), | ||
| Triple("Interface", "Theme & Display", Screen.ConfigTheme) |
There was a problem hiding this comment.
| Triple("IP Addressing", "MAVLink and WebRTC IP Settings", Screen.ConfigIp), | |
| Triple("DJI API KEY", "DJI Registration Details", Screen.ConfigDji), | |
| Triple("Interface", "Theme & Display", Screen.ConfigTheme) | |
| Triple("MAVLink protocol", "MAVLink and WebRTC IP Settings", Screen.ConfigMAVLink), | |
| Triple("WebRTC streaming", "DJI Registration Details", Screen.ConfigWebRTC), | |
| Triple("WenuLink Interface", "Theme & Display", Screen.ConfigTheme) |
| } | ||
|
|
||
| fun getMavlinkIp(context: Context): String { | ||
| return getPrefs(context).getString(KEY_MAVLINK_IP, "192.168.1.220") ?: "192.168.1.220" |
There was a problem hiding this comment.
| return getPrefs(context).getString(KEY_MAVLINK_IP, "192.168.1.220") ?: "192.168.1.220" | |
| return getPrefs(context).getString(KEY_MAVLINK_GCS_ADDR, "192.168.1.220:14550") ?: "192.168.1.220:14550" |
TimonSchreiber
left a comment
There was a problem hiding this comment.
The new UI is a clear improvement in usability and structure. The navigation system,
portrait/landscape layouts, settings screens, and custom theme engine are all well
thought out, and the DashboardUiState data class is a good step toward a clean
separation between state and presentation.
I have grouped my comments below by severity.
Supporting @angel-ayala's feedback
I agree with all points raised in the existing review and want to second them explicitly.
KeyScreen removal: The DJI API key is a build-time artifact injected into the
manifest from local.properties. Exposing it in a runtime UI screen adds no value and
the explanation on that screen ("modify values/keys.xml") is outdated since the api-key
moved to local.properties. A status indicator on the main screen is sufficient.
Addresses instead of IPs: Storing "192.168.1.220" without a port is not enough.
Both MAVLink (default port 14550) and WebRTC signaling require a host and port, and the
existing code that consumes these values presumably needs both. Storing a full address
string (e.g. "192.168.1.220:14550") also leaves the door open for future transport
protocol support such as TCP:IP:PORT without requiring a schema migration.
The AddressScreen labels, placeholders, and field names should be updated to reflect
this.
Settings restructuring: Splitting the combined IP screen into separate MAVLink protocol and WebRTC streaming entries makes the configuration hierarchy clearer and
aligns settings with the service they control. This is a meaningful UX improvement worth
addressing before merge.
PrefsManager rename and relocation: Moving to WenuLinkPreferences in the base
org.WenuLink package makes sense. The preference keys should use per-protocol prefixes
(e.g. mavlink.gcs_addr, webrtc.signal_addr) so future additions are unambiguous.
Additional concerns
Unacknowledged regression from develop: camera subsystem and command processor deleted
The patch deletes the following files entirely:
adapters/camera/CameraHandler.ktadapters/camera/CameraCommands.ktadapters/camera/CameraData.ktcontrollers/CameraController.ktadapters/commands/CommandProcessor.ktadapters/commands/ICommand.ktadapters/commands/IHandler.kt
It also strips most of CameraManager (firmware retrieval, mode switching, capture
methods), removes TelemetryMapper, MAVLinkTelemetryData, Quaternion,
OrientationUtils, and MessageUtils.toShortArray.
These files were introduced or substantially modified in the PRs immediately preceding
this one. If this is a deliberate architectural decision it should be stated explicitly in the
description. If it is not intentional, the branch needs a careful review of what has been
dropped relative to develop before this can be approved.
Could you confirm whether these deletions are intentional and, if so, add a note to the
PR description explaining the rationale?
canRunService is derived from string-matching a display field
val canRunService = sdkStatus.contains("Connected")sdkStatus is a human-readable LiveData<String> intended for display purposes.
Deriving a behavior gate from a substring of a display string is fragile: any rewording
of that status message silently breaks the button without a compiler error.
HomeViewModel should expose a dedicated StateFlow<Boolean> or LiveData<Boolean> for
this state, and AppNavigation should observe that instead.
TelemetryData.altitude semantic change may be incorrect in SimManager
Previously:
val altitude: Float = takeOffAltitude + relativeAltitudeNow altitude is a direct field set in SimManager to state.positionZ, which in the
DJI simulator represents height above the takeoff point, not an absolute MSL altitude.
The old computation added takeOffAltitude as the absolute base. If any downstream
MAVLink message consumer (for example MSG_GLOBAL_POSITION_INT.alt) expects MSL
altitude, simulation telemetry will now be wrong.
BootCommand dispatched in AircraftHandler.init before registerHandlerScope
init {
dispatchCommand(BootCommand(5000))
}The command processor is not started until registerHandlerScope is called. Because the
channel has Channel.UNLIMITED capacity the command is silently queued rather than
failing, but whether it is eventually processed depends entirely on
registerHandlerScope being called later. This is the same structural concern that was
raised during the review of the command processor PR. Is this the intended resolution, or
was it carried over unintentionally?
Required fixes before merge
Dependencies hardcoded in build.gradle.kts instead of libs.versions.toml
implementation("androidx.navigation:navigation-compose:2.9.7")
implementation("androidx.compose.material:material-icons-extended:1.7.8")The project manages all dependency versions through the version catalog. Please move
these into libs.versions.toml and reference them via libs.* aliases, consistent with
every other dependency in the file.
Run the linter before committing
Nearly all new files are missing a final newline, many files use wildcard
imports (import org.WenuLink.ui.screens.config.*), and other linter violations.
Please run gradlew ktlintFormat again.
Android Studio can help with the imports automatically:
Click on a package or file and press Ctrl+Alt+O / Cmd+Alt+O, this will expand wildcards
and remove unused imports.
Minor suggestions
ThemeOptionItem, IpFieldItem, and ContributorItem should be private.
They are only used within their own files and exposing them as public top-level
composables leaks internal UI structure unnecessarily.
Dead branch in MenuScreen icon dispatch. There is a "Flight Logs" case in the
when expression that can never be reached because that string is not in the items list.
More generally, deriving icons from display strings is fragile for the same reason as
above. Attaching the icon to each item definition directly would be cleaner.
AboutScreen top bar uses the wrong TopAppBarColors factory. It calls
TopAppBarDefaults.centerAlignedTopAppBarColors() on a TopAppBar, not a
CenterAlignedTopAppBar. The correct factory for TopAppBar is
TopAppBarDefaults.topAppBarColors(), consistent with the other screens.
| "Angel Ayala Maldonado" to "PhD in Computational Science / Co-advisor at DeltaV Drones Team (POLI-UPE)", | ||
| "Max Johenneken" to "Researcher in UAV Systems (H-BRS)", | ||
| "Eliton Sena de Souza" to "Operational Leader at DeltaV Drones Team (POLI-UPE)", | ||
| "Timon Schreiber" to "Developer (Placeholder)" // TODO: add description |
There was a problem hiding this comment.
| "Timon Schreiber" to "Developer (Placeholder)" // TODO: add description | |
| "Timon Schreiber" to "Computer Science Student (H-BRS)" |
| "Angel Ayala Maldonado" to "PhD in Computational Science / Co-advisor at DeltaV Drones Team (POLI-UPE)", | ||
| "Max Johenneken" to "Researcher in UAV Systems (H-BRS)", | ||
| "Eliton Sena de Souza" to "Operational Leader at DeltaV Drones Team (POLI-UPE)", | ||
| "Timon Schreiber" to "Developer (Placeholder)" // TODO: add description |
There was a problem hiding this comment.
Could you also add the same to teh README.md file please.
Thanks @ElitonSena , such these changes fix #4 and are still work in progress right? We can help you somehow, if you can describe here what is needed or how to address the implementation possibly with @TimonSchreiber can apply some changes.
I will do a review in terms of the architecture and the new preferences related classes.
@TimonSchreiber can you do a review and check if last architectural changes are met?
This new interface is really good, I already saw it and is definitively a really good enhancement in terms of usability. I'm wondering about how such checks and flags that were addressed lately can be included or linked to the current views.