<table><tr><td style="">sebas planned changes to this revision.<br />sebas added inline comments.</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D2330" rel="noreferrer">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D2330#inline-9330" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">graesslin</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xrandrconfig.cpp:517-520</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">it looks strange to me that you first access the currentModeId and afterwards check whether there is a currentMode.</p>

<p style="padding: 0; margin: 8px;">What about a one liner?</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">const int modeId = kscreenOutput->currentMode() ? kscreenOutput->currentModeId().toInt() : kscreenOutput->preferredModeId().toInt();</pre></div>

<p style="padding: 0; margin: 8px;">If you don't like the one-liner (which I can understand) I would inverse the logic and init the modeId with preferredModeId and only set to currentModeId if there is a currentMode.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I had it the other way round first.</p>

<p style="padding: 0; margin: 8px;">preferredModeId() does a lot of magic to find a mode, so it's much slower and more invasive than currentModeId(), therefore, it makes sense to only execute preferredModeId() if we have to, hence the slightly reversed logic.</p>

<p style="padding: 0; margin: 8px;">Your one-liner proposal addresses this as well, and looks more logical. I'll change.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D2330#inline-9334" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dvratil</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xrandrconfig.cpp:519</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I wonder if it might be safer to query the preferred mode from corresponding <tt style="background: #ebebeb; font-size: 13px;">XRandROutput</tt> rather than trusting user-provided <tt style="background: #ebebeb; font-size: 13px;">KScreen::Output</tt> here?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Well, the API doesn't forbid to not set a mode but enable an output. This is how I triggered the crash:</p>

<p style="padding: 0; margin: 8px;">kscreen-doctor output.280.enable output.280.position.2560,0</p>

<p style="padding: 0; margin: 8px;">No mode set, without patch it crashes, with patch, it succeeds and does the logical thing.</p>

<p style="padding: 0; margin: 8px;">I *think* (haven't checked) that falling back to preferred here is really a good idea, since</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">otherwise, the next thing is that we pass an invalid mode into xcb -- who knows what happens</li>
<li class="remarkup-list-item">the config can be loaded from a file which doesn't have mode id set (always the case for disabled displays), so at some point we need to look at preferred mode and pick that if we want to enable (I think the kded module does a pretty good job here, but I wouldn't guarantee that it *always* gets this right (as I said, not enforced), so better safe than sorry here.</li>
</ul>

<p style="padding: 0; margin: 8px;">The mode may not be set on the XRandROutput on disabled outputs, that's the problem I'm addressing here.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D2330#inline-9333" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dvratil</span> wrote in <span style="color: #4b4d51; font-weight: bold;">xrandrconfig.cpp:561</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">In <tt style="background: #ebebeb; font-size: 13px;">changeOutput()</tt> we only change position, mode or rotation of an already-enabled output. I would argue that if the mode is missing in the <tt style="background: #ebebeb; font-size: 13px;">kscreenOutput</tt> here (for any reason) it would make more sense to interpret it as "no change" and use <tt style="background: #ebebeb; font-size: 13px;">xOutput->currentMode()->id()</tt> and only if <tt style="background: #ebebeb; font-size: 13px;">xOutput->currentMode()</tt> is null then fall back to preferred mode (although that already hints that something weird is going on).</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Makes sense. I'll change.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>rLIBKSCREEN KScreen Library</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D2330" rel="noreferrer">https://phabricator.kde.org/D2330</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>sebas, dvratil<br /><strong>Cc: </strong>graesslin, plasma-devel, Plasma, ali-mohamed, jensreuterberg, abetts, sebas<br /></div>