On Sun, Aug 21, 2011 at 11:39 PM, Peter Hutterer
<peter.hutte...@who-t.net> wrote:
> On Fri, Aug 19, 2011 at 05:23:53PM -0700, Jason Gerecke wrote:
>> Instead of having 'set_output' be responsible for choosing an
>> appropriate helper function, we let the helper functions be
>> autonomous. If they cannot find an appropriate output (or
>> encounter an error), they simply return False to let the
>> caller know that it failed.
>> ---
>> Changes from v1:
>> * Don't allow Xinerama to work if RandR looks like it would
>> * Add documentation to the functions I touched
>>
>> tools/xsetwacom.c | 77
>> ++++++++++++++++++++++++++++++++++++++++-------------
>> 1 files changed, 58 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/xsetwacom.c b/tools/xsetwacom.c
>> index dd0a3e5..ffb2875 100644
>> --- a/tools/xsetwacom.c
>> +++ b/tools/xsetwacom.c
>> @@ -2003,8 +2003,15 @@ static void _set_matrix(Display *dpy, XDevice *dev,
>> _set_matrix_prop(dpy, dev, matrix);
>> }
>>
>> -static void set_output_xrandr(Display *dpy, XDevice *dev, param_t *param,
>> int argc, char **argv)
>> +/**
>> + * Adjust the transformation matrix based on RandR settings. This function
>> + * will attempt to map the given device to the output named in the first
>> + * command-line argument. If the function succeeds, it returns 'true'.
>> + */
>> +static Bool set_output_xrandr(Display *dpy, XDevice *dev, param_t *param,
>> int argc, char **argv)
>> {
>> + int opcode, event, error;
>> + int maj, min;
>> int i, found = 0;
>> int x, y, width, height;
>> char *output_name;
>> @@ -2012,6 +2019,21 @@ static void set_output_xrandr(Display *dpy, XDevice
>> *dev, param_t *param, int ar
>> XRROutputInfo *output_info;
>> XRRCrtcInfo *crtc_info;
>>
>> + if (!XQueryExtension(dpy, "RANDR", &opcode, &event, &error) ||
>> + !XRRQueryVersion(dpy, &maj, &min) || (maj * 1000 + min) < 1002 ||
>> + XQueryExtension(dpy, "NV-CONTROL", &opcode, &event, &error))
>
> this blurb is used twice, just inverted. would be easier to just define a
> small function need_xrandr() and pick based on that.
>
That's probably a good idea. Its need may be negated if we can get a
parser to work (since only the parser would need to know that info,
given our restricted syntax of Xinerama heads), but I'll keep it in
mind.
>> + {
>> + /* RandR not present or earlier than version 1.2
>> + *
>> + * Server bug causes the NVIDIA driver to report RandR 1.3
>> + * support but it doesn't expose RandR CRTCs; ignore if
>> + * this is the case.
>> + */
>> + TRACE("RandR extension not found, too old, or NV-CONTROL "
>> + "extension is also present.\n");
>> + return False;
>> + }
>> +
>> output_name = argv[0];
>>
>> res = XRRGetScreenResources(dpy, DefaultRootWindow(dpy));
>> @@ -2048,36 +2070,60 @@ static void set_output_xrandr(Display *dpy, XDevice
>> *dev, param_t *param, int ar
>> TRACE("Setting CRTC %s\n", output_name);
>> _set_matrix(dpy, dev, crtc_info->x, crtc_info->y,
>> crtc_info->width, crtc_info->height);
>> +
>> + return True;
>> } else
>> + {
>> printf("Unable to find output '%s'. "
>> "Output may not be connected.\n", output_name);
>>
>> + return False;
>> + }
>> }
>>
>> /**
>> - * Adjust the transformation matrix based on the Xinerama settings. For
>> - * TwinView This would better be done with libXNVCtrl but until they learn
>> - * to package it properly, rely on Xinerama. Besides, libXNVCtrl isn't
>> + * Adjust the transformation matrix based on the Xinerama settings. This
>> + * function will attempt to map the given pointer to the Xinerama head
>> + * (which should be formatted as 'HEAD-N') specified in the first command-
>> + * line argument. If this function succeeds in modifying the transformation
>> + * matrix, it returns 'true'.
>> + *
>> + * For TwinView This would better be done with libXNVCtrl but until they
>> + * learn to package it properly, rely on Xinerama. Besides, libXNVCtrl isn't
>> * available on RHEL, so we'd have to do it through Xinerama there anyway.
>> */
>> -static void set_output_xinerama(Display *dpy, XDevice *dev, param_t *param,
>> int argc, char **argv)
>> +static Bool set_output_xinerama(Display *dpy, XDevice *dev, param_t *param,
>> int argc, char **argv)
>> {
>> - int event, error;
>> + int opcode, event, error;
>> + int maj, min;
>> XineramaScreenInfo *screens;
>> int nscreens;
>> int head;
>> + Bool success = False;
>> +
>> + if (XQueryExtension(dpy, "RANDR", &opcode, &event, &error) &&
>> + XRRQueryVersion(dpy, &maj, &min) && (maj * 1000 + min) >= 1002 &&
>> + !XQueryExtension(dpy, "NV-CONTROL", &opcode, &event, &error))
>> + {
>> + /* You have a new enough version of RandR, and aren't
>> + * subject to the NVIDIA driver bug mentioned in
>> + * set_output_randr.
>> + */
>> + TRACE("Working version of RandR present. Xinerama
>> disabled.\n");
>> + return False;
>> + }
>>
>> if (!XineramaQueryExtension(dpy, &event, &error))
>> {
>> fprintf(stderr, "Unable to set screen mapping. Xinerama
>> extension not found\n");
>> - return;
>> + return False;
>> }
>>
>> if (!convert_value_from_user(param, argv[0], &head))
>> {
>> fprintf(stderr, "Please specify the output name as HEAD-X,"
>> "where X is the screen number\n");
>> - return;
>> + return False;
>> }
>>
>> screens = XineramaQueryScreens(dpy, &nscreens);
>> @@ -2099,15 +2145,15 @@ static void set_output_xinerama(Display *dpy,
>> XDevice *dev, param_t *param, int
>> screens[head].x_org, screens[head].y_org,
>> screens[head].width, screens[head].height);
>>
>> + success = True;
>> +
>> out:
>> XFree(screens);
>> + return success;
>> }
>>
>> static void set_output(Display *dpy, XDevice *dev, param_t *param, int
>> argc, char **argv)
>> {
>> - int opcode, event, error;
>> - int maj, min;
>> -
>> if (argc == 0)
>> {
>> float matrix[9] = { 1, 0, 0,
>> @@ -2123,14 +2169,7 @@ static void set_output(Display *dpy, XDevice *dev,
>> param_t *param, int argc, cha
>> return;
>> }
>>
>> - /* Check for RandR 1.2. Server bug causes the NVIDIA driver to
>> - * report with RandR 1.3 support but it doesn't expose RandR CRTCs.
>> - * Force Xinerama if NV-CONTROL is present */
>> - if (XQueryExtension(dpy, "NV-CONTROL", &opcode, &event, &error) ||
>> - !XQueryExtension(dpy, "RANDR", &opcode, &event, &error) ||
>> - !XRRQueryVersion(dpy, &maj, &min) || (maj * 1000 + min) < 1002)
>> - set_output_xinerama(dpy, dev, param, argc, argv);
>> - else
>> + if (!set_output_xinerama(dpy, dev, param, argc, argv))
>> set_output_xrandr(dpy, dev, param, argc, argv);
>
> and now that I've read the other patches too, I think this approach may
> simply not be ideally suited. Why don't you split argument parsing into a
> separate function and then switch based on the return code. I think this
> would fit well with the goal. I'm thinking of something similar to
> convert_value_from_user, returning the type of output so that the set_output
> logic can be:
>
> enum OutputType output_type;
> char *output_name;
>
> output_type = convert_output_name(argc, argv, &output_name);
> switch(output_type) {
> case OUTPUT_TYPE_XINERAMA: set_output_xinerama(...) ...
> case OUTPUT_TYPE_XRANDR: set_output_xrandr(...) ...
> case OUTPUT_TYPE_INVALID: show error ...
> }
>
> the output parsing itself doesn't need to check for validity either, that
> can then be done in the respective handlers
>
I considered using a parser (it'd be cleaner, more logical, and fix
the issue of whether its better to TRACE or printf multiple functions
failing), but was having some difficulty coming up with something that
would handle the empty-string used for desktop mapping. I'll do some
more thinking... I'll probably just have to deny "true" and "false"
from being valid XRandR output names.
Jason
---
Day xee-nee-svsh duu-'ushtlh-ts'it;
nuu-wee-ya' duu-xan' 'vm-nvshtlh-ts'it.
Huu-chan xuu naa~-gha.
------------------------------------------------------------------------------
uberSVN's rich system and user administration capabilities and model
configuration take the hassle out of deploying and managing Subversion and
the tools developers use with it. Learn more about uberSVN and get a free
download at: http://p.sf.net/sfu/wandisco-dev2dev
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
