[Differential] [Planned Changes To] D2330: [xrandr backend] fall back to preferred mode, fix crash

[sebas (Sebastian Kügler)]

sebas planned changes to this revision.
sebas added inline comments.

INLINE COMMENTS

> graesslin wrote in xrandrconfig.cpp:517-520
> it looks strange to me that you first access the currentModeId and afterwards check whether there is a currentMode.
> 
> What about a one liner?
> 
>   const int modeId = kscreenOutput->currentMode() ? kscreenOutput->currentModeId().toInt() : kscreenOutput->preferredModeId().toInt();
> 
> 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.

I had it the other way round first.

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.

Your one-liner proposal addresses this as well, and looks more logical. I'll change.

> dvratil wrote in xrandrconfig.cpp:519
> I wonder if it might be safer to query the preferred mode from corresponding `XRandROutput` rather than trusting user-provided `KScreen::Output` here?

Well, the API doesn't forbid to not set a mode but enable an output. This is how I triggered the crash:

kscreen-doctor output.280.enable output.280.position.2560,0

No mode set, without patch it crashes, with patch, it succeeds and does the logical thing.

I *think* (haven't checked) that falling back to preferred here is really a good idea, since

- otherwise, the next thing is that we pass an invalid mode into xcb -- who knows what happens
- 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.

The mode may not be set on the XRandROutput on disabled outputs, that's the problem I'm addressing here.

> dvratil wrote in xrandrconfig.cpp:561
> In `changeOutput()` we only change position, mode or rotation of an already-enabled output. I would argue that if the mode is missing in the `kscreenOutput` here (for any reason) it would make more sense to interpret it as "no change" and use `xOutput->currentMode()->id()` and only if `xOutput->currentMode()` is null then fall back to preferred mode (although that already hints that something weird is going on).

Makes sense. I'll change.

REPOSITORY
  rLIBKSCREEN KScreen Library

REVISION DETAIL
  https://phabricator.kde.org/D2330

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: sebas, dvratil
Cc: graesslin, plasma-devel, #plasma, ali-mohamed, jensreuterberg, abetts, sebas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160802/4deb9324/attachment.html>