Skip to content

feat: Add automatic external display switching - #133

Closed
JuanDelPueblo wants to merge 13 commits into
OpenGamingCollective:mainfrom
JuanDelPueblo:external-display-auto-switch
Closed

feat: Add automatic external display switching#133
JuanDelPueblo wants to merge 13 commits into
OpenGamingCollective:mainfrom
JuanDelPueblo:external-display-auto-switch

Conversation

@JuanDelPueblo

@JuanDelPueblo JuanDelPueblo commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Description

Add configurable automatic GPU handling for external displays connected through dGPU-owned DRM connectors.

The daemon discovers connector ownership, monitors display topology changes through udev, and remembers the current mode and per-GPU state. Manual mode stays active and temporarily unblocks the required GPU; other modes switch to Hybrid. The previous state is restored after disconnect unless the user changes it manually.

The feature is disabled by default and exposed through D-Bus, the CLI, the GUI, and the NixOS module. External-display handling takes priority over battery auto-switching.

Related to #7.

TODO

  • Implement connector discovery and external-display switching
  • Restore the previous mode and GPU state after disconnect
  • Expose configuration through D-Bus, CLI, GUI, and NixOS
  • Add tests and update the mdBook documentation

Checklist:

  • My code follows the style guidelines of this project (cargo fmt)
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the mdBook documentation
  • My changes generate no new warnings (clippy/clang)
  • New and existing unit tests pass locally with my changes

Summary by CodeRabbit

  • New Features
    • Added optional automatic switching for external displays, disabled by default.
    • Automatically makes required GPUs available and selects Hybrid mode when compatible displays connect.
    • Restores previous GPU and display mode settings after disconnection.
    • Added GUI toggle, CLI configuration command, and NixOS configuration option.
  • Bug Fixes
    • Improved GPU and display detection, including more reliable connector matching.
    • Prevented automatic GPU blocking when needed by an external display.
  • Documentation
    • Documented configuration, behavior, and restoration rules.

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The change adds external-display auto-switching. The daemon detects connected external displays, changes GPU and mode state, restores prior state after disconnection, and exposes the setting through D-Bus, CLI, GUI, Nix, and documentation.

Changes

External display auto-switching

