March 2017 - Wine-Devel - list.winehq.org

Re: [PATCH v2 2/5] winebus.sys: Build device reports for linux event devices

by Roderick Colenbrander

Hi Aric, I have had a quick look over this patch and have some initial feedback, maybe more later if time permits. For the absolute and relative axes don't hard code the minimum and maximum values for the report descriptors. These are not correct for many devices e.g. Xbox controllers use 16-bit absolute axes, while DS3/DS4 for example use 8-bit. In addition xbox controllers use signed data, while DS3/DS4 use unsigned (0-255)Also many devices use signed data. You should rely on the absinfo data returned by EVIOCGABS and base your input ranges on that. Thanks, Roderick On Mon, Feb 27, 2017 at 9:32 AM, Aric Stewart <aric(a)codeweavers.com> wrote: > > v2: Updates from Sebastian Lackner > > Signed-off-by: Aric Stewart <aric(a)codeweavers.com> > --- > dlls/winebus.sys/bus_udev.c | 438 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 436 insertions(+), 2 deletions(-) > > > > >

March 2, 2017

Re: [PATCH 2/2] comctl32/propsheet: Implement PSPCB_ADDREF/PSPCB_RELEASE notifications

by Dmitry Timoshkov

Nikolay Sivov <bunglehead(a)gmail.com> wrote: > >> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> > >> --- > >> dlls/comctl32/propsheet.c | 41 +++++++++++++++++++++++++++++++++++------ > >> dlls/comctl32/tests/propsheet.c | 4 ---- > >> 2 files changed, 35 insertions(+), 10 deletions(-) > > > > And of course it's a pure coincidence that you immediately started to work on > > PSPCB_ADDREF/PSPCB_RELEASE once a patch for it has been added to wine-staging. > > Why should or shouldn't it be? What does it have to do with a patch? I > worked on getting another propsheet fix tracked in wine-staging in, as > well as some of my own. Simply because I care about fixes getting into > Wine, you - apparently not so much. Apparently the fact that wine-staging is an official part of the Wine project means not that much for you. > > I'd suggest to stop such ill-behaved practices, or at least give a credit to > > the source if you decided to "borrow" an idea. Or is this an indication that > > you just ran out of your own ideas to work on? :) > > > > I would gladly give a credit for it, if it worked in a first place. > Since it couldn't possibly work, and behavior is well documented, I > don't think it qualifies as an idea on its own. The wine-staging patch worked well for an application it was tagetting (Process Hacker), what is an application name that it didn't work for? > Now, to get all the credit you can possibly get, because that's > obviously is the most important thing there is, there is a way that > you're well aware of. I just pointed out to an ill-behaving practices you're using, and asked to either stop doing such bad things, or give a credit to the source. Since the original wine-staging patch is pretty trivial it's not a big problem in this case, but taking into account your practice to "borrow" wine-staging work it may become a real problem at some point, at least as a distorted image of the company you are working for. P.S. Thanks for confirming that the wine-staging patch was the starting point of your work. -- Dmitry.

March 2, 2017

Re: [PATCH 2/2] comctl32/propsheet: Implement PSPCB_ADDREF/PSPCB_RELEASE notifications

by Nikolay Sivov

On 02.03.2017 6:13, Dmitry Timoshkov wrote: > Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > >> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> >> --- >> dlls/comctl32/propsheet.c | 41 +++++++++++++++++++++++++++++++++++------ >> dlls/comctl32/tests/propsheet.c | 4 ---- >> 2 files changed, 35 insertions(+), 10 deletions(-) > > And of course it's a pure coincidence that you immediately started to work on > PSPCB_ADDREF/PSPCB_RELEASE once a patch for it has been added to wine-staging. Why should or shouldn't it be? What does it have to do with a patch? I worked on getting another propsheet fix tracked in wine-staging in, as well as some of my own. Simply because I care about fixes getting into Wine, you - apparently not so much. > I'd suggest to stop such ill-behaved practices, or at least give a credit to > the source if you decided to "borrow" an idea. Or is this an indication that > you just ran out of your own ideas to work on? :) > I would gladly give a credit for it, if it worked in a first place. Since it couldn't possibly work, and behavior is well documented, I don't think it qualifies as an idea on its own. Now, to get all the credit you can possibly get, because that's obviously is the most important thing there is, there is a way that you're well aware of.

March 2, 2017

Re: [PATCH 2/2] comctl32/propsheet: Implement PSPCB_ADDREF/PSPCB_RELEASE notifications

by Dmitry Timoshkov

Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> > --- > dlls/comctl32/propsheet.c | 41 +++++++++++++++++++++++++++++++++++------ > dlls/comctl32/tests/propsheet.c | 4 ---- > 2 files changed, 35 insertions(+), 10 deletions(-) And of course it's a pure coincidence that you immediately started to work on PSPCB_ADDREF/PSPCB_RELEASE once a patch for it has been added to wine-staging. I'd suggest to stop such ill-behaved practices, or at least give a credit to the source if you decided to "borrow" an idea. Or is this an indication that you just ran out of your own ideas to work on? :) -- Dmitry.

March 2, 2017

Re: [PATCH v3 1/5] winebus.sys: Handle linux input event device via udev

by Aric Stewart

