dkochmanski
commented
4 years ago this incompatibility is fine, lgtm, thanks!
Closed paulapatience closed 4 years ago
dkochmanski
commented
4 years ago this incompatibility is fine, lgtm, thanks!
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.