feat: autodetect dash-qt binary location#297
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances handling of the dash-qt executable path by adding auto-detection via the which crate and refactors repeated database update calls into a new, reusable save method.
- Introduce
whichdependency and auto-detectdash-qtin$PATHwhen no custom path is set. - Encapsulate database update logic for
dash-coresettings inNetworkChooserScreen::save(). - Replace direct
update_dash_core_execution_settingscalls withsave()for consistency and maintainability.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/app.rs | Use which::which to locate dash-qt as default path |
| src/ui/network_chooser_screen.rs | Extract DB updates into save() and replace direct calls |
| Cargo.toml | Added which = "7.0.3" dependency |
Comments suppressed due to low confidence (3)
src/ui/network_chooser_screen.rs:94
- Add unit tests for
save()to verify that database update logic behaves correctly across various scenarios (e.g., success and failure cases).
fn save(&self) -> Result<(), String> {
src/ui/network_chooser_screen.rs:164
- [nitpick] Consider providing more context in the panic message or handling the error gracefully instead of relying on a generic
expectmessage.
self.save().expect("Expected to save db settings");
src/app.rs:196
- [nitpick] Rename
dash_qttodash_qt_pathfor clarity since it holds the executable path rather than the executable itself.
let dash_qt = which::which("dash-qt")
|
|
||
| /// Save the current settings to the database | ||
| /// | ||
| /// TODO: doesn't save local network settings like password yet. |
There was a problem hiding this comment.
Remove or address the TODO by implementing support for saving local network settings or documenting this limitation explicitly in the method doc.
This pull request introduces several enhancements to improve the handling of
dash-qtexecutable paths and database settings, as well as refactoring for better code maintainability. The most important changes include adding thewhichcrate for locating executables, modifying the logic for determining defaultdash-qtpaths, and refactoring repeated database update calls into a newsavemethod.Dependency Updates:
Cargo.toml: Added thewhichcrate (version = "7.0.3") to enable system PATH searches for executables.Enhancements to
dash-qtPath Handling:src/app.rs: Updated the logic to use thewhichcrate for locating thedash-qtexecutable in the system PATH as a default value when no custom path is provided. This includes error handling with logging for failed searches.Code Refactoring for Database Updates:
src/ui/network_chooser_screen.rs: Introduced a newsavemethod inNetworkChooserScreento encapsulate database update logic fordash-coreexecution settings. This method improves code reuse and readability.src/ui/network_chooser_screen.rs: Replaced direct database update calls with the newsavemethod in multiple places, including file selection validation, clearing the custom path, and toggling theoverwrite dash.confcheckbox. [1] [2]