[PATCH app/xdpyinfo v2] Use XRANDR 1.2 extension for reporting dimensions and resolution per output

[Giuseppe Bilotta]

Hello Pali,

I like the idea of this patch, wasn't aware of its existence, thanks
for pinging me about it. A few comments below:

On Tue, Apr 3, 2018 at 10:15 PM, Pali Rohár <pali.rohar at gmail.com> wrote:
>> @@ -455,27 +462,75 @@ print_screen_info(Display *dpy, int scr)
>>      double xres, yres;
>>      int ndepths = 0, *depths = NULL;
>>      unsigned int width, height;
>> -
>> -    /*
>> -     * there are 2.54 centimeters to an inch; so there are 25.4 millimeters.
>> -     *
>> -     *     dpi = N pixels / (M millimeters / (25.4 millimeters / 1 inch))
>> -     *         = N pixels / (M inch / 25.4)
>> -     *         = N * 25.4 pixels / M inch
>> -     */

You may want to keep this comment here, rather than in the else block below,
since the formula applies to any conversion, regardless if it's core
or per-output DPI.

>> -    xres = ((((double) DisplayWidth(dpy,scr)) * 25.4) /
>> -         ((double) DisplayWidthMM(dpy,scr)));
>> -    yres = ((((double) DisplayHeight(dpy,scr)) * 25.4) /
>> -         ((double) DisplayHeightMM(dpy,scr)));
>> +#ifdef XRANDR
>> +    int event_base, error_base;
>> +    int major, minor;
>> +    XRRScreenResources *res = NULL;
>> +    XRROutputInfo *output;
>> +    XRRCrtcInfo *crtc;
>> +#endif
>>
>>      printf ("\n");
>>      printf ("screen #%d:\n", scr);
>> -    printf ("  dimensions:    %dx%d pixels (%dx%d millimeters)\n",
>> -         XDisplayWidth (dpy, scr),  XDisplayHeight (dpy, scr),
>> -         XDisplayWidthMM(dpy, scr), XDisplayHeightMM (dpy, scr));
>> -    printf ("  resolution:    %dx%d dots per inch\n",
>> -         (int) (xres + 0.5), (int) (yres + 0.5));

I would unconditionally show the core output, regardless of RANDR
state. Even if it's fictitious when RANDR is enabled,
it can still be useful to spot inconsistencies. It also ensures that
the xdpyinfo output is "less" broken by this patch (search for
xdpyinfo grep resolution to get an idea of how used this is as a debug tool).

>> +
>> +#ifdef XRANDR
>> +    if (XRRQueryExtension (dpy, &event_base, &error_base) &&
>> +        XRRQueryVersion (dpy, &major, &minor) &&
>> +        (major > 1 || (major == 1 && minor >= 2)) &&
>> +        (res = XRRGetScreenResources (dpy, RootWindow (dpy, scr))))
>> +    {

I'm pondering whether it would be a good idea to print the RANDR
version here, something like:
printf("  RANDR (%d.%d) outputs:\n", major, minor);
and then nesting the per-output data even more.

>> +        for (i = 0; i < res->noutput; ++i) {
>> +            output = XRRGetOutputInfo (dpy, res, res->outputs[i]);
>> +            if (!output || !output->crtc || output->connection != RR_Connected)
>> +                continue;
>> +
>> +            crtc = XRRGetCrtcInfo (dpy, res, output->crtc);
>> +            if (!crtc) {
>> +                XRRFreeOutputInfo (output);
>> +                continue;
>> +            }
>> +
>> +            printf ("  output: %s\n", output->name);
>> +            printf ("    dimensions:    %ux%u pixels (%lux%lu millimeters)\n",
>> +                    crtc->width, crtc->height, output->mm_width, output->mm_height);
>> +
>> +            if (output->mm_width && output->mm_height) {
>> +                xres = ((((double) crtc->width) * 25.4) / ((double) output->mm_width));
>> +                yres = ((((double) crtc->height) * 25.4) / ((double) output->mm_height));
>> +            } else {
>> +                xres = 0;
>> +                yres = 0;
>> +            }
>> +            printf ("    resolution:    %dx%d dots per inch\n",
>> +                    (int) (xres + 0.5), (int) (yres + 0.5));

This doesn't work correctly when the display is rotated, since the
width/height in pixel will follow the orientation,
but the physical sizes won't. You should probably take that into
account, and possibly show the output orientation (e.g. in the
dimensions line, or in the name line if you do add the "RANDR
<version> outputs" header).

Also (please don't hate me), for RANDR versions 1.5 or higher we
should include RANDR monitors too (possibly in place of individual
outputs, I'm not sure about this actually, it depends on how detailed
we want to be; I would go with both, but then again I'm an information
junkie).

Cheers,

-- 
Giuseppe "Oblomov" Bilotta