fix device image mismatch #3552
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where devices identified as 'Glass' were showing the wrong image. However, the implementation introduces significant code duplication when checking the modelNumber and deviceName. More importantly, it appears to introduce a regression by removing the check for 'DEV' in the device name, which is inconsistent with other parts of the code and may cause some devkit devices to be misidentified. I've provided comments with suggestions to address these issues.
| if (modelNumber != null && modelNumber.isNotEmpty && modelNumber.toUpperCase() != 'UNKNOWN') { | ||
| final upperModel = modelNumber.toUpperCase(); | ||
| if (upperModel.contains('DEVKIT') || upperModel.contains('FRIEND')) { | ||
|
|
||
| if (upperModel.contains('GLASS')) { | ||
| return Assets.images.omiGlass.path; | ||
| } | ||
|
|
||
| if (upperModel.contains('DEVKIT') || (upperModel.contains('FRIEND'))) { | ||
| return Assets.images.omiDevkitWithoutRope.path; | ||
| } | ||
| } | ||
| if (deviceName != null && deviceName.isNotEmpty) { | ||
| final upperName = deviceName.toUpperCase(); | ||
| if (upperName.contains('DEVKIT') || upperName.contains('DEV') || upperName.contains('FRIEND')) { | ||
|
|
||
| if (upperName.contains('GLASS')) { | ||
| return Assets.images.omiGlass.path; | ||
| } | ||
|
|
||
| if (upperName.contains('DEVKIT') || (upperName.contains('FRIEND'))) { | ||
| return Assets.images.omiDevkitWithoutRope.path; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to determine the image path from modelNumber and deviceName is nearly identical and duplicated. Both blocks of code check for 'GLASS', 'DEVKIT', and 'FRIEND'. This duplication makes the code harder to maintain and increases the risk of introducing bugs if one block is updated but the other is not. 1
To improve maintainability, consider extracting this logic into a single private helper function. This would centralize the device image selection logic for DeviceType.omi, making it cleaner and easier to update in the future.
Rules References
Footnotes
-
Prefer using existing helper functions over inlining their logic, especially when they handle complex cases like fallbacks or error handling. Inlining can introduce subtle bugs by missing parts of the original logic. ↩
|
update discussed in tg with Aarav only show the device id when there are multiple same devices. If there’s just one, don’t show the device id.
|
before glass shows the omi cv1 image
before:
after:
closes #3437