extensions: randr: fix RR-GET-SCREEN-INFO rates #174
Merged
Conversation
This file contains 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
The documentation for the RRGetScreenInfo request is admittedly opaque, but each screen size's corresponding sequence of refresh rates is preceded by a refresh rate count, which the length of the refresh rate information sequence includes, and the first of which RR-GET-SCREEN-INFO was skipping. Also, RR-GET-SCREEN-INFO was invariably reading the current refresh rate and the refresh rate information sequence whether the client had previously queried the version or not (which it had no way of knowing), which led to impenetrable SB-INT:INVALID-ARRAY-INDEX-ERRORs (on SBCL) when the server omitted the refresh rate information sequence in its reply. This commit introduces RR-MAYBE-QUERY-VERSION, which queries the version only when necessary (i.e., when supplied with NIL MAJOR and MINOR arguments), to conveniently handle version-dependent requests, and RR-HAS-RATES to handle the conditional refresh rates. Functions requiring RR-MAYBE-QUERY-VERSION should themselves accept MAJOR and MINOR as arguments in order to pass them on to RR-MAYBE-QUERY-VERSION. Although this commit introduces two backwards-incompatible changes, they should (hopefully) not be too inconvenient because this extension is as yet unfinished and thus unsuitable for general use. The first, and more important, change is the replacement of optional arguments with keyword arguments in all request functions having optional arguments, which affects only those callers who were supplying any optional arguments. Keyword arguments are more practical when functions have many unrequired arguments, and this will be the case of all functions executing version-dependent requests because the functions will need the extra (unrequired) MAJOR and MINOR arguments. The second, and more stylistic, change is the reordering of RR-GET-SCREEN-INFO's multiple return values in order that the current refresh rate and the refresh rate information sequence be located at the end (which evidently affects only the callers of the function). This is more consistent, because any parameters introduced in later protocol versions will belong at the end of any existing multiple return values in order to preserve backwards compatibility. Additionally: - Declaim RR-QUERY-VERSION and RR-GET-SCREEN-INFO, and expand their docstrings. - Fix the incorrect type definition of ROTATION-MASK. - Wrap some overlong lines. - Clean up some comments and whitespace. - Conform various details to the rest of the codebase.
paulapatience
force-pushed
the
bugfix/rr-get-screen-info
branch
from
2a00e0c to
9c91ef6
Compare
|
this incompatibility is fine, lgtm, thanks! |
JMC-design
reviewed
Sep 18, 2020
There was a problem hiding this comment.
Every extension needs to have it's version queried before any other use of the extension takes place. This is something better fixed with documentation as it is standard procedure.
As for changing the &optionals to &keys, why? You've just made every function slower.
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.
The documentation for the RRGetScreenInfo request is admittedly opaque, but each screen size's corresponding sequence of refresh rates is preceded by a refresh rate count, which the length of the refresh rate information sequence includes, and the first of which RR-GET-SCREEN-INFO was skipping. Also, RR-GET-SCREEN-INFO was invariably reading the current refresh rate and the refresh rate information sequence whether the client had previously queried the version or not (which it had no way of knowing), which led to impenetrable SB-INT:INVALID-ARRAY-INDEX-ERRORs (on SBCL) when the server omitted the refresh rate information sequence in its reply.
This commit introduces RR-MAYBE-QUERY-VERSION, which queries the version only when necessary (i.e., when supplied with NIL MAJOR and MINOR arguments), to conveniently handle version-dependent requests, and RR-HAS-RATES to handle the conditional refresh rates. Functions requiring RR-MAYBE-QUERY-VERSION should themselves accept MAJOR and MINOR as arguments in order to pass them on to RR-MAYBE-QUERY-VERSION.
Although this commit introduces two backwards-incompatible changes, they should (hopefully) not be too inconvenient because this extension is as yet unfinished and thus unsuitable for general use. The first, and more important, change is the replacement of optional arguments with keyword arguments in all request functions having optional arguments, which affects only those callers who were supplying any optional arguments. Keyword arguments are more practical when functions have many unrequired arguments, and this will be the case of all functions executing version-dependent requests because the functions will need the extra (unrequired) MAJOR and MINOR arguments. The second, and more stylistic, change is the reordering of RR-GET-SCREEN-INFO's multiple return values in order that the current refresh rate and the refresh rate information sequence be located at the end (which evidently affects only the callers of the function). This is more consistent, because any parameters introduced in later protocol versions will belong at the end of any existing multiple return values in order to preserve backwards compatibility.
Additionally:
Merging this PR after #172 or #173 will result in a conflict. If either of those PRs is accepted before this one, I can rebase this one to facilitate its merge.