bb-imager-ui: Add new UI - #710
Open
Ayush1325 wants to merge 4 commits into
Open
Conversation
Ayush1325
commented
Aug 5, 2026
Ayush1325
commented
Member
- A simplified UI to reduce number of actions required.
- Only has Light theme, so that's a minus.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new shared UI crate (bb-imager-ui) to provide a simplified, unified GUI flow and rewires bb-imager-gui to render these new pages instead of its legacy UI modules.
Changes:
- Add the new
bb-imager-uicrate (pages, shared helpers, assets, and preview examples) and include it in the workspace. - Replace
bb-imager-gui’s in-crate UI implementations with calls intobb-imager-ui, refactoring GUI state to match the new page/state types. - Remove now-unused GUI UI modules and
bb-iced-widgetscanvas widgets, and update board icon URLs inconfig.json.
Reviewed changes
Copilot reviewed 52 out of 66 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds preview-* targets to run bb-imager-ui example pages. |
| config.json | Updates several board icon URLs to point at beagleboard.org-hosted images. |
| Cargo.toml | Adds bb-imager-ui to the workspace members. |
| Cargo.lock | Adds the bb-imager-ui package and links it where needed. |
| bb-imager-ui/Cargo.toml | New crate manifest for the shared UI library. |
| bb-imager-ui/src/lib.rs | Defines the shared UI message enum and centralizes app theming/window settings. |
| bb-imager-ui/src/constants.rs | Adds UI theme colors, font bytes, and embedded icon handles. |
| bb-imager-ui/src/helpers.rs | Shared UI layout helpers + ETA formatting/math (with tests). |
| bb-imager-ui/src/board_selection.rs | Device/board selection page implementation. |
| bb-imager-ui/src/img_selection.rs | OS/image selection page implementation. |
| bb-imager-ui/src/dest_selection.rs | Destination selection page implementation. |
| bb-imager-ui/src/customization.rs | Customization page implementation (sysconfig/cloud-init). |
| bb-imager-ui/src/review.rs | Review page implementation. |
| bb-imager-ui/src/flashing.rs | Flashing progress page implementation. |
| bb-imager-ui/src/flash_success.rs | Flash-success page implementation. |
| bb-imager-ui/src/flash_fail.rs | Flash-failure page implementation. |
| bb-imager-ui/src/flash_cancel.rs | Flash-cancel page implementation. |
| bb-imager-ui/src/app_options.rs | App options/info page implementation. |
| bb-imager-ui/examples/board_selection/main.rs | Preview example for board selection. |
| bb-imager-ui/examples/img_selection/main.rs | Preview example for image selection (with icon downloads). |
| bb-imager-ui/examples/dest_selection/main.rs | Preview example for destination selection. |
| bb-imager-ui/examples/customization-sysconfig/main.rs | Preview example for sysconfig customization. |
| bb-imager-ui/examples/customization-cloudinit/main.rs | Preview example for cloud-init customization. |
| bb-imager-ui/examples/review/main.rs | Preview example for review page. |
| bb-imager-ui/examples/flashing/main.rs | Preview example for flashing/progress page. |
| bb-imager-ui/examples/flash-success/main.rs | Preview example for success page. |
| bb-imager-ui/examples/flash-fail/main.rs | Preview example for failure page. |
| bb-imager-ui/examples/flash-cancel/main.rs | Preview example for cancel page. |
| bb-imager-ui/examples/app-options/main.rs | Preview example for app-options page. |
| bb-imager-ui/assets/icons/board.svg | New embedded icon asset for the shared UI. |
| bb-imager-ui/assets/icons/search.svg | New embedded icon asset for the shared UI. |
| bb-imager-ui/assets/icons/format.svg | New embedded icon asset for the shared UI. |
| bb-imager-ui/assets/icons/file-add.svg | New embedded icon asset for the shared UI. |
| bb-imager-ui/assets/icons/file-save.svg | New embedded icon asset for the shared UI. |
| bb-imager-ui/assets/icons/usb.svg | New embedded icon asset for the shared UI. |
| bb-imager-ui/assets/icons/content-copy.svg | New embedded icon asset for the shared UI. |
| bb-imager-ui/assets/icons/arrow-back.svg | New embedded icon asset for the shared UI. |
| bb-imager-ui/assets/icons/beagleboard-logo.svg | New embedded logo asset for the shared UI. |
| bb-imager-ui/assets/icons/icon.png | New window icon (Git LFS) for the shared UI. |
| bb-imager-ui/assets/fonts/Nunito-Regular-subset.ttf | New font asset (Git LFS) for the shared UI. |
| bb-imager-ui/assets/fonts/Nunito-Bold-subset.ttf | New font asset (Git LFS) for the shared UI. |
| bb-imager-gui/Cargo.toml | Adds dependency on bb-imager-ui. |
| bb-imager-gui/src/main.rs | Switches GUI startup to wrap the app with bb_imager_ui::application. |
| bb-imager-gui/src/ui/mod.rs | Replaces old per-page UI modules with bb-imager-ui page render calls. |
| bb-imager-gui/src/state.rs | Refactors GUI state to store bb-imager-ui page state and introduces a flashing context to preserve selections. |
| bb-imager-gui/src/persistance.rs | Adds conversions between persisted config and bb-imager-ui customization types; removes BCF/Zepto customization persistence. |
| bb-imager-gui/src/db/mod.rs | Adjusts DB types/queries to return bb-imager-ui list item structs for boards/images. |
| bb-imager-gui/src/db/tests.rs | Updates tests to match new string/container types and new image ID enums. |
| bb-imager-gui/src/constants.rs | Removes legacy GUI UI/theme/font/icon constants that moved into bb-imager-ui. |
| bb-imager-gui/src/ui/review.rs | Removes legacy review UI implementation (migrated to bb-imager-ui). |
| bb-imager-gui/src/ui/image_selection.rs | Removes legacy image-selection UI implementation (migrated to bb-imager-ui). |
| bb-imager-gui/src/ui/helpers.rs | Removes legacy UI helper layer (migrated to bb-imager-ui). |
| bb-imager-gui/src/ui/flash.rs | Removes legacy flashing UI implementation (migrated to bb-imager-ui). |
| bb-imager-gui/src/ui/flash_finish.rs | Removes legacy finish UI implementation (migrated to bb-imager-ui). |
| bb-imager-gui/src/ui/destination_selection.rs | Removes legacy destination-selection UI implementation (migrated to bb-imager-ui). |
| bb-imager-gui/src/ui/configuration.rs | Removes legacy configuration UI implementation (migrated to bb-imager-ui). |
| bb-imager-gui/src/ui/board_selection.rs | Removes legacy board-selection UI implementation (migrated to bb-imager-ui). |
| bb-imager-gui/src/ui/app_info.rs | Removes legacy app-info UI implementation (migrated to bb-imager-ui app-options). |
| bb-imager-gui/assets/icons/info.svg | Removes legacy asset no longer used by GUI UI. |
| bb-imager-gui/assets/icons/arrow-forward-ios.svg | Removes legacy asset no longer used by GUI UI. |
| bb-iced-widgets/src/lib.rs | Removes exports for canvas-based progress widgets no longer used. |
| bb-iced-widgets/src/progress_circle.rs | Removes unused canvas progress-circle widget implementation. |
| bb-iced-widgets/src/circle_bar.rs | Removes unused canvas circle-bar widget implementation. |
| bb-bmap-parser/tests/builder.rs | Test refactor: introduces Case type alias and reformats builders for readability. |
Suppressed comments (1)
bb-imager-gui/src/persistance.rs:190
- Keymap is dropped when converting
SdSysconfCustomizationintobb_imager_ui::customization::CloudInit(keymap: None). This means a user-selected keymap won’t be restored in the UI and can’t be persisted reliably.
hostname: value.hostname.map(Into::into),
timezone: value.timezone,
// TODO: Fix
keymap: None,
user: value.user.map(|u| (u.username.into(), u.password.into())),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+244
to
+246
| crate::flashing::Progress::Verifying | crate::flashing::Progress::Customizing => { | ||
| Some(Duration::from_secs(1)) | ||
| } |
| .PHONY: debug-gui | ||
| debug-gui: | ||
| $(_DIOXUS_CLI) serve -p bb-imager-gui ${_RUST_ARGS_GUI} --features debug | ||
|
|
Comment on lines
+40
to
+41
| // TODO: Fix | ||
| keymap: None, |
Comment on lines
+75
to
+77
| let icon = iced::window::icon::from_file_data(constants::WINDOW_ICON_BYTES, None).ok(); | ||
| assert!(icon.is_some()); | ||
|
|
Comment on lines
+10
to
+16
| pub struct State { | ||
| pub has_customization: bool, | ||
| pub device_name: Box<str>, | ||
| pub software_name: Box<str>, | ||
| pub storage: (Box<str>, Option<u64>), | ||
| pub modificiations: Box<[&'static str]>, | ||
| pub copy_text: std::sync::Arc<str>, |
Comment on lines
+33
to
+37
| DestinationFilter(bool), | ||
|
|
||
| UpdateCustomizaton(customization::Customization), | ||
| Next, | ||
|
|
Comment on lines
+52
to
+55
| widget::text("Device is ready to be used with your BeagleBoard hardware!") | ||
| .style(widget::text::primary) | ||
| .style(widget::text::primary) | ||
| .font(FONT_BOLD), |
Comment on lines
+43
to
+46
| Progress::Writing(f) => ( | ||
| widget::text(format!("Writing... ({}%)", (f * 100.0) as u8)), | ||
| widget::progress_bar(0.0..=1.0, f), | ||
| ), |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 66 changed files in this pull request and generated no new comments.
Suppressed comments (8)
bb-imager-ui/src/helpers.rs:330
time_remaining_fromreturns a fixed 1s ETA forProgress::Verifying, but this test currently assertsNone, so the unit test will fail andmake test/CI will break. Either the function or this expectation needs to change; updating the assertion to match the current behavior is the smallest fix.
bb-imager-gui/src/persistance.rs:42- Keymap is currently dropped when building
bb_imager_ui::customization::CloudInitfrom persisted config (keymap: Nonewith a TODO). This will cause users’ saved keymap preference to be lost/never pre-populated in the new UI.
.as_ref()
.and_then(|x| x.sysconf.as_ref().and_then(|y| y.timezone)),
// TODO: Fix
keymap: None,
user: value.sd_customization.as_ref().and_then(|x| {
bb-imager-gui/src/persistance.rs:190
- Keymap is currently dropped when converting persisted
SdSysconfCustomizationintobb_imager_ui::customization::CloudInit(keymap: Nonewith a TODO). This prevents round-tripping the keymap field through persistence and will surprise users who previously set it.
hostname: value.hostname.map(Into::into),
timezone: value.timezone,
// TODO: Fix
keymap: None,
user: value.user.map(|u| (u.username.into(), u.password.into())),
bb-imager-ui/src/lib.rs:37
- The message variant is misspelled as
UpdateCustomizaton(missing an "i"). Since this is part of the publicbb_imager_ui::MessageAPI and is used across multiple modules/crates, the typo will spread and be painful to fix later. Consider renaming toUpdateCustomization(optionally leaving a deprecated alias for compatibility).
bb-imager-ui/src/review.rs:16 - Field name
modificiationsis misspelled; becausereview::Stateis part of thebb_imager_uipublic API and the field is used throughout the UI, this typo will leak into more call sites. Renaming tomodifications(and updating all uses) will avoid carrying the typo forward.
bb-imager-ui/src/flash_success.rs:55 - Redundant
.style(widget::text::primary)is applied twice to the samewidget::textcall, which is confusing and makes future edits error-prone.
bb-imager-ui/src/flash_success.rs:16 - Field name
modificiationsis misspelled inflash_success::State. Since this struct is part of thebb_imager_uipublic surface, the typo will propagate to callers; renaming tomodifications(and updating all references) will prevent an API footgun.
Makefile:532 - The help comment documents the target as
preview-image-selection, but the actual make target is namedpreview-img-selection. This mismatch makesmake helpoutput misleading and breaks copy/paste.
## preview: preview-image-selection: Preview Image selection page.
preview-img-selection:
$(_DIOXUS_CLI) serve -p bb-imager-ui --example img_selection --features debug
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 66 changed files in this pull request and generated no new comments.
Suppressed comments (9)
bb-imager-ui/src/lib.rs:36
UpdateCustomizatonis misspelled (missing 'i'). Since this is a core message variant used by the GUI, the typo will be repeated across many match statements and makes the API look unpolished. Consider renaming toUpdateCustomizationand updating all call sites.
bb-imager-gui/src/persistance.rs:189- Keymap persistence is also dropped when converting
SdSysconfCustomization -> bb_imager_ui::customization::CloudInit(keymap: None). This prevents restoring a saved keymap into the UI; resolve the stored string to a'staticentry viaKEYMAP_LAYOUTS.
// TODO: Fix
keymap: None,
bb-imager-ui/src/review.rs:15
- Field name
modificiationsis misspelled. Since this is part of the UI state type, it will propagate throughout the crate/API and make call sites harder to read/search. Consider renaming it tomodificationsand updating all references.
bb-imager-ui/src/helpers.rs:252 time_remaining_fromreturnsSome(1s)forProgress::Verifying, but the tests below expect verifying to have no ETA (and it’s also likely misleading to show a fixed 1s during verify). This currently makes the unit tests fail and produces inconsistent UI behavior.
bb-imager-ui/src/lib.rs:76assert!(icon.is_some())will panic at startup if the window icon bytes cannot be decoded (e.g., missing/incorrect LFS assets). Sinceiced::window::Settings::iconis already optional, it’s safer to allowNonehere and continue running without an icon.
bb-imager-ui/src/flash_success.rs:55- This text has
.style(widget::text::primary)applied twice, which is redundant and makes the styling chain harder to read.
Makefile:530 - The help text says
preview-image-selection, but the actual target ispreview-img-selection. This mismatch makesmake helpoutput misleading.
## preview: preview-image-selection: Preview Image selection page.
bb-imager-gui/src/persistance.rs:41
- Keymap persistence is currently dropped on load (
keymap: Nonewith a TODO), so a previously-saved keymap won’t be restored into the UI. Sincebb-imager-uiexpectsOption<&'static str>, you can map the persistedStringback onto one of theKEYMAP_LAYOUTSentries (which are already'static).
This issue also appears on line 188 of the same file.
// TODO: Fix
keymap: None,
bb-imager-ui/src/flash_success.rs:15
- Field name
modificiationsis misspelled. As a public-ish state struct field, this typo will spread through the codebase and makes usage/searching harder. Consider renaming it tomodificationsand updating all references.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 66 changed files in this pull request and generated 1 comment.
Suppressed comments (13)
bb-imager-gui/src/db/mod.rs:56
rusqlite::Row::getcannot deserialize directly intoBox<str>/Option<Arc<str>>(noFromSqlimpls), soOsImage::from_rowis likely to fail to compile. ReadString/Option<String>and convert.
id: value.get("id")?,
name: value.get("name")?,
init_format: value.get("init_format")?,
info_text: value.get("info_text")?,
url: value.get("url")?,
bb-imager-gui/src/db/mod.rs:474
- Same
rusqlite::Row::getissue here:bb_imager_ui::board_selection::Board.nameis aBox<str>, whichrusqlitewill not deserialize into directly. Convert fromStringinstead.
Ok(bb_imager_ui::board_selection::Board {
id: value.get("id")?,
icon: value.get("icon")?,
name: value.get("name")?,
})
bb-imager-gui/src/db/mod.rs:540
ImageItem.label/descriptionareCow<'static, str>, whichrusqlitecannot deserialize into directly. Deserialize asStringand.into()theCowto avoid a compile error.
id: bb_imager_ui::img_selection::ImageId::OsImage(value.get("id")?),
label: value.get("name")?,
icon: value.get("icon")?,
description: value.get("description")?,
size: Some(size as u64),
bb-imager-gui/src/db/mod.rs:579
- Same
rusqlite::Row::getissue here:ImageItem.label/descriptionareCow<'static, str>, so they need to be built fromStringrather than deserialized directly.
label: value.get("name")?,
icon: value.get("icon")?,
description: value.get("description")?,
size: None,
release_date: None,
bb-imager-gui/src/db/mod.rs:711
- Same
rusqlite::Row::getissue:ImageItem.label/descriptionareCow<'static, str>and should be created fromString(then.into()), otherwise this closure likely won’t compile.
id: bb_imager_ui::img_selection::ImageId::OsImage(value.get("id")?),
label: value.get("name")?,
icon: value.get("icon")?,
description: value.get("description")?,
size: Some(size as u64),
bb-imager-ui/src/helpers.rs:253
time_remaining_fromcurrently returns a fixed 1s ETA forVerifying, but the unit test below assertsVerifyingshould returnNone. This makes the test fail (and likely the UI inconsistent).
bb-imager-ui/src/flashing.rs:46- The
Writing(f32)progress value is used directly for both the percent label andprogress_bar. Iffis ever >1.0 (or <0.0), the percent can become nonsensical and the progress bar may misbehave. Clampfto[0.0, 1.0]before rendering.
bb-imager-ui/src/lib.rs:76 - Asserting the window icon is present will panic the whole app if the icon bytes are missing/corrupt (e.g. bad LFS checkout). Since
Settings.iconis optional, this should degrade gracefully instead of crashing.
bb-imager-ui/src/flash_success.rs:55 - This text has
.style(widget::text::primary)applied twice; the second call is redundant and makes the chain harder to read.
bb-imager-ui/src/review.rs:15 - Field name
modificiationsis misspelled (should bemodifications). Keeping the typo in a public-ish UI state struct makes it easy to propagate and harder to refactor later.
bb-imager-ui/src/lib.rs:35 - Message variant
UpdateCustomizatonis misspelled (should beUpdateCustomization). This typo is now part of the UI message API and will make downstream code harder to read/search.
bb-imager-gui/src/persistance.rs:41 - Keymap persistence is currently dropped (
keymap: None), so saved keymap preferences won’t round-trip throughGuiConfiguration. Since the UI expectsOption<&'static str>, you can recover it by matching the stored string againstconstants::KEYMAP_LAYOUTS.
// TODO: Fix
keymap: None,
bb-imager-ui/src/flash_success.rs:15
- Field name
modificiationsis misspelled (should bemodifications). This is part of the public UI state type and will likely spread to more call sites.
Comment on lines
29
to
33
| id: value.get("id")?, | ||
| name: value.get("name")?, | ||
| icon: value.get("icon")?, | ||
| description: value.get("description")?, | ||
| documentation: value.get("documentation")?, | ||
| specification: serde_json::from_slice(&spec).unwrap(), | ||
| oshw: value.get("oshw")?, | ||
| flasher: value.get("flasher")?, | ||
| instructions: value.get("instructions")?, | ||
| }) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 67 changed files in this pull request and generated no new comments.
Suppressed comments (12)
bb-imager-ui/src/review.rs:15
- Field name
modificiationslooks like a typo; it should likely bemodifications. Since this is a public state struct used across the new UI and GUI integration, fixing it sooner will avoid locking in the misspelling.
bb-imager-ui/src/lib.rs:35 - Message variant
UpdateCustomizatonappears to be misspelled (missing 'i'). This becomes part of the public UI message API, so correcting it now will prevent the typo from spreading further.
bb-imager-gui/src/persistance.rs:190 - Keymap is also dropped when converting stored
SdSysconfCustomizationinto the UICloudInitstate (keymap: None). This prevents the UI from rehydrating the persisted selection.
// TODO: Fix
keymap: None,
user: value.user.map(|u| (u.username.into(), u.password.into())),
bb-imager-ui/src/flashing.rs:46
Progress::Writing(f)passesfdirectly intoprogress_barand the displayed percent. If the upstream progress ever drifts outside 0..=1, the label and bar can become nonsensical (and some widgets assume the value is in-range). Clampfbefore use.
bb-imager-ui/src/lib.rs:78assert!(icon.is_some())will panic without context if icon loading fails. Prefer anexpect(...)with a clear message and passSome(icon)into window settings.
bb-imager-gui/src/persistance.rs:41- Keymap is currently dropped when converting persisted GUI config into the new UI
CloudInitstate (keymap: None). This loses a user setting on app restart, even thoughSdSysconfCustomizationstill stores a keymap string. Consider mapping the stored string back to one of the supported keymap literals.
This issue also appears on line 188 of the same file.
// TODO: Fix
keymap: None,
bb-imager-ui/src/flash_success.rs:55
- The success subheading applies
.style(widget::text::primary)twice, which is redundant and makes future edits noisier.
bb-imager-ui/src/helpers.rs:253 time_remaining_fromcurrently returns a 1s ETA forProgress::Verifying, but the unit test below expects no ETA in verifying. This also diverges from the previous GUI behavior (ETA only for writing/customizing).
bb-imager-ui/src/app_options.rs:136- These
text_inputfields are rendered as editable, but theiron_inputhandler always emitsMessage::Null, so user edits are ignored and the SAVE button cannot meaningfully apply changes. Either wire the inputs to state updates or render them as non-editable text to avoid a confusing UX.
bb-imager-ui/src/flash_fail.rs:37 - UI heading text "Write Fail" is ungrammatical; consider "Write Failed" to match the other status headings (e.g., "Write Complete").
bb-imager-ui/src/customization.rs:46 - This doc comment is now outdated:
SelectableSdis already present inCustomization. Leaving a TODO here is misleading for future readers.
Makefile:528 - These new
preview-*targets are not marked.PHONY. If a file with the same name exists,makemay treat the target as up-to-date and skip running the preview command. Mark the preview targets as phony (as is done for other utility targets likedebug-gui).
## preview: preview-board-selection: Preview Board selection page.
preview-board-selection:
$(_DIOXUS_CLI) serve -p bb-imager-ui --example board_selection --features debug
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #710 +/- ##
==========================================
+ Coverage 35.53% 36.95% +1.41%
==========================================
Files 34 26 -8
Lines 5363 4108 -1255
Branches 5363 4108 -1255
==========================================
- Hits 1906 1518 -388
+ Misses 3349 2471 -878
- Partials 108 119 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 67 changed files in this pull request and generated no new comments.
Suppressed comments (9)
bb-imager-ui/src/review.rs:16
- Field name
modificiationsis misspelled. Because this struct is part of the UI crate’s public surface and the name is used across multiple modules/crates, this typo will leak into downstream code and makes the API harder to use/search.
bb-imager-ui/src/lib.rs:36 UpdateCustomizatonis misspelled (missing an 'i'). Because this is a public enum variant and it’s already used throughout the UI and GUI integration, keeping the typo will permanently bake it into the API.
bb-imager-gui/src/persistance.rs:186- Keymap is also dropped when converting
SdSysconfCustomizationinto the UICloudInitstate (keymap: None). This will silently lose the user’s configured keymap when revisiting customization.
// TODO: Fix
keymap: None,
user: value.user.map(|u| (u.username.into(), u.password.into())),
Makefile:531
- The help/description header doesn’t match the target name: the comment says
preview-image-selectionbut the actual target ispreview-img-selection, which can confusemake helpoutput and users copying the command. Rename one side so they match.
## preview: preview-image-selection: Preview Image selection page.
preview-img-selection:
bb-imager-ui/src/flash_success.rs:16
- Field name
modificiationsis misspelled here as well. Keeping the typo in exported structs will spread it across the codebase and makes the API inconsistent with the UI text (“Modifications Applied”).
bb-imager-ui/src/flash_success.rs:55 - This text widget chains
.style(widget::text::primary)twice. The second call is redundant and makes the intent unclear.
bb-imager-ui/src/lib.rs:78 assert!(icon.is_some())will panic at runtime if the window icon bytes can’t be decoded (e.g., missing/invalid asset or a partial git-lfs checkout). The app should still be able to run without a window icon.
bb-imager-ui/Cargo.toml:22bb-configis listed both as a normal dependency and a dev-dependency. The duplicate entry adds noise and can confuse version/feature resolution when adjusting dependencies later.
[dev-dependencies]
tempfile = "3.27"
bb-config = { path = "../bb-config" }
serde_json = { version = "1.0.150" }
tokio = { version = "1.52" }
bb-downloader = { path = "../bb-downloader" }
bb-imager-gui/src/persistance.rs:41
- Keymap persistence is currently dropped on load (
keymap: None), so users who previously saved a keymap won’t see it reflected in the new UI. This is a functional regression (and the TODO suggests it’s unfinished).
This issue also appears on line 184 of the same file.
// TODO: Fix
keymap: None,
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 68 changed files in this pull request and generated no new comments.
Suppressed comments (12)
bb-imager-gui/src/db/mod.rs:33
rusqlite::Row::getcannot deserialize directly intoBox<str>/Option<Box<str>>because those types do not implementFromSql. This currently makesBoard::from_rowfail to compile. Read intoString/Option<String>first and then convert.
fn from_row(value: &rusqlite::Row<'_>) -> rusqlite::Result<Self> {
Ok(Self {
id: value.get("id")?,
name: value.get("name")?,
flasher: value.get("flasher")?,
instructions: value.get("instructions")?,
})
bb-imager-gui/src/db/mod.rs:60
OsImage::from_rowhas the same issue asBoard::from_row:Row::getcan’t returnBox<str>orOption<Arc<str>>directly because they don’t implementFromSql. Deserialize asString/Option<String>and convert.
impl OsImage {
fn from_row(value: &rusqlite::Row<'_>) -> rusqlite::Result<Self> {
Ok(Self {
id: value.get("id")?,
name: value.get("name")?,
init_format: value.get("init_format")?,
info_text: value.get("info_text")?,
url: value.get("url")?,
image_download_sha256: value.get("image_download_sha256")?,
extract_size: value.get("extract_size")?,
bmap: value.get("bmap")?,
})
bb-imager-gui/src/db/mod.rs:475
board_listconstructsbb_imager_ui::board_selection::Boardwithname: value.get("name")?, butnameisBox<str>and cannot be fetched directly from rusqlite. This will not compile; fetch aStringand convert toBox<str>.
.query_map([format!("%{}%", search)], |value| {
Ok(bb_imager_ui::board_selection::Board {
id: value.get("id")?,
icon: value.get("icon")?,
name: value.get("name")?,
})
})?
bb-imager-gui/src/db/mod.rs:542
os_images_by_board_idbuildsbb_imager_ui::img_selection::ImageItem, butlabel/descriptionareCow<'static, str>, which rusqlite cannot deserialize into directly viaRow::get. Read them asStringand convert with.into()toCow::Owned.
.query_map(rusqlite::params![board_id, parent_id], |value| {
let size: i64 = value.get("extract_size")?;
Ok(bb_imager_ui::img_selection::ImageItem {
id: bb_imager_ui::img_selection::ImageId::OsImage(value.get("id")?),
label: value.get("name")?,
icon: value.get("icon")?,
description: value.get("description")?,
size: Some(size as u64),
release_date: value.get("release_date")?,
})
bb-imager-gui/src/db/mod.rs:581
os_sublistshas the same rusqlite deserialization issue asos_images_by_board_id:label/descriptionareCow<'static, str>and need to be read asStringthen converted.
let res = stmt
.query_map(rusqlite::params![board_id, parent_id], |value| {
Ok(bb_imager_ui::img_selection::ImageItem {
id: bb_imager_ui::img_selection::ImageId::OsSublist((
value.get("id")?,
value.get("flasher")?,
)),
label: value.get("name")?,
icon: value.get("icon")?,
description: value.get("description")?,
size: None,
release_date: None,
})
})?
bb-imager-gui/src/db/mod.rs:714
os_images_by_namerepeats the same issue:label/descriptionareCow<'static, str>inImageItem, sovalue.get("name")?/value.get("description")?won’t compile. FetchStringand convert.
let res = stmt
.query_map(
rusqlite::params![board_id, format!("%{search}%")],
|value| {
let size: i64 = value.get("extract_size")?;
Ok(bb_imager_ui::img_selection::ImageItem {
id: bb_imager_ui::img_selection::ImageId::OsImage(value.get("id")?),
label: value.get("name")?,
icon: value.get("icon")?,
description: value.get("description")?,
size: Some(size as u64),
release_date: value.get("release_date")?,
})
},
bb-imager-ui/src/review.rs:16
- Field name
modificiationsis misspelled (should bemodifications). Since this is part of the UI state type and is used across multiple modules/crates (examples + bb-imager-gui state), the typo will propagate into more call sites and makes the API harder to use.
bb-imager-ui/src/lib.rs:37 UpdateCustomizatonis misspelled (should beUpdateCustomization). Because this enum is a cross-crate message type (used by bb-imager-gui), keeping the typo makes downstream code noisier and harder to read.
bb-imager-gui/src/persistance.rs:43- Keymap is currently dropped when loading GUI config into the new UI (
keymap: Nonewith a TODO). This means a user’s saved keymap selection will never be restored, and it will also be lost when convertingSdSysconfCustomizationintobb_imager_ui::customization::CloudInit. This should be implemented before merging (either by changing the UI keymap type away fromOption<&'static str>or by mapping stored strings back to one of the known keymap options).
impl From<&GuiConfiguration> for bb_imager_ui::customization::CloudInit {
fn from(value: &GuiConfiguration) -> Self {
Self {
hostname: value.sd_customization.as_ref().and_then(|x| {
x.sysconf
.as_ref()
.and_then(|y| y.hostname.clone().map(Into::into))
}),
timezone: value
.sd_customization
.as_ref()
.and_then(|x| x.sysconf.as_ref().and_then(|y| y.timezone)),
// TODO: Fix
keymap: None,
user: value.sd_customization.as_ref().and_then(|x| {
x.sysconf.as_ref().and_then(|y| {
bb-imager-ui/src/app_options.rs:136
- The App Options values are rendered as a
text_inputbut theon_inputhandler ignores changes (Message::Null), so the widget looks editable but is effectively broken/unresponsive. If these fields are meant to be read-only, render them as text (or a non-editable text editor for copy/select) instead of a text input.
bb-imager-ui/src/flash_success.rs:55 - There is a redundant
.style(widget::text::primary)call on the samewidget::text(...)chain, which is dead code and makes the intent harder to read.
Makefile:532 - The help comment and the target name are inconsistent: the
## preview: ...line advertisespreview-image-selection, but the actual target ispreview-img-selection. Sincemake helpappears to parse these## category: target:lines, it will list a non-existent target.
## preview: preview-image-selection: Preview Image selection page.
preview-img-selection:
$(_DIOXUS_CLI) serve -p bb-imager-ui --example img_selection --features debug
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 67 changed files in this pull request and generated no new comments.
Suppressed comments (10)
bb-imager-ui/src/flash_success.rs:58
- Duplicate
.style(widget::text::primary)call is redundant and makes the widget styling harder to read.
bb-imager-ui/src/lib.rs:77 assert!(icon.is_some())will panic without context if the icon fails to load (e.g. missing git-lfs assets). Prefer failing with an actionable message and avoid the separateassert!.
bb-imager-ui/src/flashing.rs:46- Progress values are used directly for the percent label and progress bar. If the backend ever reports values slightly outside [0, 1], the UI can show nonsensical percentages and may violate widget expectations. Clamp before rendering.
bb-imager-gui/src/persistance.rs:41 - Keymap is currently dropped when converting persisted GUI configuration into the UI state (
keymap: None), so a user's saved keymap selection can never be restored.
// TODO: Fix
keymap: None,
bb-imager-ui/src/lib.rs:36
- The message variant is spelled
UpdateCustomizaton(missing an "i"). Since this is part of the publicbb-imager-uiAPI and is re-exported/used bybb-imager-gui, the typo will propagate and be difficult to change later.
bb-imager-ui/src/review.rs:16 - Field name
modificiationsis misspelled. Becausereview::Stateis public API and is also constructed by bb-imager-gui, this typo will be user-facing in downstream code and harder to correct later.
bb-imager-ui/src/customization.rs:28 CloudInit::is_invaliddoes not treat an empty username as invalid, so the UI can enable "NEXT" with a blank username (as long as the password is non-empty). This will later fail or produce unusable images.
bb-imager-ui/src/app_options.rs:119- The text input uses the current value as both placeholder and value, which is confusing and makes empty values render with no hint. The placeholder should be the label (or another static prompt).
bb-imager-gui/src/persistance.rs:185 - Keymap is dropped when converting
SdSysconfCustomizationinto the UICloudInitstate (keymap: None). This prevents keymap from round-tripping through persistence even when it was saved.
// TODO: Fix
keymap: None,
bb-imager-ui/src/flash_success.rs:16
- Field name
modificiationsis misspelled. Sinceflash_success::Stateis public API and constructed by bb-imager-gui, the typo will propagate and make future refactors harder.
- UI now lives in a separate crate. - Allows much faster development/experimentation per page. Signed-off-by: Ayush Singh <ayush@beagleboard.org>
Signed-off-by: Ayush Singh <ayush@beagleboard.org>
No longer used. Signed-off-by: Ayush Singh <ayush@beagleboard.org>
Basic cargo udpate. Removes unused deps. Signed-off-by: Ayush Singh <ayush@beagleboard.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.