Fix handling of non V1.5 XRandr.#364
Fix handling of non V1.5 XRandr.#364tsipinakis merged 2 commits intodunst-project:masterfrom SteveJones:master
Conversation
tsipinakis
left a comment
tsipinakis
left a comment
There was a problem hiding this comment.
Sorry for the delay in reviewing this, I wasn't home for the past few days.
|
|
||
| int randr_event_base = 0; | ||
|
|
||
| static int randr_major_version = 1; |
There was a problem hiding this comment.
Why default these to 1 and 5? I think setting them to 0 would be a better choice.
There was a problem hiding this comment.
This was me reading the protocol docs which say that the client has to send the version it expects, but the xrandr code sends this itself, it doesn't use the initial value of the pointers you pass in. I'll fix this.
There was a problem hiding this comment.
This was me reading the protocol docs which say that the client has to send the version it expects
Where does it say that? It's not mentioned anywhere in the xrandr man page?
There was a problem hiding this comment.
It's in the protocol specification, but the library API doesn't match that.
There was a problem hiding this comment.
Found it, JFTR in this document search for QueryVersion.
The client sends the highest supported version to the server
and the server sends the highest version it supports, but no
higher than the requested version. Major versions changes can
introduce incompatibilities in existing functionality, minor
version changes introduce only backward compatible changes.
It is the clients responsibility to ensure that the server
supports a version which is compatible with its expectations.
In that case it should probably be left in even though it's not doing anything currently and the change is trivial I think it's best to go with the spec.
| int n; | ||
| XRRMonitorInfo *m = XRRGetMonitors(xctx.dpy, RootWindow(xctx.dpy, DefaultScreen(xctx.dpy)), true, &n); | ||
|
|
||
| if (m == NULL || n == -1) { |
There was a problem hiding this comment.
In xrandr.c I see that there's an error check for n == -1 so this check should probably stay and have a separate check for version.
There was a problem hiding this comment.
Hmm, looking at https://cgit.freedesktop.org/xorg/lib/libXrandr/tree/src/XrrMonitor.c it's checking a weird edge case where the server returns 0 monitors, but I guess that's possible. I'll put the check back.
| XRRMonitorInfo *m = XRRGetMonitors(xctx.dpy, RootWindow(xctx.dpy, DefaultScreen(xctx.dpy)), true, &n); | ||
|
|
||
| if (m == NULL || n == -1) { | ||
| if (randr_minor_version < 5) { |
There was a problem hiding this comment.
You're only checking for minor < 5 here which is going to break at the next major version. A more complete check would be randr_major_version < 1 || (randr_major_version == 1 && randr_minor_version < 5)
Additionally, there should probably be a more descriptive error message mentioning the outdated version.
|
@SteveJones Ping, are you still active? |
|
Hi yeah, sorry this slipped my mind. I'll try and get this fixed up in the next day or so. |
|
Looks like you misspelled the major version variable. Have you tested this in a system with an outdated xrandr version (and probably on one with no xrandr installed at all)? Just to confirm that it works as intended. |
|
That'll teach me to add in a cosmetic change after testing it builds (it won't). I've tested it on outdated, XRandR, it's why I wrote it. |
tsipinakis
left a comment
tsipinakis
left a comment
There was a problem hiding this comment.
Just a minor mistake after that this should be ready to be merged
Can you squash the commits to avoid polluting the history?
PS: Sorry for the delay (again) you caught me both times on days that I was out.
| void randr_update() | ||
| { | ||
| if (randr_minor_version < 5) { | ||
| if (randr_major_version > 1 |
There was a problem hiding this comment.
Noticed this just as I was about to click merge:
We're checking for the error condition so it should be randr_major_version < 1
|
Right, fixed that and squashed. Tests just passed. It builds and seems happy. Should be good to go. |
Apparently not :p Just realised you used tabs, we're using 8 spaces for indentation. should be a quick fix. |
If we're so nitpicky now, please also remove the last point on your commit headline. It's a common standard. |
That's not a nitpick in my opinion, it's adhering to the projects code style else it just becomes a mess. The commit message format is generally up to the contributor and I don't interfere with that, just give hints about the correct standards (and I didn't actually notice the period this time). |
|
Ping @SteveJones |
Due to the way Xlib handles errors XRRGetMonitors will cause dunst to exit if the server doesn't support version 1.5 of RandR. The check for null is effectively dead code but in theory the number of monitors can be < 1 so treat this as an error. Change the code to fetch the RandR version and fallback if the version is less than 1.5 when getting monitors. This fix was brought up by @SteveJones on GitHub
Albeit it's possible to run with XRandR extension < 1.5, dunst needs the headers of libxrandr 1.5.
|
I misused my privileges the first time now pushed the stylefixes to this PR. |
Fix handling of non V1.5 XRandr.
3 participants
Due to the way Xlib handles errors XRRGetMonitors will cause dunst to exit if
the server doesn't support version 1.5 of Randr. The check for null is
effectively dead code. Change the code to fetch the Randr version and fallback
if the version is less than 1.5 when getting monitors.