xrandr backend: "invalid arguments" error

[Walther]

Okay, sorry if this report is going to be a little vague, but it happened in the past, yet I can't reproduce it again.
Basically, due to another issue I had, I had to use xrandr rather than disper to setup my second monitor for a presentation. But after it, disper would stop working, XRRSetScreenConfigAndRate would throw me X error messages hinting at invalid parameters.

Anyway. I decided to track this down and see what was xrandr doing differently from disper. Xrandr was issuing XRRSetScreenConfigAndRate on a single output pipe, whereas disper kept calling the function to two pipes (and the second one was throwing off the error). By the way, there was no external monitor connected at this time.

In the end, I found that the reason disper had tagged both pipes as "changed" was this piece of code (_arrange_outputs in xrandr/core.py):

        # Normalize the postions so to the upper left cornor of all outputs
        # is at 0,0
        min_x = 32768
        min_y = 32768
        for output in self.get_outputs():
            if output._mode == None: continue
            if output._x < min_x: min_x = output._x
            if output._y < min_y: min_y = output._y
        for output in self.get_outputs():
            if output._mode == None: continue
            output._x -= min_x
            output._y -= min_y
            output._changes = output._changes | xrandr.CHANGES_POSITION

min_x and min_y were both zero, yet disper was tagging all the connected pipes as "changed".

Can I recommend that you encapsulate that last "for" in an "if", like this?

        if min_x != 0 or min_y != 0:
            for output in self.get_outputs():
                if output._mode == None: continue
                output._x -= min_x
                output._y -= min_y
                output._changes = output._changes | xrandr.CHANGES_POSITION

I do not know if it would really fix whatever caused the bug, but it would at least prevent pipes that were not changed from being marked as modified. Maybe the real bug-fix is to skip "not changed" outputs? Because that's what a similar loop is doing at the beginning of the function (it reads: " Skip not changed and not used outputs")

PS: Also, this function has a redundant line:

            if not relative or not relative._mode:
                output._x = 0
                output._y = 0
                output._changes = output._changes | xrandr.CHANGES_POSITION

Because the set of "changed position" is done anyway later on, in line 873.

I attach a diff with both suggestions in case the text isn't clear (disper version 0.3.0).

[Walther (walther-md)]

I just realized that your xrandr backend is more or less a "translation" for the code of xrandr. If so, then indeed xrandr does a " if (min_x || min_y)" before iterating pipes and setting their "changed position" flag. So my proposed patch is most likely the correct solution.

Though, you are missing a "continue" in the case where the monitor isn't relative. Your backend reads like this:

            if not relative or not relative._mode:
                output._x = 0
                output._y = 0
                output._changes = output._changes | xrandr.CHANGES_POSITION
            if output._relation == xrandr.RELATION_LEFT_OF:

Whereas xrandr's code reads more like this:

            if not relative or not relative._mode:
                output._x = 0
                output._y = 0
                output._changes = output._changes | xrandr.CHANGES_POSITION
                continue
            if output._relation == xrandr.RELATION_LEFT_OF:

Well, you could solve this by adding the "continue" line, or just by removing the "changes" line (as patch does) AND changing the next if for a "elif". You can decide which one to apply.

[Walther (walther-md)]

Okay, I've been working on this some more, and I realized that just applying my patch "breaks" disper, because "disper -s -r max" does nothing.

Again, following the code, it appears that no display was tagged as "changed" with my fix, though this seems to imply a bug somewhere else (since a display shouldn't be tagged as changed in _arrange_output when it was not modified there!). I think the bug lies, furthermore, in function set_to_mode (xrandr/core.py), since it isn't setting changes to CHANGES_MODE.

I attach an updated patch which addresses this other issue. Though now I wonder if "set_to_preferred_mode" shouldn't apply a similar fix, and set its display as "modified"? (testing here reveals that it works with my patch when you just issue a "disper.py -s").

[wvengen (wvengen)]

Hi Walther, thanks for looking into this! I do not use XRandR myself and this code is not mine, so I'm really glad you take the time to get it fixed. I'll happily apply the patch when it's done. Keep up the good work.

[Walther (walther-md)]

Oh, I had no idea nobody really was maintaining this code. Though it is easy to test, even with an nvidia card, since I do that. Using nouveau means I can't use the nvidia back-end :P

Anyway, I read some more around, and it appears my last patch works because "set_to_preferred_mode" is a function which is never used in the code-base.

But, just for completeness, I update the patch to set the "changes" field even in that case.

The attached file here should be considered as the fix for this issue.

[wvengen (wvengen)]

Sorry for the delay, your "final" patch has been committed. Thanks!
I haven't tested it too thoroughly though, if there are issues please let me know.