On Wed, Aug 29, 2012 at 9:44 PM, Peter Hutterer
<peter.hutte...@who-t.net> wrote:
>
> re-reviewing this with fresher eyes this time:
>
I appreciate it :)
> On Fri, Aug 24, 2012 at 09:51:46AM -0700, Jason Gerecke wrote:
>> On Thu, Aug 23, 2012 at 11:27 PM, Peter Hutterer
>> <peter.hutte...@who-t.net> wrote:
>> > On Thu, Aug 02, 2012 at 05:53:06PM -0700, Jason Gerecke wrote:
>> >> Tablets like the Graphire4 have mouse buttons present on the tablet
>> >> rather than ExpressKeys. Because mouse buttons are expected to work
>> >> in a certain way by default, it is insufficient for some clients to
>> >> know only that e.g. button 'A' has been pressed; they furthermore
>> >> need to know that it is expected to by default trigger e.g.
>> >> 'navigate-forward'.
>
> why do they need to know that?
> if the button is left as-is, it'll do what it does by default. otherwise,
> it'll do what it is mapped to. Why do we need the special casing here?
>
The one client I had specifically in mind with this patch was
xf86-input-wacom. Given an e.g. BTN_BACK kernel event, the driver has
no way of knowing the physical button which precipitated it. This
knowledge is necessary to "fix" the Bamboo/Graphire button ordering
issue in the X driver (which are the three other RFC patches I
posted).
The class of applications that need to know the default action
associated with a physical button is admittedly pretty small. The only
real use I can think of would be something like the g-c-c button
mapping dialog, providing the user with knowledge of what a button
will do if a custom action is removed.
> is it more useful to add some mapping that describes the default for any
> button (where applicable), regardless of what that button's scancode
> actually is?
>
I don't understand the question. Are you asking if it'd be more useful
to describe a default action for a button (e.g. "A = Back") instead of
a button for a default action (e.g. "Back = A")? If so, I imagine the
utility would be approximately the same -- ultimately the "default" is
probably going to be something scancode-like. Perhaps the capacitive
buttons on the 24HD/22HD are a counterexample, though...
Ultimately, we should be careful not to cross the line from
description to prescription. This patch already smells too much like
the latter IMO, which is part of the reason I posted it RFC.
>> >> Five mouse button types have been added: Left, Middle, Right,
>> >> Forward, and Back. To associate these types with buttons, data files
>> >> require a new "[MouseButtons]" section that accepts the previously-
>> >> mentioned types as keywords and button lists as values. See the
>> >> Graphire4 data file for example.
>
> doesn't need a new section, tbh, this could be added to the current buttons
> section. but if you plan to keep it separate, "DefaultMappings" (or
> something similar) would be a more explanatory name.
>
Sounds reasonable.
>> >>
>> >> Signed-off-by: Jason Gerecke <killert...@gmail.com>
>> >> ---
>> >> This is just one of several possible ways to augment libwacom with
>> >> mouse button data. If you think something would be better done
>> >> differently, please let me know -- I don't work with libwacom much
>> >> myself :)
>> >>
>> >> data/graphire4-4x5.tablet | 4 +++
>> >> libwacom/libwacom-database.c | 76
>> >> +++++++++++++++++++++++++++++++++++++++-----
>> >> libwacom/libwacom.h | 9 ++++++
>> >> test/load.c | 9 ++++++
>> >> 4 files changed, 90 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/data/graphire4-4x5.tablet b/data/graphire4-4x5.tablet
>> >> index b93c65b..84aa7cd 100644
>> >> --- a/data/graphire4-4x5.tablet
>> >> +++ b/data/graphire4-4x5.tablet
>> >> @@ -29,3 +29,7 @@ Buttons=2
>> >>
>> >> [Buttons]
>> >> Top=A;B
>> >> +
>> >> +[MouseButtons]
>> >> +Back=A
>> >> +Forward=B
>> >> diff --git a/libwacom/libwacom-database.c b/libwacom/libwacom-database.c
>> >> index 5468975..e24f967 100644
>> >> --- a/libwacom/libwacom-database.c
>> >> +++ b/libwacom/libwacom-database.c
>> >> @@ -41,6 +41,7 @@
>> >> #define FEATURES_GROUP "Features"
>> >> #define DEVICE_GROUP "Device"
>> >> #define BUTTONS_GROUP "Buttons"
>> >> +#define MOUSEBUTTONS_GROUP "MouseButtons"
>> >>
>> >> static WacomClass
>> >> libwacom_class_string_to_enum(const char *class)
>> >> @@ -246,6 +247,26 @@ struct {
>> >> { "OLEDs", WACOM_BUTTON_OLED }
>> >> };
>> >>
>> >> +static int
>> >> +libwacom_button_to_index (const char *button)
>> >> +{
>> >> + char val;
>> >> +
>> >> + if (button == NULL)
>> >> + return -1;
>> >> +
>> >> + val = *button;
>> >> + if (strlen (button) > 1 ||
>> >> + val < 'A' ||
>> >> + val > 'Z') {
>> >> + return -2;
>> >> + }
>> >> + val -= 'A';
>> >> +
>> >> + g_assert (val >= 0);
>> >> + return val;
>> >> +}
>> >> +
>> >> static void
>> >> libwacom_parse_buttons_key(WacomDevice *device,
>> >> GKeyFile *keyfile,
>> >> @@ -259,17 +280,12 @@ libwacom_parse_buttons_key(WacomDevice *device,
>> >> if (vals == NULL)
>> >> return;
>> >> for (i = 0; vals[i] != NULL; i++) {
>> >> - char val;
>> >> -
>> >> - val = *vals[i];
>> >> - if (strlen (vals[i]) > 1 ||
>> >> - val < 'A' ||
>> >> - val > 'Z') {
>> >> + int index = libwacom_button_to_index(vals[i]);
>> >> + if (index < 0) {
>> >> g_warning ("Ignoring value '%s' in key '%s'",
>> >> vals[i], key);
>> >> continue;
>> >> }
>> >> - val -= 'A';
>> >> - device->buttons[(int) val] |= flag;
>> >> + device->buttons[index] |= flag;
>> >> }
>> >>
>> >> g_strfreev (vals);
>> >> @@ -308,6 +324,49 @@ libwacom_parse_buttons(WacomDevice *device,
>> >> device->strips_num_modes = libwacom_parse_num_modes(device,
>> >> keyfile, "StripsNumModes", WACOM_BUTTON_TOUCHSTRIP_MODESWITCH);
>> >> }
>> >>
>> >> +struct {
>> >> + const char *key;
>> >> + WacomMouseButton button;
>> >> +} mouseoptions[] = {
>> >> + { "Left", WMOUSE_LEFT },
>> >> + { "Middle", WMOUSE_MIDDLE },
>> >> + { "Right", WMOUSE_RIGHT },
>> >> + { "Forward", WMOUSE_FORWARD },
>> >> + { "Back", WMOUSE_BACK },
>> >> +};
>> >> +
>> >> +static void
>> >> +libwacom_parse_mousebuttons_key (WacomDevice *device,
>> >> + GKeyFile *keyfile,
>> >> + const char *key,
>> >> + WacomMouseButton button)
>> >> +{
>> >> + guint i;
>> >> + char **vals;
>> >> +
>> >> + vals = g_key_file_get_string_list (keyfile, MOUSEBUTTONS_GROUP,
>> >> key, NULL, NULL);
>> >> + if (vals == NULL)
>> >> + return;
>> >> + for (i = 0; vals[i] != NULL; i++) {
>> >> + int index = libwacom_button_to_index (vals[i]);
>> >> + if (index < 0) {
>> >> + g_warning ("Ignoring value '%s' in key '%s'",
>> >> vals[i], key);
>> >> + continue;
>> >> + }
>> >> + device->buttons[index] |= button;
>> >> + }
>> >> +}
>> >> +
>> >> +static void
>> >> +libwacom_parse_mousebuttons (WacomDevice *device,
>> >> + GKeyFile *keyfile)
>> >> +{
>> >> + guint i;
>> >> +
>> >> + for (i = 0; i < G_N_ELEMENTS (mouseoptions); i++)
>> >> + libwacom_parse_mousebuttons_key (device, keyfile,
>> >> mouseoptions[i].key, mouseoptions[i].button);
>> >> +}
>> >> +
>> >> static WacomDevice*
>> >> libwacom_parse_tablet_keyfile(const char *path)
>> >> {
>> >> @@ -412,6 +471,7 @@ libwacom_parse_tablet_keyfile(const char *path)
>> >> if (device->num_buttons > 0) {
>> >> device->buttons = g_new0 (WacomButtonFlags,
>> >> device->num_buttons);
>> >> libwacom_parse_buttons(device, keyfile);
>> >> + libwacom_parse_mousebuttons(device, keyfile);
>> >> }
>> >>
>> >> out:
>> >> diff --git a/libwacom/libwacom.h b/libwacom/libwacom.h
>> >> index f7e6cf9..463d93b 100644
>> >> --- a/libwacom/libwacom.h
>> >> +++ b/libwacom/libwacom.h
>> >> @@ -145,6 +145,14 @@ typedef enum {
>> >> WSTYLUS_PUCK
>> >> } WacomStylusType;
>> >>
>> >> +typedef enum {
>> >> + WMOUSE_LEFT = (1 & 0xf) << 10,
>> >> + WMOUSE_MIDDLE = (2 & 0xf) << 10,
>> >> + WMOUSE_RIGHT = (3 & 0xf) << 10,
>> >> + WMOUSE_BACK = (8 & 0xf) << 10,
>> >> + WMOUSE_FORWARD = (9 & 0xf) << 10
>> >> +} WacomMouseButton;
>
> btw, the 0xf is superfluous, it won't do anything. and I'm not sure what
> you're planing to do here, these values won't work as bitmasks but you
> appear to be setting them as bitmasks in libwacom_parse_mousebuttons.
>
These are constants that can be stored in the 4 bits reserved by
WACOM_BUTTON_MOUSEBUTTON. They let you so stuff like:
WacomButtonFlags flags = libwacom_get_button_flag(dev, 'A');
if (flags & WACOM_BUTTON_MOUSEBUTTON == WMOUSE_LEFT) {
// Button 'A' is the left-mouse button
}
The 0xf mirrors the 0xf of WACOM_BUTTON_MOUSEBUTTON and is a gentle
reminder that you've got 4 bits to play with.
>> >> +
>> >> /**
>> >> * Capabilities of the various tablet buttons
>> >> */
>> >> @@ -159,6 +167,7 @@ typedef enum {
>> >> WACOM_BUTTON_TOUCHSTRIP_MODESWITCH = (1 << 7),
>> >> WACOM_BUTTON_TOUCHSTRIP2_MODESWITCH = (1 << 8),
>> >> WACOM_BUTTON_OLED = (1 << 9),
>> >> + WACOM_BUTTON_MOUSEBUTTON = (0xf << 10), /* 10, 11, 12,
>> >> 13 */
>
> btw, just noticing this: if you have to explain your bitmasks, you might as
> well use the numbers directly
>
That's true, though (as is often the case) I didn't think it'd be
confusing :D Storing the mousebutton in a new struct is going to be
more complicated, but probably a little more logical in the end.
>> >
>> > Do we need to double-abstract this? i.e. one bit for "mouse button
>> > capability" and then an interface for getting the list of actual buttons
>> > present? bit worried here about ending up too many bits for the buttons if
>> > we start adding more meanings in the future.
>> >
>> I was worried about this as well. We could store the mousebutton info
>> elsewhere and provide a function "getMouseButton(char physicalButton)"
>> (ignore naming style for the moment -- I haven't bothered to check
>> that its consistent) that will return the mouse button that the kernel
>> will send for a given physical button identifier.
>>
>> > Then again, the Graphire's are an older series, so I suspect this is
>> > outdated now by the expresskeys?
>> >
>> The Graphires used to report BTN_n "expresskey" buttons like any other
>> tablet, but were then changed to report mouse buttons to bring them in
>> line with the original Bamboo Touch (which apparently was first
>> written to work with xf86-input-synaptics).
>>
>> Actually, I'm surprised at how recently this changeover occurred:
>> Linux 3.1. This shouldn't be a problem for the faster-releasing
>> distros like Fedora and Ubuntu (both of which only use newer kernels
>> in currently-supported releases), but may cause hiccoughs in some
>> other environments.
>
> I'm still unsure on how you actually want to use this. can you knock up some
> client pseudo-code on how you expect this to be used?
>
Look at "[PATCH RFC 3/3] Switch to use of libwacom for pad mouse
button data" -- I'm using this there to figure out the physical button
associated with "mouse" button events from the kernel.
Jason
---
Day xee-nee-svsh duu-'ushtlh-ts'it;
nuu-wee-ya' duu-xan' 'vm-nvshtlh-ts'it.
Huu-chan xuu naa~-gha.
> Cheers,
> Peter
>
>> >
>> >> WACOM_BUTTON_MODESWITCH = (WACOM_BUTTON_RING_MODESWITCH
>> >> | WACOM_BUTTON_RING2_MODESWITCH | WACOM_BUTTON_TOUCHSTRIP_MODESWITCH |
>> >> WACOM_BUTTON_TOUCHSTRIP2_MODESWITCH),
>> >> WACOM_BUTTON_DIRECTION = (WACOM_BUTTON_POSITION_LEFT |
>> >> WACOM_BUTTON_POSITION_RIGHT | WACOM_BUTTON_POSITION_TOP |
>> >> WACOM_BUTTON_POSITION_BOTTOM),
>> >> WACOM_BUTTON_RINGS_MODESWITCH = (WACOM_BUTTON_RING_MODESWITCH
>> >> | WACOM_BUTTON_RING2_MODESWITCH),
>> >> diff --git a/test/load.c b/test/load.c
>> >> index b449fa6..48dcd15 100644
>> >> --- a/test/load.c
>> >> +++ b/test/load.c
>> >> @@ -121,6 +121,15 @@ int main(int argc, char **argv)
>> >> assert(device);
>> >> assert(libwacom_is_builtin(device));
>> >>
>> >> + libwacom_destroy(device);
>> >> +
>> >> + device = libwacom_new_from_name(db, "Wacom Graphire4 4x5", NULL);
>> >> + assert(device);
>> >> + assert(libwacom_get_button_flag(device, 'A') & WMOUSE_BACK);
>> >> + assert(libwacom_get_button_flag(device, 'B') & WMOUSE_FORWARD);
>> >> +
>> >> + libwacom_destroy(device);
>> >> +
>> >> libwacom_database_destroy (db);
>> >>
>> >> return 0;
>> >> --
>> >> 1.7.11.4
>> >>
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