On 3/1/17 10:53 AM, Sebastian Lackner wrote: > I haven't done a full review yet, but here are some first comments. > > On 01.03.2017 17:16, Aric Stewart wrote: >> + subsystem = udev_device_get_subsystem(dev); >> usbdev = udev_device_get_parent_with_subsystem_devtype(dev, "usb", "usb_device"); >> if (usbdev) >> { >> + #ifdef HAS_PROPER_INPUT_HEADER >> + const platform_vtbl *other_vtbl = NULL; >> + DEVICE_OBJECT *dup = NULL; >> + if (strcmp(subsystem, "hidraw") == 0) >> + other_vtbl = &lnxev_vtbl; >> + else if (strcmp(subsystem, "input") == 0) >> + other_vtbl = &hidraw_vtbl; >> + >> + if (other_vtbl) >> + dup = bus_enumerate_hid_devices(other_vtbl, check_same_device, dev); > > Shouldn't we have some preference of either hidraw or linux event devices? > Otherwise this could lead to different results in each run. > Preference is much harder because we don't know what order the devices come in so we don't know if given an input device if we will ever get a readable hidraw device or not, and vice versa. It would involve having to get all the devices, waiting for things to settle and then deciding. This would mean that devices would not be initialized for wine apps until after this settle, and also it would mean we would have to do this collect, wait, decide on every plug in as well. It felt highly impractical. >> } >> >> +DEVICE_OBJECT* bus_enumerate_hid_devices(const platform_vtbl *vtbl, enum_func function, void* context) >> +{ >> + struct pnp_device *dev; >> + DEVICE_OBJECT *ret = NULL; >> + >> + TRACE("(%p)\n", vtbl); >> + >> + EnterCriticalSection(&device_list_cs); >> + LIST_FOR_EACH_ENTRY(dev, &pnp_devset, struct pnp_device, entry) >> + { >> + struct device_extension *ext = (struct device_extension *)dev->device->DeviceExtension; >> + if (ext->vtbl != vtbl) continue; >> + if (function(dev->device, context) == 0) >> + ret = dev->device; >> + break; > > This is wrong and will always abort after the first function call. > Doh! Too much python work recently. -aric >> + } >> + LeaveCriticalSection(&device_list_cs); >> + return ret; >> +} >> + > >

March 1, 2017

Re: [PATCH 3/6] wined3d: Handle raw thread group shared memory declarations.

by Józef Kucia

On Wed, Mar 1, 2017 at 5:49 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > On 28.02.2017 14:42, Józef Kucia wrote: >> + reg_maps->tgsm_count = reg_idx + 1; > > Wouldn't it be better to set it to max(reg_maps->tgsm_count, ...) to > allow decreasing reg indices (even if not seen in practice yet)? > Alternatively you could also get rid of this variable and update patch > 6 to check the index against the capacity. Yes, it would be better, but I think it doesn't matter in practice.

March 1, 2017

Re: [PATCH v3 1/5] winebus.sys: Handle linux input event device via udev

by Sebastian Lackner

I haven't done a full review yet, but here are some first comments. On 01.03.2017 17:16, Aric Stewart wrote: > + subsystem = udev_device_get_subsystem(dev); > usbdev = udev_device_get_parent_with_subsystem_devtype(dev, "usb", "usb_device"); > if (usbdev) > { > + #ifdef HAS_PROPER_INPUT_HEADER > + const platform_vtbl *other_vtbl = NULL; > + DEVICE_OBJECT *dup = NULL; > + if (strcmp(subsystem, "hidraw") == 0) > + other_vtbl = &lnxev_vtbl; > + else if (strcmp(subsystem, "input") == 0) > + other_vtbl = &hidraw_vtbl; > + > + if (other_vtbl) > + dup = bus_enumerate_hid_devices(other_vtbl, check_same_device, dev); Shouldn't we have some preference of either hidraw or linux event devices? Otherwise this could lead to different results in each run. > } > > +DEVICE_OBJECT* bus_enumerate_hid_devices(const platform_vtbl *vtbl, enum_func function, void* context) > +{ > + struct pnp_device *dev; > + DEVICE_OBJECT *ret = NULL; > + > + TRACE("(%p)\n", vtbl); > + > + EnterCriticalSection(&device_list_cs); > + LIST_FOR_EACH_ENTRY(dev, &pnp_devset, struct pnp_device, entry) > + { > + struct device_extension *ext = (struct device_extension *)dev->device->DeviceExtension; > + if (ext->vtbl != vtbl) continue; > + if (function(dev->device, context) == 0) > + ret = dev->device; > + break; This is wrong and will always abort after the first function call. > + } > + LeaveCriticalSection(&device_list_cs); > + return ret; > +} > +

March 1, 2017

Re: [PATCH 3/6] wined3d: Handle raw thread group shared memory declarations.

by Sebastian Lackner

On 28.02.2017 14:42, Józef Kucia wrote: > + reg_maps->tgsm_count = reg_idx + 1; Wouldn't it be better to set it to max(reg_maps->tgsm_count, ...) to allow decreasing reg indices (even if not seen in practice yet)? Alternatively you could also get rid of this variable and update patch 6 to check the index against the capacity. Best regards, Sebastian

March 1, 2017

Re: [PATCH 2/2] winmm: Dispatch MCI_BREAK to the driver

by Bruno Jesus

On Wed, Mar 1, 2017 at 11:58 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > On 01.03.2017 08:10, Bruno Jesus wrote: >> + TRACE("(%08x, %08X, vkey %04X\n", wDevID, dwFlags, lpParms->nVirtKey); > > Did you forget to add a ")" ? Yes, thanks ;-) I'll resend today later.

March 1, 2017

Re: [PATCH 2/2] winmm: Dispatch MCI_BREAK to the driver

by Sebastian Lackner

On 01.03.2017 08:10, Bruno Jesus wrote: > + TRACE("(%08x, %08X, vkey %04X\n", wDevID, dwFlags, lpParms->nVirtKey); Did you forget to add a ")" ?

March 1, 2017