Layer / File(s) Summary
Display discovery and DRM matching
crates/cardwire-daemon/src/core/gpu/*, crates/cardwire-daemon/src/core/inode.rs
The daemon classifies DRM connectors, finds connected external-display cards, ranks GPUs, and matches DRM entries exactly.
Configuration and mode transitions
crates/cardwire-daemon/src/file/config.rs, crates/cardwire-daemon/src/interface/{config,mode,gpu,debug}.rs
Configuration stores the setting. D-Bus updates persist it. ModeInterface applies Hybrid or restore modes with serialized transitions, rollback, and startup fallback.
Display monitoring and daemon wiring
crates/cardwire-daemon/src/tasks/*, crates/cardwire-daemon/src/models.rs, crates/cardwire-daemon/src/daemon.rs
A background monitor processes udev events, debounces topology changes, retries failures, and delays or cancels restoration. Battery monitoring uses the shared mode interface.
CLI configuration commands
crates/cardwire-cli/src/{args,completion,dbus,main}.rs
The CLI reads and updates external-display switching through D-Bus and provides boolean completions.
GUI, Nix, and documentation surfaces
crates/cardwire-gui/src/*, nix/nixos-module.nix, docs/development/dbus.md, docs/getting-started/*
The GUI adds the setting control and D-Bus subscriptions. Nix and documentation describe the setting and its default.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Display as External display
  participant Monitor as Display monitor
  participant Mode as ModeInterface
  participant GPU as GpuInterface
  Display->>Monitor: topology change
  Monitor->>Mode: reconcile connection state
  Mode->>GPU: update required and block state
  Mode-->>Monitor: apply Hybrid or restore mode
Loading

Possibly related PRs

Suggested reviewers: luytan

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.83% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main feature: automatic external display switching.
Description check ✅ Passed The description summarizes the feature, motivation, issue reference, implementation tasks, documentation, tests, and completed checklist.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@JuanDelPueblo
JuanDelPueblo force-pushed the external-display-auto-switch branch from 41163fe to c5ed665 Compare August 1, 2026 17:18
@JuanDelPueblo
JuanDelPueblo marked this pull request as ready for review August 1, 2026 17:52

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/cardwire-cli/src/args.rs`:
- Line 114: Update the automatic Hybrid mode descriptions to explicitly say they
apply to dGPU-owned external displays, not internal displays: change the command
description in crates/cardwire-cli/src/args.rs at lines 114-114 and the matching
completion description in crates/cardwire-cli/src/completion.rs at lines 31-31.

In `@crates/cardwire-daemon/src/core/gpu/discover.rs`:
- Line 372: Update the default-GPU selection using max_by_key in the discovery
flow to include the GPU id as a secondary tie-break after
default_gpu_rank(stats). Ensure equal-ranked GPUs consistently select the same
highest or lowest GPU id according to the intended ordering, while preserving
the existing primary rank ordering.

In `@crates/cardwire-daemon/src/interface/config.rs`:
- Around line 148-168: Update set_external_display_auto_switch to avoid leaving
runtime state changed when save_to_file or ObjectServer::interface fails:
resolve the object-server interface before invoking
external_display_setting_changed, and restore the previous mode state if
persistence fails. Preserve the existing error propagation and mode-change
emission behavior for successful updates.

In `@crates/cardwire-daemon/src/interface/mode.rs`:
- Around line 233-246: Update apply_at_startup in
crates/cardwire-daemon/src/interface/mode.rs (lines 233-246) to treat
required_external_cards failures as no external display: log a warning, use
current_mode_value(), and continue without returning the topology error; only
propagate set_mode_value failures. In crates/cardwire-daemon/src/models.rs
(lines 136-141), keep apply_startup_fallback limited to mode-application
failures so topology read errors do not overwrite the persisted mode.

In `@crates/cardwire-daemon/src/tasks/monitor_display.rs`:
- Around line 188-245: Wrap the existing monitor lifecycle in a supervising loop
so transient setup or readiness errors do not terminate display monitoring.
Rename the current body to run_display_monitor, accepting references to mode and
mode_interface, and have monitor_display_changes repeatedly call it, log
failures, wait using a fixed restart backoff, then retry; preserve the existing
event and reconciliation logic inside the worker.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 35b1a3f2-8b8b-4a1e-8616-ebe6443769b4

📥 Commits

Reviewing files that changed from the base of the PR and between cdb4ac9 and 167b479.

📒 Files selected for processing (24)
  • crates/cardwire-cli/src/args.rs
  • crates/cardwire-cli/src/completion.rs
  • crates/cardwire-cli/src/dbus.rs
  • crates/cardwire-cli/src/main.rs
  • crates/cardwire-daemon/src/core/gpu/discover.rs
  • crates/cardwire-daemon/src/core/gpu/mod.rs
  • crates/cardwire-daemon/src/core/inode.rs
  • crates/cardwire-daemon/src/daemon.rs
  • crates/cardwire-daemon/src/file/config.rs
  • crates/cardwire-daemon/src/interface/config.rs
  • crates/cardwire-daemon/src/interface/mode.rs
  • crates/cardwire-daemon/src/models.rs
  • crates/cardwire-daemon/src/tasks/mod.rs
  • crates/cardwire-daemon/src/tasks/monitor_display.rs
  • crates/cardwire-gui/src/app.rs
  • crates/cardwire-gui/src/helpers/dbus.rs
  • crates/cardwire-gui/src/message.rs
  • crates/cardwire-gui/src/models.rs
  • crates/cardwire-gui/src/subscription.rs
  • crates/cardwire-gui/src/ui.rs
  • docs/development/dbus.md
  • docs/getting-started/installation.md
  • docs/getting-started/usage.md
  • nix/nixos-module.nix

Comment thread crates/cardwire-cli/src/args.rs Outdated
Comment thread crates/cardwire-daemon/src/core/gpu/discover.rs Outdated
Comment thread crates/cardwire-daemon/src/interface/config.rs
Comment thread crates/cardwire-daemon/src/interface/mode.rs
Comment thread crates/cardwire-daemon/src/tasks/monitor_display.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/cardwire-daemon/src/interface/config.rs (1)

148-181: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift

Snapshot previous_mode and previous_auto_switch before the lock: race with concurrent mode changes.

previous_mode (Line 158) and previous_auto_switch (Line 159-162) are read before any lock is held. external_display_setting_changed (mode.rs) only acquires transition_lock after this snapshot. If a concurrent mode transition (for example a client-driven Mode property write, or the display monitor calling apply_external_display_mode) completes between this snapshot and the internal lock acquisition, and save_to_file then fails, the rollback at Line 167-175 reverts to the stale previous_mode, silently discarding the concurrent transition's result.

Capture the previous mode atomically as part of the same locked operation in ModeInterface, and have it returned to the caller, instead of reading it here before the lock.

As per the concurrency-issue guidance for TOCTOU races, this needs a fix that closes the pre-lock read window.

🛠️ Proposed fix: capture previous mode inside the lock
-    pub async fn external_display_setting_changed(&self, enabled: bool) -> fdo::Result<bool> {
+    pub async fn external_display_setting_changed(&self, enabled: bool) -> fdo::Result<(bool, Modes)> {
         let _transition = self.transition_lock.lock().await;
+        let previous_mode = self.current_mode_value().await;
         let was_enabled = self.external_display_auto_switch_enabled();
         self.config
             .external_display_auto_switch
             .store(enabled, Ordering::Relaxed);
         if !enabled || was_enabled {
-            return Ok(false);
+            return Ok((false, previous_mode));
         }

         let res = async {
             let cards = self.required_external_cards().await?;
             let connected = !cards.is_empty();
             let mode = self.external_display_target_mode(connected).await?;
             let changed = self.set_mode_value_locked(mode, false).await?;
             if connected && changed {
                 Self::notify_drm_change(&cards).await;
             }
             Ok(changed)
         }
         .await;

-        if res.is_err() {
-            self.config
-                .external_display_auto_switch
-                .store(was_enabled, Ordering::Relaxed);
+        match &res {
+            Ok(changed) => return Ok((*changed, previous_mode)),
+            Err(_) => {
+                self.config
+                    .external_display_auto_switch
+                    .store(was_enabled, Ordering::Relaxed);
+            }
         }

-        res
+        res.map(|changed| (changed, previous_mode))
     }
     pub async fn set_external_display_auto_switch(
         &self,
         state: bool,
         #[zbus(object_server)] object_server: &ObjectServer,
     ) -> fdo::Result<()> {
         let interface = object_server
             .interface::<_, ModeInterface>("/com/github/opengamingcollective/cardwire")
             .await
             .map_err(|err| fdo::Error::Failed(err.to_string()))?;
-        let previous_mode = self.mode_interface.current_mode_value().await;
         let previous_auto_switch = self
             .config
             .external_display_auto_switch
             .load(Ordering::Relaxed);
-        let changed = self
+        let (changed, previous_mode) = self
             .mode_interface
             .external_display_setting_changed(state)
             .await?;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/cardwire-daemon/src/interface/config.rs` around lines 148 - 181,
Update ModeInterface::external_display_setting_changed to acquire
transition_lock before capturing the current mode, and return the atomically
captured previous mode alongside its changed result. In
set_external_display_auto_switch, remove the pre-lock current_mode_value
snapshot and use the mode returned by external_display_setting_changed for
rollback, while preserving the existing save failure restoration behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/cardwire-daemon/src/interface/config.rs`:
- Around line 148-181: Update ModeInterface::external_display_setting_changed to
acquire transition_lock before capturing the current mode, and return the
atomically captured previous mode alongside its changed result. In
set_external_display_auto_switch, remove the pre-lock current_mode_value
snapshot and use the mode returned by external_display_setting_changed for
rollback, while preserving the existing save failure restoration behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 61d8d940-3bf7-42d7-b1fd-36cfd89a8004

📥 Commits

Reviewing files that changed from the base of the PR and between 167b479 and 1bb175d.

📒 Files selected for processing (6)
  • crates/cardwire-cli/src/args.rs
  • crates/cardwire-cli/src/completion.rs
  • crates/cardwire-daemon/src/core/gpu/discover.rs
  • crates/cardwire-daemon/src/interface/config.rs
  • crates/cardwire-daemon/src/interface/mode.rs
  • crates/cardwire-daemon/src/tasks/monitor_display.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
crates/cardwire-daemon/src/interface/config.rs (2)

185-191: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Restore the previous mode after a failed save.

If save_to_file returns an error, this setter has already changed external_display_auto_switch_mode. A later display disconnect can use the unpersisted mode, while a daemon restart loads the old mode. Restore the previous value before returning the error.

🛠️ Proposed fix
 pub async fn set_external_display_auto_switch_mode(&self, mode: u32) -> fdo::Result<()> {
     Modes::try_from(mode).map_err(|err| fdo::Error::InvalidArgs(err.to_string()))?;
+    let previous_mode = self
+        .config
+        .external_display_auto_switch_mode
+        .load(Ordering::Relaxed);
     self.config
         .external_display_auto_switch_mode
         .store(mode, Ordering::Relaxed);
-    self.save_to_file().await
+    if let Err(err) = self.save_to_file().await {
+        self.config
+            .external_display_auto_switch_mode
+            .store(previous_mode, Ordering::Relaxed);
+        return Err(err);
+    }
+    Ok(())
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/cardwire-daemon/src/interface/config.rs` around lines 185 - 191,
Update set_external_display_auto_switch_mode to capture the current
external_display_auto_switch_mode before storing the new validated mode, then
restore that previous value if save_to_file returns an error before propagating
the failure. Preserve the existing validation and successful-save behavior.

163-166: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Cast a transactional rollback around external_display_setting_changed. The setting is restored on its internal error path, but apply_mode can change GPU or BPF state before a later operation fails. mode_state is saved only after apply_mode succeeds, and ? prevents caller rollback. Restore previous_mode and all affected state before returning the error.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/cardwire-daemon/src/interface/config.rs` around lines 163 - 166,
Update the apply flow around external_display_setting_changed so failures roll
back transactionally: preserve the pre-apply mode and affected GPU/BPF state
before apply_mode, restore them when that operation or a later step returns an
error, and only commit mode_state after the entire sequence succeeds. Ensure the
error is still propagated after rollback rather than allowing ? to bypass
restoration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/cardwire-daemon/src/interface/config.rs`:
- Around line 172-175: Update the rollback handling around
mode_interface.set_mode_value in the save_to_file failure path so restoration
failures are not discarded. Preserve the original save error while propagating
or clearly reporting when restoring previous_mode fails, ensuring callers can
detect that rollback did not complete.

---

Outside diff comments:
In `@crates/cardwire-daemon/src/interface/config.rs`:
- Around line 185-191: Update set_external_display_auto_switch_mode to capture
the current external_display_auto_switch_mode before storing the new validated
mode, then restore that previous value if save_to_file returns an error before
propagating the failure. Preserve the existing validation and successful-save
behavior.
- Around line 163-166: Update the apply flow around
external_display_setting_changed so failures roll back transactionally: preserve
the pre-apply mode and affected GPU/BPF state before apply_mode, restore them
when that operation or a later step returns an error, and only commit mode_state
after the entire sequence succeeds. Ensure the error is still propagated after
rollback rather than allowing ? to bypass restoration.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 743c32d6-6905-40b7-b3fe-49bd2a15fb81

📥 Commits

Reviewing files that changed from the base of the PR and between 1bb175d and 2277846.

📒 Files selected for processing (1)
  • crates/cardwire-daemon/src/interface/config.rs

Comment thread crates/cardwire-daemon/src/interface/config.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/cardwire-daemon/src/interface/config.rs`:
- Around line 179-181: Update the !state branch in the setting handler to
restore the external display snapshot rather than call
cancel_external_display_snapshot, preserving the user’s prior mode and per-GPU
state when disabling the setting. Use the existing snapshot restoration method
on mode_interface and leave the enabled path unchanged.

In `@crates/cardwire-daemon/src/interface/mode.rs`:
- Around line 212-243: Fix the lock-order inversion between refresh_gpu and
set_required_cards_locked: avoid awaiting block_gpu or unblock_gpu while holding
gpu_list.write(), since those operations acquire pci_list.read() after
refresh_gpu acquires pci_list.write() and then gpu_list.write(). Restructure the
set_required_cards_locked flow to determine and record required GPU updates
under the GPU lock, release that lock, then perform blocking/unblocking
operations in the established pci_list-before-gpu_list order while preserving
the existing state and restore_departed behavior.
- Around line 535-544: Update the mode-change flow around set_mode_value_locked
and external_display_auto_switch_enabled so explicit non-manual changes
(Integrated, Smart, and Hybrid) also restore external-display requirements when
a dGPU display is connected. Preserve the existing manual-mode behavior, and
ensure external_display_required is reapplied before reconciliation can be
skipped due to last_connected being true.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b3e70897-dc35-4bb5-bfe8-409fe701b1eb

📥 Commits

Reviewing files that changed from the base of the PR and between 2277846 and b1828d2.

📒 Files selected for processing (23)
  • crates/cardwire-cli/src/args.rs
  • crates/cardwire-cli/src/completion.rs
  • crates/cardwire-cli/src/dbus.rs
  • crates/cardwire-cli/src/main.rs
  • crates/cardwire-daemon/src/daemon.rs
  • crates/cardwire-daemon/src/file/config.rs
  • crates/cardwire-daemon/src/interface/config.rs
  • crates/cardwire-daemon/src/interface/debug.rs
  • crates/cardwire-daemon/src/interface/gpu.rs
  • crates/cardwire-daemon/src/interface/mode.rs
  • crates/cardwire-daemon/src/models.rs
  • crates/cardwire-daemon/src/tasks/battery_switch.rs
  • crates/cardwire-daemon/src/tasks/monitor_display.rs
  • crates/cardwire-gui/src/app.rs
  • crates/cardwire-gui/src/helpers/dbus.rs
  • crates/cardwire-gui/src/message.rs
  • crates/cardwire-gui/src/models.rs
  • crates/cardwire-gui/src/subscription.rs
  • crates/cardwire-gui/src/ui.rs
  • docs/development/dbus.md
  • docs/getting-started/installation.md
  • docs/getting-started/usage.md
  • nix/nixos-module.nix
💤 Files with no reviewable changes (7)
  • crates/cardwire-cli/src/main.rs
  • crates/cardwire-gui/src/message.rs
  • crates/cardwire-gui/src/app.rs
  • crates/cardwire-gui/src/models.rs
  • crates/cardwire-gui/src/subscription.rs
  • crates/cardwire-cli/src/dbus.rs
  • docs/getting-started/installation.md

Comment thread crates/cardwire-daemon/src/interface/config.rs Outdated
Comment thread crates/cardwire-daemon/src/interface/mode.rs
Comment thread crates/cardwire-daemon/src/interface/mode.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
crates/cardwire-daemon/src/interface/mode.rs (2)

568-582: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

A stumble in bookkeeping turns "Integrated" into "Hybrid" forever.

When set_mode applies a non-Manual mode (Integrated/Smart) while a required external display is connected, it calls apply_external_display_cards_locked(&cards, false) at Line 577. capture_snapshot=false skips capture_external_display_snapshot_locked, so latest_mode stays None. Inside that call, target = Hybrid gets persisted through set_mode_value_locked(target, false), overwriting the user's just-saved "Integrated" choice in mode.json with "Hybrid," with no snapshot to restore from once the display disconnects. clear_external_display_snapshot_locked() at Line 573 also wipes any prior snapshot before the reapply, so an existing snapshot cannot survive this call either.

The same conjuring happens at startup: apply_at_startup (Line 423) calls the identical apply_external_display_cards_locked(&cards, false). If the persisted mode is Integrated/Smart and the display is already connected at boot, the daemon silently rewrites the on-disk mode to Hybrid every time it starts, with no way back.

Capture a snapshot of the just-applied mode before handing control to Hybrid, mirroring the connect-time flow in apply_external_display_mode.

🪄 Proposed fix for `set_mode`
     pub(crate) async fn set_mode(&self, mode: u32) -> fdo::Result<()> {
         // Valide inputs and turn into a Modes
         let mode = Modes::try_from(mode).map_err(|err| fdo::Error::InvalidArgs(err.to_string()))?;
         let _transition = self.transition_lock.lock().await;
         self.set_mode_value_locked(mode, false).await?;
-        self.clear_external_display_snapshot_locked().await;
-        if self.external_display_auto_switch_enabled() {
-            let cards = self.required_external_cards().await?;
-            if !cards.is_empty() {
-                self.apply_external_display_cards_locked(&cards, false)
-                    .await?;
-            }
-        }
+        if self.external_display_auto_switch_enabled() {
+            let cards = self.required_external_cards().await?;
+            if !cards.is_empty() {
+                let capture_snapshot = !self.external_display_override_active_locked().await;
+                self.apply_external_display_cards_locked(&cards, capture_snapshot)
+                    .await?;
+            } else {
+                self.clear_external_display_snapshot_locked().await;
+            }
+        } else {
+            self.clear_external_display_snapshot_locked().await;
+        }
         Ok(())
     }

Apply the same capture_snapshot logic to the apply_at_startup call at Line 423.

Also applies to: 416-431

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/cardwire-daemon/src/interface/mode.rs` around lines 568 - 582, Update
the external-display application flows in set_mode and apply_at_startup to call
apply_external_display_cards_locked with snapshot capture enabled, matching the
connect-time behavior in apply_external_display_mode. Preserve the existing mode
transition and card application logic while ensuring the newly selected
Integrated/Smart mode is snapshotted before Hybrid is applied.

357-393: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Disabling the ward does not release what it was guarding.

external_display_setting_changed's !enabled branch (Lines 367-369) stores false into the config and returns without touching latest_mode or any GPU's external_display_required flag. If the setting was actively protecting a GPU (a display is connected and external_display_required is true) when the user disables the feature, that state never clears. external_display_override_active_locked() keeps returning true afterward, so set_battery_mode_value keeps rejecting battery-triggered mode changes even though external-display auto-switch is now off.

Restore or clear the snapshot when the feature transitions from enabled to disabled.

🪄 Proposed fix
         if !enabled {
+            if was_enabled {
+                if let Err(err) = self.restore_external_display_snapshot_locked().await {
+                    warn!("failed to restore external-display state while disabling: {err}");
+                }
+            }
             return Ok((false, previous_mode));
         }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/cardwire-daemon/src/interface/mode.rs` around lines 357 - 393, Update
external_display_setting_changed so the enabled-to-disabled transition restores
or clears the external-display snapshot before returning, releasing latest_mode
and each GPU’s external_display_required state. Preserve the existing no-op
behavior when already disabled and return the previous mode with the transition
result after cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/cardwire-daemon/src/interface/mode.rs`:
- Around line 568-582: Update the external-display application flows in set_mode
and apply_at_startup to call apply_external_display_cards_locked with snapshot
capture enabled, matching the connect-time behavior in
apply_external_display_mode. Preserve the existing mode transition and card
application logic while ensuring the newly selected Integrated/Smart mode is
snapshotted before Hybrid is applied.
- Around line 357-393: Update external_display_setting_changed so the
enabled-to-disabled transition restores or clears the external-display snapshot
before returning, releasing latest_mode and each GPU’s external_display_required
state. Preserve the existing no-op behavior when already disabled and return the
previous mode with the transition result after cleanup.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c0e4cef3-308d-4275-880c-3912ec7ece84

📥 Commits

Reviewing files that changed from the base of the PR and between b1828d2 and 52b1ea0.

📒 Files selected for processing (1)
  • crates/cardwire-daemon/src/interface/mode.rs

@luytan luytan linked an issue Aug 2, 2026 that may be closed by this pull request

@luytan luytan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split it

Comment on lines +136 to +187
#[zbus(property)]
pub async fn set_external_display_auto_switch(
&self,
state: bool,
#[zbus(object_server)] object_server: &ObjectServer,
) -> fdo::Result<()> {
let interface = object_server
.interface::<_, ModeInterface>("/com/github/opengamingcollective/cardwire")
.await
.map_err(|err| fdo::Error::Failed(err.to_string()))?;
let previous_auto_switch = self
.config
.external_display_auto_switch
.load(Ordering::Relaxed);
let (changed, previous_mode) = self
.mode_interface
.external_display_setting_changed(state)
.await?;
if let Err(err) = self.save_to_file().await {
self.config
.external_display_auto_switch
.store(previous_auto_switch, Ordering::Relaxed);
if state
&& !previous_auto_switch
&& let Err(rollback_err) = self
.mode_interface
.restore_external_display_snapshot()
.await
{
warn!(
"failed to restore display state during config save rollback: {rollback_err}"
);
if changed
&& let Err(mode_err) = self
.mode_interface
.set_mode_value(previous_mode, false)
.await
{
warn!("failed to restore mode value during config save rollback: {mode_err}");
}
}
return Err(err);
}
if !state {
self.mode_interface.cancel_external_display_snapshot().await;
}
self.mode_interface
.emit_mode_change(&interface, changed)
.await
.map_err(|err| fdo::Error::Failed(err.to_string()))?;
Ok(())
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is doing far too much, config.rs should only be used as simple setters/getters for the config, in this case true or false.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, simplified to just a boolean setter/getter. Will now close this PR and split it

Signed-off-by: JuanDelPueblo <edyancruz@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Auto-switch mode upon monitor plug-in

2 participants