[Merge] lp:~samuel-thibault/unity/shortcuts into lp:unity

[Marco Trevisan (Treviño)]

Thanks for this, looks sane... Check the comments  please.

Diff comments:

> === modified file 'unity-shared/InputMonitor.cpp'
> --- unity-shared/InputMonitor.cpp	2017-02-28 17:16:14 +0000
> +++ unity-shared/InputMonitor.cpp	2017-12-14 17:21:32 +0000
> @@ -154,6 +155,8 @@
>      {
>        LOG_ERROR(logger) << "Missing XInput, impossible to setup an InputMonitor";
>      }
> +
> +    basemasks = XIGetSelectedEvents (dpy, root, &num_basemasks);

Get off the space before the opening bracket (.
Also, please move this  inside a new else statement of the check if (XIQueryVersion(dpy, &maj, &min) == BadRequest).

>    }
>  
>    ~Impl()
> @@ -223,14 +227,53 @@
>      return events;
>    }
>  
> +  // Prepare a mask based on the Compiz-configured mask
> +  void InitializeMask(int deviceid, XIEventMask* ourmask)
> +  {
> +    int wanted_mask_len = XIMaskLen(XI_LASTEVENT);
> +    int mask_len = -1;
> +    unsigned char* mask;
> +
> +    for (int i = 0; i < num_basemasks; i++)
> +    {
> +      if (basemasks[i].deviceid == deviceid) {

Move bracket in new line

> +	// Compiz is already listening for some events on this device, use this as a base
> +
> +	// Compute final mask length

Get off this obiouvs comment

> +	mask_len = basemasks[i].mask_len;
> +	if (mask_len < wanted_mask_len)
> +	  // The existing mask is smaller than what we might want to set

Same here

> +	  mask_len = wanted_mask_len;
> +
> +	// Make a fresh copy of the existing mask

And again

> +	mask = (unsigned char*) malloc(mask_len);
> +	memcpy(mask, basemasks[i].mask, basemasks[i].mask_len);
> +	memset(mask + basemasks[i].mask_len, 0, mask_len - basemasks[i].mask_len);
> +	break;
> +      }
> +    }
> +
> +    if (mask_len < 0)
> +    {
> +      // No existing mask

Here too

> +      mask_len = wanted_mask_len;
> +      mask = (unsigned char*) calloc(mask_len, 1);
> +    }
> +
> +    ourmask->deviceid = deviceid;
> +    ourmask->mask_len = mask_len;
> +    ourmask->mask = mask;
> +  }
> +
>    void UpdateEventMonitor()
>    {
>      auto* nux_dpy = nux::GetGraphicsDisplay();
>      auto* dpy = nux_dpy ? nux_dpy->GetX11Display() : gdk_x11_get_default_xdisplay();
>      Window root = DefaultRootWindow(dpy);
>  
> -    unsigned char master_dev_bits[XIMaskLen(XI_LASTEVENT)] = { 0 };

Can't you keep this (call it master_dev_mask, if you prefer), and just associate this to master_dev.mask so that you don't have to do the dynamic allocation in InitializeMask?

> -    XIEventMask master_dev = { XIAllMasterDevices, sizeof(master_dev_bits), master_dev_bits };
> +    XIEventMask master_dev;
> +
> +    InitializeMask(XIAllMasterDevices, &master_dev);
>  
>      if (!barrier_callbacks_.empty())
>      {
> @@ -238,8 +281,9 @@
>        XISetMask(master_dev.mask, XI_BarrierLeave);
>      }
>  
> -    unsigned char all_devs_bits[XIMaskLen(XI_LASTEVENT)] = { 0 };

And same for this...

> -    XIEventMask all_devs = { XIAllDevices, sizeof(all_devs_bits), all_devs_bits };
> +    XIEventMask all_devs;
> +
> +    InitializeMask(XIAllDevices, &all_devs);
>  
>      if (!pointer_callbacks_.empty())
>      {
> @@ -374,6 +427,8 @@
>    EventCallbackSet key_callbacks_;
>    EventCallbackSet barrier_callbacks_;
>    EventCallbackSet removal_queue_;
> +  XIEventMask* basemasks;
> +  int num_basemasks;

Please add trailing '_' to the private variables (i.e. basemarks_).

Also they need to initialized to nullptr and 0 in the Impl constructor.

>  };
>  
>  Monitor::Monitor()


-- 
https://code.launchpad.net/~samuel-thibault/unity/shortcuts/+merge/335231
Your team Unity Team is requested to review the proposed merge of lp:~samuel-thibault/unity/shortcuts into lp:unity.