fix: improve output configuration reliability and error handling#579
Merged
zccrs merged 1 commit intolinuxdeepin:masterfrom Oct 30, 2025
Merged
fix: improve output configuration reliability and error handling#579zccrs merged 1 commit intolinuxdeepin:masterfrom
zccrs merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's GuideRefactors output configuration to an atomic multi-output approach by leveraging WOutputRenderWindow for state changes, extending helper interfaces for granular control, and updating the manager to fully map and queue configurations, ensuring settings are saved only upon successful commits. Sequence diagram for atomic multi-output configuration and commit processsequenceDiagram
participant Helper
participant OutputManager
participant RenderWindow
participant OutputViewport
participant Settings
Helper->>OutputManager: onOutputTestOrApply(config, onlyTest)
OutputManager->>RenderWindow: For each output, set state via WOutputRenderWindow
RenderWindow->>OutputViewport: Apply state changes (mode, scale, transform, etc.)
RenderWindow->>Helper: Notify commit finished (onOutputCommitFinished)
Helper->>Settings: Save settings if all commits succeed
Helper->>OutputManager: sendResult(config, success)
ER diagram for output configuration state mappingerDiagram
WOutputState {
bool enabled
int x
int y
wlr_output_mode* mode
QSize customModeSize
int customModeRefresh
float scale
int transform
bool adaptiveSyncEnabled
}
qw_output_configuration_v1 ||--o{ qw_output_configuration_head_v1 : contains
qw_output_configuration_head_v1 {
wlr_output* output
OutputState state
}
OutputState {
bool enabled
int x
int y
wlr_output_mode* mode
float scale
int transform
bool adaptive_sync_enabled
CustomMode custom_mode
}
CustomMode {
int width
int height
int refresh
}
OutputState o|--|{ CustomMode : has
Class diagram for updated output configuration and helper interfacesclassDiagram
class Helper {
+onOutputTestOrApply(config, onlyTest)
+onOutputCommitFinished(config, success)
-PendingOutputConfig m_pendingOutputConfig
}
class PendingOutputConfig {
+qw_output_configuration_v1 *config
+QList<WOutputState> states
+int pendingCommits
+bool allSuccess
}
class WOutputHelper {
+setScale(float)
+setTransform(Transform)
+setMode(wlr_output_mode*)
+setCustomMode(int32_t, int32_t, int32_t)
+setAdaptiveSyncEnabled(bool)
+setEnabled(bool)
+addCommitListener(CommitListener)
+hasPendingModeChange() const
}
class WOutputRenderWindow {
+setOutputScale(WOutputViewport*, float)
+rotateOutput(WOutputViewport*, Transform)
+setOutputMode(WOutputViewport*, wlr_output_mode*)
+setOutputCustomMode(WOutputViewport*, int32_t, int32_t, int32_t)
+setOutputAdaptiveSync(WOutputViewport*, bool)
+setOutputEnabled(WOutputViewport*, bool)
}
Helper --> "1" PendingOutputConfig
WOutputRenderWindow --> WOutputHelper
Class diagram for updated WOutputManagerV1 configuration handlingclassDiagram
class WOutputManagerV1 {
+updateConfig()
+sendResult(config, ok)
-QList<WOutputState> stateList
-QList<WOutputState> stateListPending
}
class qw_output_configuration_v1 {
+send_succeeded()
+send_failed()
}
WOutputManagerV1 --> qw_output_configuration_v1
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The EBUSY retry paths don’t increment
retryCount, so a busy output could retry indefinitely—consider incrementing and bounding those retries againstMAX_RETRIESas well. - Directly calling
updateConfig()at the end ofsendResult()creates recursive flows that could blow the stack—consider scheduling the next configuration update on the event loop instead of immediate recursion. - The current backoff (RETRY_DELAY_BASE_MS * retryCount) is linear and may not be sufficient under load—consider using exponential backoff with jitter for more robust retry behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The EBUSY retry paths don’t increment `retryCount`, so a busy output could retry indefinitely—consider incrementing and bounding those retries against `MAX_RETRIES` as well.
- Directly calling `updateConfig()` at the end of `sendResult()` creates recursive flows that could blow the stack—consider scheduling the next configuration update on the event loop instead of immediate recursion.
- The current backoff (RETRY_DELAY_BASE_MS * retryCount) is linear and may not be sufficient under load—consider using exponential backoff with jitter for more robust retry behavior.
## Individual Comments
### Comment 1
<location> `waylib/src/server/protocols/woutputmanagerv1.cpp:125-127` </location>
<code_context>
- config->send_succeeded();
- else
+
+ if (!ok && d->retryCount < d->MAX_RETRIES) {
+ d->retryCount++;
+ QTimer::singleShot(d->RETRY_DELAY_BASE_MS * d->retryCount, this, [this, config]() {
+ sendResult(config, true);
+ });
</code_context>
<issue_to_address>
**issue (bug_risk):** Retry logic may cause unexpected behavior due to always sending success after delay.
Currently, the retry always sends a success result without reattempting the operation, which may hide real failures. Please update the logic to retry the actual operation and only send success if it truly succeeds.
</issue_to_address>
### Comment 2
<location> `waylib/src/server/protocols/woutputmanagerv1.cpp:152-157` </location>
<code_context>
+ wlr_output_state_set_scale(&output_state, state.scale);
+ wlr_output_state_set_transform(&output_state, static_cast<wl_output_transform>(state.transform));
+
+ if (!wlr_output_test_state(wlr_output, &output_state)) {
+ if (errno == EBUSY) {
+ QTimer::singleShot(d->RETRY_DELAY_BASE_MS, this, [this, config]() {
+ sendResult(config, true);
+ });
+ wlr_output_state_finish(&output_state);
+ return;
+ }
</code_context>
<issue_to_address>
**suggestion:** Handling of EBUSY is duplicated for both test and commit, consider refactoring.
Extract the EBUSY handling into a shared helper to avoid code duplication and simplify future maintenance.
</issue_to_address>
### Comment 3
<location> `waylib/src/server/protocols/woutputmanagerv1.cpp:121` </location>
<code_context>
void WOutputManagerV1::sendResult(qw_output_configuration_v1 *config, bool ok)
{
W_D(WOutputManagerV1);
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring sendResult and updateConfig by extracting helper functions and centralizing retry logic to reduce nesting and improve clarity.
```suggestion
// 1) Extract per-output test/commit into a helper:
enum class StateResult { Success, Retry, Fail };
static StateResult tryApplyState(wlr_output *out, const WOutputState &st) {
struct wlr_output_state os;
wlr_output_state_init(&os);
wlr_output_state_set_enabled(&os, st.enabled);
if (st.mode) wlr_output_state_set_mode(&os, st.mode);
wlr_output_state_set_scale(&os, st.scale);
wlr_output_state_set_transform(&os, (wl_output_transform)st.transform);
auto cleanup = [&]{ wlr_output_state_finish(&os); };
if (!wlr_output_test_state(out, &os)) {
if (errno == EBUSY) { cleanup(); return StateResult::Retry; }
cleanup(); return StateResult::Fail;
}
if (!wlr_output_commit_state(out, &os)) {
if (errno == EBUSY) { cleanup(); return StateResult::Retry; }
cleanup(); return StateResult::Fail;
}
cleanup();
return StateResult::Success;
}
// 2) Centralize retry scheduling:
void WOutputManagerV1::scheduleRetry(qw_output_configuration_v1 *cfg) {
++d->retryCount;
int delay = d->RETRY_DELAY_BASE_MS * d->retryCount;
QTimer::singleShot(delay, this, [this, cfg] {
sendResult(cfg, /*retry=*/true);
});
}
// 3) Flatten sendResult:
void WOutputManagerV1::sendResult(qw_output_configuration_v1 *cfg, bool ok) {
W_D(WOutputManagerV1);
if (!ok && d->retryCount < d->MAX_RETRIES) {
return scheduleRetry(cfg);
}
d->retryCount = 0;
bool allOk = true;
for (auto &st : d->stateListPending) {
auto *out = st.output->nativeHandle();
if (!out) { allOk = false; break; }
auto r = tryApplyState(out, st);
if (r == StateResult::Retry) return scheduleRetry(cfg);
if (r == StateResult::Fail) { allOk = false; break; }
}
if (allOk) {
cfg->send_succeeded();
d->stateList = std::move(d->stateListPending);
} else {
cfg->send_failed();
}
delete cfg;
d->stateListPending.clear();
updateConfig();
}
// 4) Simplify updateConfig with a small lambda:
void WOutputManagerV1::updateConfig() {
W_D(WOutputManagerV1);
auto *cfg = qw_output_configuration_v1::create();
auto apply = [&](const WOutputState &s){
auto *h = qw_output_configuration_head_v1::create(*cfg, s.output->nativeHandle())->handle()->state;
h->enabled = s.enabled; h->x = s.x; h->y = s.y;
if (!s.enabled) return;
h->mode = s.mode;
h->scale = s.scale;
h->transform = (wl_output_transform)s.transform;
h->adaptive_sync_enabled = s.adaptiveSyncEnabled;
if (s.customModeSize.isValid()) {
h->custom_mode = {
uint32_t(s.customModeSize.width()),
uint32_t(s.customModeSize.height()),
s.customModeRefresh
};
}
};
for (auto &st : d->stateList) apply(st);
d->manager->set_configuration(*cfg);
}
```
These extracts remove deep nesting in `sendResult`, centralize retry logic, and keep `updateConfig` a single flat loop with one small lambda.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Dami-star
force-pushed
the
multi-output
branch
from
8acd979 to
6f016de
Compare
Collaborator
Author
|
@sourcery-ai review |
SourceryAI
reviewed
Oct 9, 2025
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- updateConfig never populates stateListPending before calling set_configuration, so sendResult ends up working on an empty list—assign stateListPending = stateList (or the new state) prior to invoking the configuration.
- The shared d->retryCount can get out of sync across overlapping configuration attempts—consider tracking retries per config instance or using a local counter scoped to each sendResult invocation.
- When scheduling a retry you’re calling sendResult(config, true), which jumps into the success path on retry—pass ok=false or refactor so retries properly re-attempt the failed operation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- updateConfig never populates stateListPending before calling set_configuration, so sendResult ends up working on an empty list—assign stateListPending = stateList (or the new state) prior to invoking the configuration.
- The shared d->retryCount can get out of sync across overlapping configuration attempts—consider tracking retries per config instance or using a local counter scoped to each sendResult invocation.
- When scheduling a retry you’re calling sendResult(config, true), which jumps into the success path on retry—pass ok=false or refactor so retries properly re-attempt the failed operation.
## Individual Comments
### Comment 1
<location> `waylib/src/server/protocols/woutputmanagerv1.cpp:125-131` </location>
<code_context>
- else
+
+ if (!ok && d->retryCount < d->MAX_RETRIES) {
+ d->retryCount++;
+ QTimer::singleShot(d->RETRY_DELAY_MS * (1 << d->retryCount), this, [this, config]() {
+ sendResult(config, true);
</code_context>
<issue_to_address>
**suggestion:** retryCount is incremented in multiple places, which could lead to confusion.
Since retryCount is incremented in both the main retry block and within the tryOperation lambda, this could cause off-by-one errors or make the logic harder to follow. Centralize the increment to improve clarity and prevent mistakes.
```suggestion
if (!ok && d->retryCount < d->MAX_RETRIES) {
QTimer::singleShot(d->RETRY_DELAY_MS * (1 << (d->retryCount + 1)), this, [this, config]() {
W_D(WOutputManagerV1);
d->retryCount++;
sendResult(config, true);
});
return;
}
```
</issue_to_address>
### Comment 2
<location> `waylib/src/server/protocols/woutputmanagerv1.cpp:121` </location>
<code_context>
void WOutputManagerV1::sendResult(qw_output_configuration_v1 *config, bool ok)
{
W_D(WOutputManagerV1);
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring retry and commit logic into small helper methods and flattening control flow in sendResult() for clarity.
Here’s one way to peel apart the retry‐and‐commit logic into small helpers and flatten the nesting in `sendResult()`. You can apply the same pattern to the other retry site, and likewise factor out the per‐state heads in `updateConfig()`.
```cpp
// In WOutputManagerV1Private:
void scheduleConfigRetry(qw_output_configuration_v1 *config) {
retryCount++;
const int delay = RETRY_DELAY_MS * (1 << retryCount);
QTimer::singleShot(delay, q, [q = QPointer<WOutputManagerV1>(q), config] {
if (q) q->sendResult(config, true);
});
}
void resetRetry() { retryCount = 0; }
// A helper to commit/test a single output state:
bool commitOutputState(const WOutputState &st) {
struct wlr_output_state os;
wlr_output_state_init(&os);
// apply st to os...
if (!wlr_output_test_state(h, &os) || !wlr_output_commit_state(h, &os)) {
wlr_output_state_finish(&os);
return false;
}
wlr_output_state_finish(&os);
return true;
}
```
Then simplify `sendResult()`:
```cpp
void WOutputManagerV1::sendResult(qw_output_configuration_v1 *config, bool ok) {
W_D(WOutputManagerV1);
// 1) Retry-on-failure blanket
if (!ok && d->retryCount < d->MAX_RETRIES) {
d->scheduleConfigRetry(config);
return;
}
d->resetRetry();
// 2) Hard fail if still bad
if (!ok) {
config->send_failed();
delete config;
d->stateListPending.clear();
QTimer::singleShot(0, this, &WOutputManagerV1::updateConfig);
return;
}
// 3) Try each pending state
bool allSuccess = true;
for (auto &st : d->stateListPending) {
if (!commitOutputState(st)) {
allSuccess = false;
break;
}
}
// 4) Succeed or fail
if (allSuccess) {
config->send_succeeded();
d->stateList.swap(d->stateListPending);
} else {
config->send_failed();
}
// 5) Cleanup & reapply
delete config;
d->stateListPending.clear();
QTimer::singleShot(0, this, &WOutputManagerV1::updateConfig);
}
```
And for `updateConfig()`, pull the inner‐loop body into a helper:
```cpp
void WOutputManagerV1::applyStateToConfig(qw_output_configuration_v1 *cfg, const WOutputState &st) {
auto *h = qw_output_configuration_head_v1::create(*cfg, st.output->nativeHandle())->handle();
h->state.enabled = st.enabled;
if (!st.enabled) return;
h->state.x = st.x; h->state.y = st.y;
h->state.mode = st.mode; /*…and so on…*/
}
void WOutputManagerV1::updateConfig() {
W_D(WOutputManagerV1);
auto *cfg = qw_output_configuration_v1::create();
for (auto &st : d->stateList)
applyStateToConfig(cfg, st);
d->manager->set_configuration(*cfg);
}
```
This:
- Extracts retry scheduling & reset into two tiny methods.
- Flattens `sendResult()` into 5 clear steps with early returns.
- Removes the inline lambda retry and duplicates.
- Moves per-state “head” setup into its own helper so `updateConfig()` reads as a straight loop.
</issue_to_address>Hi @Dami-star! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR improves the reliability of output configuration in the Wayland output manager by implementing error-based retry mechanisms and more comprehensive state management.
- Adds retry logic with progressive delays and EBUSY error handling for output configuration operations
- Expands output state configuration to include enabled flag, mode, scale, transform, adaptive sync, and custom mode settings
- Implements proper error handling and validation during output state operations
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
zccrs
requested changes
Oct 11, 2025
Dami-star
force-pushed
the
multi-output
branch
from
6f016de to
162f2cb
Compare
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/seat/helper.cpp:481` </location>
<code_context>
void Helper::onOutputTestOrApply(qw_output_configuration_v1 *config, bool onlyTest)
{
QList<WOutputState> states = m_outputManager->stateListPending();
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring duplicated state-building, viewport update, and settings logic into helper functions to simplify the main loops.
```markdown
Consider pulling out the duplicated “build‐qw_output_state” logic (and similarly viewport updates / settings writes) into small helpers. That will collapse the two big loops in onOutputTestOrApply and applyOutputConfiguration into a few calls.
Example:
```cpp
// header
private:
qw_output_state buildState(const WOutputState &s);
void updateViewport(const WOutputState &s);
void saveSettings(const QList<WOutputState> &states, const QString &groupPrefix);
```
```cpp
// impl
qw_output_state Helper::buildState(const WOutputState &s) {
qw_output_state st;
st.set_enabled(s.enabled);
if (!s.enabled)
return st;
if (s.mode)
st.set_mode(s.mode);
else
st.set_custom_mode(s.customModeSize.width(),
s.customModeSize.height(),
s.customModeRefresh);
st.set_adaptive_sync_enabled(s.adaptiveSyncEnabled);
st.set_transform(static_cast<wl_output_transform>(s.transform));
st.set_scale(s.scale);
return st;
}
void Helper::updateViewport(const WOutputState &s) {
if (!s.enabled) return;
if (auto *vp = getOutput(s.output)->screenViewport()) {
if (auto *item = qobject_cast<WOutputItem*>(vp->parentItem())) {
item->setX(s.x);
item->setY(s.y);
}
}
}
void Helper::saveSettings(const QList<WOutputState> &states,
const QString &groupPrefix)
{
QString loc = QStandardPaths::writableLocation(
QStandardPaths::AppConfigLocation)
+ "/output.ini";
QSettings settings(loc, QSettings::IniFormat);
for (auto &s : states) {
settings.beginGroup(
QString("%1.%2").arg(groupPrefix).arg(s.output->name()));
const auto &m = s.mode;
settings.setValue("width",
m ? m->width : s.customModeSize.width());
settings.setValue("height",
m ? m->height : s.customModeSize.height());
settings.setValue("refresh",
m ? m->refresh : s.customModeRefresh);
settings.setValue("transform", s.transform);
settings.setValue("scale", s.scale);
settings.setValue("adaptiveSyncEnabled",
s.adaptiveSyncEnabled);
settings.endGroup();
}
}
```
Then your loops reduce to:
```cpp
// test‐only path
bool ok = true;
for (auto &st : states) {
auto built = buildState(st);
ok &= st.output->handle()->test_state(built);
}
m_outputManager->sendResult(config, ok);
// apply path
QVector<qw_output_state> v;
v.reserve(states.size());
for (auto &st : states)
v.append(buildState(st));
// … run tests + commit …
if (ok) {
for (auto &st : states)
updateViewport(st);
saveSettings(states, "output");
}
```
This collapses duplication, and makes each method’s structure far clearer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Dami-star
force-pushed
the
multi-output
branch
3 times, most recently
from
c672f29 to
5047633
Compare
There was a problem hiding this comment.
Sorry @Dami-star, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
zccrs
requested changes
Oct 15, 2025
Dami-star
force-pushed
the
multi-output
branch
from
5047633 to
8d9767c
Compare
zccrs
requested changes
Oct 17, 2025
Dami-star
force-pushed
the
multi-output
branch
from
8d9767c to
fd66298
Compare
Dami-star
force-pushed
the
multi-output
branch
2 times, most recently
from
9ec4da6 to
b728b6e
Compare
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The Helper::onOutputTestOrApply method is quite large and mixes test/apply logic heavily—consider refactoring common state‐preparation into smaller helper functions to reduce duplication and improve clarity.
- The m_outputConfigTimer member in Helper is never used—either remove it or implement the intended delay/throttle behavior for pending output commits.
- In WOutputHelper::commit, the hardwareChanged flag uses
ok || committedBits, but it probably should beok && committedBitsto only emit change signals on successful commits.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Helper::onOutputTestOrApply method is quite large and mixes test/apply logic heavily—consider refactoring common state‐preparation into smaller helper functions to reduce duplication and improve clarity.
- The m_outputConfigTimer member in Helper is never used—either remove it or implement the intended delay/throttle behavior for pending output commits.
- In WOutputHelper::commit, the hardwareChanged flag uses `ok || committedBits`, but it probably should be `ok && committedBits` to only emit change signals on successful commits.
## Individual Comments
### Comment 1
<location> `src/seat/helper.cpp:582-583` </location>
<code_context>
+ }
+ m_pendingOutputConfig.pendingCommits++;
+ }
+ for (int i = 0; i < states.size(); ++i) {
+ onOutputCommitFinished(m_pendingOutputConfig.config, true);
+ }
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Output commit completion is called synchronously for all outputs, which may not reflect actual commit results.
The current approach assumes all output commits succeed, which may hide individual failures or asynchronous issues. Please update the logic to reflect actual commit results for each output, using appropriate completion signals.
</issue_to_address>
### Comment 2
<location> `waylib/src/server/qtquick/woutputhelper.cpp:245-246` </location>
<code_context>
bool ok = d->qwoutput()->commit_state(&state);
wlr_output_state_finish(&state);
+ bool hardwareChanged = ok || (state.committed & (WLR_OUTPUT_STATE_MODE | WLR_OUTPUT_STATE_SCALE | WLR_OUTPUT_STATE_TRANSFORM));
+ if (hardwareChanged) {
+ if (state.committed & WLR_OUTPUT_STATE_MODE) {
+ Q_EMIT d->output->modeChanged();
</code_context>
<issue_to_address>
**question (bug_risk):** The hardwareChanged condition may emit signals even if commit failed.
Since 'hardwareChanged' can be true even if 'ok' is false, signals may be emitted after a failed commit. Please review if signals should only be emitted when the commit succeeds, or if error handling should be added.
</issue_to_address>
### Comment 3
<location> `waylib/src/server/qtquick/woutputrenderwindow.cpp:1063-1061` </location>
<code_context>
if (output()->offscreen())
return true;
+ if (hasPendingModeChange()) {
+ WOutputHelper::commit();
+ return true;
+ }
+
</code_context>
<issue_to_address>
**question (bug_risk):** Early return on pending mode change may skip buffer commit logic.
Please verify that committing and returning early when hasPendingModeChange is true will not cause missed buffer updates or inconsistent output, particularly if mode changes and buffer commits can occur together.
</issue_to_address>
### Comment 4
<location> `src/seat/helper.cpp:481` </location>
<code_context>
void Helper::onOutputTestOrApply(qw_output_configuration_v1 *config, bool onlyTest)
{
QList<WOutputState> states = m_outputManager->stateListPending();
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring onOutputTestOrApply by extracting viewport/render-window lookup and settings-save logic into helper methods to reduce duplication and improve maintainability.
```markdown
The new `onOutputTestOrApply` is still very large and duplicates viewport/render-window lookup and settings-save logic across test vs. apply paths. Splitting it into smaller methods and extracting the common “find viewport & renderWindow” and “write settings” pieces will make it much easier to follow and maintain.
For example, at the top of your .cpp you could add:
```cpp
static bool findViewportAndWindow(WOutput *output,
WOutputViewport*& outViewport,
WOutputRenderWindow*& outWindow)
{
outViewport = getOutput(output)->screenViewport();
if (!outViewport) return false;
outWindow = outViewport->outputRenderWindow();
return outWindow != nullptr;
}
```
And a helper to save your final list of states:
```cpp
static void writeStatesToSettings(const QList<WOutputState>& states) {
QString file = QStandardPaths::writableLocation(QStandardPaths::AppConfigLocation)
+ "/output.ini";
QSettings settings(file, QSettings::IniFormat);
for (auto &s : states) {
settings.beginGroup(QString("output.%1").arg(s.output->name()));
settings.setValue("width",
s.mode ? s.mode->width : s.customModeSize.width());
settings.setValue("height",
s.mode ? s.mode->height : s.customModeSize.height());
settings.setValue("refresh",
s.mode ? s.mode->refresh : s.customModeRefresh);
settings.setValue("transform", s.transform);
settings.setValue("scale", s.scale);
settings.setValue("adaptiveSyncEnabled",
s.adaptiveSyncEnabled);
settings.endGroup();
}
}
```
Then your `onOutputTestOrApply` reduces to:
```cpp
void Helper::onOutputTestOrApply(qw_output_configuration_v1 *config,
bool onlyTest)
{
auto states = m_outputManager->stateListPending();
if (onlyTest) {
bool ok = true;
for (auto &st : states) {
WOutputViewport *vp;
WOutputRenderWindow *rw;
if (!findViewportAndWindow(st.output, vp, rw)) {
ok = false;
} else {
qw_output_state ns;
// ... set ns fields ...
ok &= st.output->handle()->test_state(ns);
}
}
m_outputManager->sendResult(config, ok);
return;
}
// apply case
if (m_pendingOutputConfig.config)
m_outputManager->sendResult(m_pendingOutputConfig.config, false);
m_pendingOutputConfig = { config, states, 0, true };
for (auto &st : states) {
WOutputViewport *vp;
WOutputRenderWindow *rw;
if (!findViewportAndWindow(st.output, vp, rw)) {
qWarning() << "No viewport/renderWindow for" << st.output->name();
m_outputManager->sendResult(config, false);
m_pendingOutputConfig = {};
return;
}
// ... do renderWindow->setOutputXXX(vp, st.xxx) ...
m_pendingOutputConfig.pendingCommits++;
}
// immediately fire fake commit callbacks
for (int i = 0; i < states.size(); ++i)
onOutputCommitFinished(config, true);
}
```
And finally in `onOutputCommitFinished`:
```cpp
void Helper::onOutputCommitFinished(...) {
// ...
if (m_pendingOutputConfig.pendingCommits == 0) {
if (m_pendingOutputConfig.allSuccess)
writeStatesToSettings(m_pendingOutputConfig.states);
m_outputManager->sendResult(config, ok);
m_pendingOutputConfig = {};
}
}
```
This extracts the two major concerns (lookup & settings-save) and splits the test vs. apply flows into concise blocks. It keeps all functionality intact, removes duplication, and drastically reduces the cyclomatic complexity of the original large method.
</issue_to_address>
### Comment 5
<location> `waylib/src/server/qtquick/woutputrenderwindow.cpp:1819` </location>
<code_context>
}
}
+void WOutputRenderWindow::setOutputMode(WOutputViewport *output, const wlr_output_mode *mode)
+{
+ Q_D(WOutputRenderWindow);
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a helper method to encapsulate the repeated pattern of looking up the output helper, invoking a setter, and calling update().
Consider factoring out the “lookup‐helper + call + update()” pattern. For example, add in your class:
```cpp
private:
template<typename F>
void modifyOutput(WOutputViewport *output, F fn) {
Q_D(WOutputRenderWindow);
if (auto helper = d->getOutputHelper(output)) {
fn(helper);
update();
}
}
```
Then each setter collapses to a one‐liner:
```cpp
void WOutputRenderWindow::setOutputMode(WOutputViewport *output, const wlr_output_mode *mode) {
modifyOutput(output, [mode](auto *h){ h->setMode(mode); });
}
void WOutputRenderWindow::setOutputCustomMode(WOutputViewport *output,
int32_t w, int32_t h, int32_t r) {
modifyOutput(output, [w,h,r](auto *h){ h->setCustomMode(w,h,r); });
}
void WOutputRenderWindow::setOutputAdaptiveSync(WOutputViewport *output, bool e) {
modifyOutput(output, [e](auto *h){ h->setAdaptiveSyncEnabled(e); });
}
void WOutputRenderWindow::setOutputEnabled(WOutputViewport *output, bool e) {
modifyOutput(output, [e](auto *h){ h->setEnabled(e); });
}
```
If you prefer member‐pointer syntax:
```cpp
template<typename M, typename... A>
void applySetting(WOutputViewport *output, M m, A&&... args) {
Q_D(WOutputRenderWindow);
if (auto helper = d->getOutputHelper(output)) {
(helper->*m)(std::forward<A>(args)...);
update();
}
}
// usage
applySetting(output, &OutputHelper::setScale, scale);
applySetting(output, &OutputHelper::setTransform, t);
```
This eliminates the boilerplate while preserving all functionality.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Dami-star
force-pushed
the
multi-output
branch
2 times, most recently
from
839fb6c to
a02cc11
Compare
zccrs
requested changes
Oct 22, 2025
Dami-star
force-pushed
the
multi-output
branch
from
a02cc11 to
2221d98
Compare
zccrs
requested changes
Oct 27, 2025
zccrs
requested changes
Oct 27, 2025
Dami-star
force-pushed
the
multi-output
branch
from
2221d98 to
7b6403e
Compare
zccrs
requested changes
Oct 28, 2025
Dami-star
force-pushed
the
multi-output
branch
from
7b6403e to
216eda2
Compare
zccrs
requested changes
Oct 28, 2025
Dami-star
force-pushed
the
multi-output
branch
2 times, most recently
from
9a41e2d to
a663c97
Compare
zccrs
requested changes
Oct 28, 2025
Dami-star
force-pushed
the
multi-output
branch
from
a663c97 to
77221d9
Compare
zccrs
requested changes
Oct 29, 2025
Dami-star
force-pushed
the
multi-output
branch
2 times, most recently
from
4836a1c to
0a96efa
Compare
zccrs
requested changes
Oct 29, 2025
zccrs
requested changes
Oct 29, 2025
Dami-star
force-pushed
the
multi-output
branch
from
0a96efa to
1ccd436
Compare
zccrs
requested changes
Oct 30, 2025
- Use WOutputRenderWindow interface for output configuration - Complete output state management with all properties - Ensure settings only saved when configuration succeeds
Dami-star
force-pushed
the
multi-output
branch
from
1ccd436 to
51561b3
Compare
zccrs
approved these changes
Oct 30, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Dami-star, zccrs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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.
refactor: implement atomic multi-output configuration
Summary by Sourcery
Overhaul the multi-output configuration flow to use atomic test/apply phases, track individual commit outcomes, extend rendering APIs for output state changes, and ensure output settings are only persisted upon successful configuration
Bug Fixes:
Enhancements: