Repository navigation
Add configurable device aliases to Qt dropdowns - #1768
Conversation
79ed498 to
89ec3bf
Compare
89ec3bf to
d7f1be7
Compare
|
Added documentation in PR PokemonAutomation/pokemonautomation.github.io#114 |
d7f1be7 to
ded432b
Compare
ded432b to
94ad32d
Compare
pa-devbot
left a comment
There was a problem hiding this comment.
AI review: ## Summary
Nice feature — configurable aliases for the camera/audio dropdowns. I verified the following on this branch (aarch64 Linux, same toolchain as CI):
- Full build of SerialPrograms succeeds.
--command-line-test-mode: 20,325 passed, 0 failed, 3 skipped.
Things I checked that look good:
- Aliases are presentation-only: both
AudioSourceDescriptor_DeviceandVideoSourceDescriptor_Cameraserializedevice_name()(the device ID), not the display name, so changing an alias can't break settings round-trips, and dropdown selection matching usesoperator==on the ID. - The
AudioDeviceInfo::display_name()signature change (const std::string&→std::string) is safe; I checked all callers (AudioSource_Device.cpp,AudioSession.cpp,AudioSelectorWidget.cpp) — none store the reference. - No recursive-initialization hazard:
GlobalSettings's constructor doesn't enumerate cameras/audio, sodisplay_name()→GlobalSettings::instance()can't re-enter mid-construction.GlobalSettings::instance()is a function-local static, so calling it from worker-thread logging paths is safe. DeviceAliasOptionsoverridingto_json()/load_json()to serialize the raw table array is fine:GlobalSettings(aBatchOption) saves/loads it by theDEVICE_ALIASESkey, andenable_saveload=falseonly hides the table's Load/Save/Restore-Defaults buttons.- The
EnumDropdownCell+StringCellrow pattern matches existing tables (e.g.PokemonLA_CustomPathTable), and the branch is 1 commit behind main (an unrelated module), so it should merge cleanly.
Minor points below; none are blockers.
| if (!alias.empty()){ | ||
| result = std::move(alias); | ||
| } | ||
| return true; |
There was a problem hiding this comment.
AI review: Minor: the lookup stops at the first row matching (category, device ID), even when that row's alias is empty. If the settings JSON ever ends up with two rows for the same device (only possible by hand-editing the JSON, since add_device() deduplicates), an empty-alias row earlier in the table would shadow a later row with a real alias. Consider only returning true (stopping) when a non-empty alias was found, i.e. keep scanning while result is still empty.
| GlobalSettingsPanel::GlobalSettingsPanel() | ||
| : settings(GlobalSettings::instance()) | ||
| { | ||
| settings.DEVICE_ALIASES->add_connected_devices(); |
There was a problem hiding this comment.
AI review: Note: this runs synchronously on the GUI thread every time Global Settings is opened. AudioDeviceInfo::all_input_devices() probes every input device's supported formats (and get_all_cameras() enumerates the video backend), so on a machine with many virtual audio devices or a stuck USB camera this can stall the UI for several seconds while the panel is being constructed. Consider deferring it (e.g. a one-shot QTimer::singleShot after the panel is shown) or enumerating on a worker thread.
There was a problem hiding this comment.
This one is fine I think. We already have this delay when loading any program. If it becomes a problem in the future, we can add an async refresher like what the video sources already do.
| } | ||
| } | ||
|
|
||
| void DeviceDropdownAliases::add_connected_devices(){ |
There was a problem hiding this comment.
AI review: Design question: auto-added rows are persisted into the settings JSON (via GlobalSettings::to_json()) and never pruned, so over time the table accumulates rows for devices that are no longer connected (the table is collapsed by default past 10 rows, so this is mostly invisible). A stale alias could also apply to a different device that later gets the same ID (device IDs from Qt/ALSA are not guaranteed stable across reboots or when other devices are plugged in). It may be worth pruning rows for devices that aren't currently connected when refreshing here (keeping user-edited rows with aliases?), or at least mentioning the persistence in the table label.
94ad32d to
e8830e7
Compare
I made this for my fork and thought about sharing this