January 2013 - Wine-Devel - list.winehq.org

Re: macdrv: implement getting and setting the screen saver state, version 3
by Ken Thomases Jan. 21, 2013

Jan. 21, 2013

On Jan 21, 2013, at 11:50 AM, C.W. Betts wrote: > This version gets rid of redundant comments. According to Alexandre's comment on your X11 driver patch, you should be returning FALSE from the "SET" case so that user32 will update the registry. Otherwise, looking good. -Ken

1 0

Re: [PATCH] winebuild: Use Clang to assemble if found.
by Alexandre Julliard Jan. 21, 2013

Jan. 21, 2013

Charles Davis <cdavis5x(a)gmail.com> writes: > @@ -319,26 +341,27 @@ char *find_tool( const char *name, const char * const *names ) > names = alt_names; > } > > - while (*names) > + /* If we have a target triple, we really need to try the list of names > + * twice: once with the triple, and once without. > + */ This doesn't make sense, if we have a target it means we are cross-compiling, and we need to use the correct tool. -- Alexandre Julliard julliard(a)winehq.org

1 0

Re: macdrv: implement getting and setting the screen saver state, version 2
by Ken Thomases Jan. 21, 2013

Jan. 21, 2013

Hi, On Jan 20, 2013, at 2:00 PM, C.W. Betts wrote: > This version implements changes and advice from Ken Thomases, including using kIOPMAssertionTypePreventUserIdleDisplaySleep on Lion and later. Also, some comments were added. > + //Get pre-Lion no display sleep counts C++-style comments ("//") are not allowed in Wine C code. They aren't portable. > + CFNumberRef count = CFDictionaryGetValue(assertionStates, kIOPMAssertionTypeNoDisplaySleep); > + CFNumberRef count2 = NULL; > + long longCount = 0; > + long longCount2 = 0; > + CFNumberGetValue(count, kCFNumberLongType, &longCount); > + //If available, get Lion and later no display sleep counts > + count2 = CFDictionaryGetValue(assertionStates, kIOPMAssertionTypePreventUserIdleDisplaySleep); > + if (count2) > + CFNumberGetValue(count2, kCFNumberLongType, &longCount2); The issue discussed for the previous patch is still present. For example, the following code would be internally consistent and also safe: CFNumberRef count = CFDictionaryGetValue(assertionStates, kIOPMAssertionTypeNoDisplaySleep); CFNumberRef count2 = CFDictionaryGetValue(assertionStates, kIOPMAssertionTypePreventUserIdleDisplaySleep); long longCount = 0; long longCount2 = 0; if (count) CFNumberGetValue(count, kCFNumberLongType, &longCount); if (count2) CFNumberGetValue(count2, kCFNumberLongType, &longCount2); (Note that this reorganization also eliminates a dead store. You were initializing count2 and then unconditionally assigning it, making the initialization redundant. Such unnecessary initializations also defeat potentially valuable compiler warnings.) > + if (longCount || longCount2 ) There's an extraneous space before the closing parenthesis. > + //Release power assertion > + IOReturn success = IOPMAssertionRelease(powerAssertion); The comment is useless. It tells us nothing that the code doesn't. Please don't do that. In fact, most of the comments you added are like this. > + //Are we running Lion or later? > + if (kCFCoreFoundationVersionNumber >= kCFCoreFoundationVersionNumber10_7) > + //If so, use Lion's name > + assertName = kIOPMAssertionTypePreventUserIdleDisplaySleep; > + else > + //If not, use old name > + assertName = kIOPMAssertionTypeNoDisplaySleep; I'm not sure it's correct that these are two different names for roughly the same thing. I think the two assertion types do slightly different things. "NoDisplaySleep" seeks to prevent display sleep for any (non-forced) reason. "PreventUserIdleDisplaySleep" only seeks to prevent display sleep that is due to user idleness. As I suggested previously, I do want us to use PreventUserIdleDisplaySleep when it's available, so that's good. (Thank you for making that change.) What I'm saying now is mostly just that the comments are misleading. I suggest just removing the comments within the "if". The code is understandable without them. Also, this bit of code uses tabs to indent when none of the rest does. Please be consistent with the surrounding code. Thanks, Ken

2 2

Re: configure.ac: check for a good enough version of libv4l1.h (try 2)
by Alexandre Julliard Jan. 21, 2013

Jan. 21, 2013

Damjan Jovanovic <damjan.jov(a)gmail.com> writes: > @@ -676,6 +676,22 @@ AC_CHECK_HEADERS([linux/videodev.h linux/videodev2.h libv4l1.h],,, > #include <asm/types.h> > #endif]) > > +dnl **** Check for libv4l1.h **** > +if test "x$with_v4l" != "xno" > +then > + ac_save_CPPFLAGS="$CPPFLAGS" > + if test "$PKG_CONFIG" != "false" > + then > + ac_libv4l1_libs="`$PKG_CONFIG --libs libv4l1 2>/dev/null`" > + ac_libv4l1_cflags="`$PKG_CONFIG --cflags libv4l1 2>/dev/null`" > + CPPFLAGS="$CPPFLAGS $ac_libv4l1_cflags" > + AC_CHECK_TYPES([struct video_picture],,,[#include <libv4l1.h>]) > + fi > + CPPFLAGS="$ac_save_CPPFLAGS" > + WINE_NOTICE_WITH(libv4l1,[test "x$ac_cv_type_struct_video_picture" = "x"], > + [libv4l1 ${notice_platform}development files not found or too old.]) > +fi It doesn't make sense to use pkg-config for the test since we don't use it when including the header. -- Alexandre Julliard julliard(a)winehq.org

1 0

Re: [1/2] netstat: Implement the TCP stats output
by Frédéric Delanoy Jan. 21, 2013

Jan. 21, 2013

On Sun, Jan 20, 2013 at 8:09 PM, André Hentschel <nerv(a)dawncrow.de> wrote: > --- > + IDS_TCP_SEGM_RECV, "Segments Recieved" Typo here: should be "Received" (same issue on the 2nd patch) Frédéric

1 0

Re: macdrv: implement getting and setting the screen saver state.
by Ken Thomases Jan. 21, 2013

Jan. 21, 2013

Hi, On Jan 19, 2013, at 4:08 PM, C.W. Betts wrote: > This implements getting and setting the screen saver state on the Mac Wine driver. Thanks for your contribution to the Mac driver. There are some issues with the patch: > + CFDictionaryRef assertsionStats = NULL; That variable name has a misspelling. It should probably be "assertionStates". > + CFNumberRef count = CFDictionaryGetValue(assertsionStats, kIOPMAssertionTypeNoDisplaySleep); > + CFNumberRef count2 = NULL; > + long longCount = 0; > + long longCount2 = 0; > + CFNumberGetValue(count, kCFNumberLongType, &longCount); > + count2 = CFDictionaryGetValue(assertsionStats, kIOPMAssertionTypePreventUserIdleDisplaySleep); > + if(count2) > + CFNumberGetValue(count2, kCFNumberLongType, &longCount2); Why are the two assertion types handled differently here? You get the value in the initializer in one case but not in the other. You check if the CFNumber pointer is NULL in one case but not the other. They should probably both be handled the same way. Some better variable names would be nice, too. > + if (longCount + longCount2 > 0) If think it's better to use "if (longCount || longCount2)". We're really interested in whether or not either is non-zero. If, somehow, the sum overflows or one is a large negative value, we still want to consider that as the screen saver being disabled. > + { > + *(BOOL *)ptr_param = FALSE; > + } It's a minor style preference, but I prefer that single-statement bodies for "if"s not have braces. > + if(success != kIOReturnSuccess) Another style nitpick: please put a space between "if" and the condition. That applies to the "if(count2)" above, too. > + /* > + Introduced in 10.6, not available in 10.5 > + IOReturn success = IOPMAssertionCreateWithName( > + kIOPMAssertionTypeNoDisplaySleep, kIOPMAssertionLevelOn, > + CFSTR("Wine Process requesting no screen saver"), &powerAssertion); > + */ To Alexandre's chagrin, the Mac driver requires 10.6. So, you might as well use the newer function. > + IOReturn success = IOPMAssertionCreate(kIOPMAssertionTypeNoDisplaySleep, > + kIOPMAssertionLevelOn, &powerAssertion); We should use kIOPMAssertionTypePreventUserIdleDisplaySleep instead on OS versions where it's available. We could either check a framework version variable (e.g. kCFCoreFoundationVersionNumber) or check if the dictionary returned from IOPMCopyAssertionsStatus() contains kIOPMAssertionTypePreventUserIdleDisplaySleep as a key. > + break; This seems extraneous. Cheers, Ken

2 2

wiki
by Rico Schüller Jan. 20, 2013

Jan. 20, 2013

Hi, is anyone able to get the wiki up again? Somehow the wiki (http://wiki.winehq.org/) is not reachable from here. Cheers Rico

4 3

Re: [3/6] windowscodecs: Implement tiff encoder options (TiffCompressionMethod and CompressionQuality)
by Dmitry Timoshkov Jan. 20, 2013

Jan. 20, 2013

Ludger Sprenker <ludger(a)sprenker.net> wrote: > + read_hr = IPropertyBag2_Read(pIEncoderOptions, 2, pb, NULL, v, hr); > + if (SUCCEEDED(hr)) Looks like a typo. -- Dmitry.

2 1

Re: [3/5] comctl32/listview: always use large icon size when calculating icon spacing
by Marvin Jan. 20, 2013

Jan. 20, 2013

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=24024 Your paranoid android. === WINEBUILD (build) === Patch failed to apply

2 1

Re: [5/5] comctl32/listview: fix LVM_SETICONSPACING on 64bit machines
by Marvin Jan. 20, 2013

Jan. 20, 2013

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=24026 Your paranoid android. === WINEBUILD (build) === Patch failed to apply

1 0

Re: [4/5] comctl32/listview: do not touch icon spacing if set explicitly
by Marvin Jan. 20, 2013

Jan. 20, 2013

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=24025 Your paranoid android. === WINEBUILD (build) === Patch failed to apply

1 0

User driver function conventions
by C.W. Betts Jan. 19, 2013

Jan. 19, 2013

I'm wondering what the user driver functions GetScreenSaveActive and SetScreenSaveActive are supposed to do. I have an implementation against CodeWeaver's changes to Wine and I just want to make sure that I'm making the functions properly. First is GetScreenStateActive. Does it check for if the screen saver is disabled for the whole system or just the app. And SetScreenStateActive. Is it supposed to set the activity for the whole system (I.E. if two apps call SetScreenStateActive to yes, then one to no, does the screen saver run) or is it per-app?

2 3

Re: [1/6] windowscodecs: Implementation of IPropertyBag2
by Vincent Povirk Jan. 19, 2013

Jan. 19, 2013

You'll probably have to split this into smaller patches. + return E_FAIL; /* FIXME: Function is not atomar on error, but MSDN does not say anything about it */ I don't understand. Are you saying native leaks memory on error, and if so how do you know this? + FIXME("Application tried to set the unknown option %s. " + "This may be an application error but is probably caused by an incomplete implementation in wine\n", + debugstr_w(pPropBag[i].pstrName)); I don't think the extra explanation of why this is a FIXME adds anything. + if (This->properties[idx].vt != V_VT(pvarValue+i)) + return E_FAIL; Are you sure this shouldn't be converted? + while (TRUE) + { + CoTaskMemFree( pPropBag[--i].pstrName ); + if(i == 0) + break; + } This could be written more simply as a do/while loop. +extern HRESULT CreateEncoderPropertyBag(PROPBAG2 *options, UINT count, IPropertyBag2 **property) DECLSPEC_HIDDEN; This adds code that's not really used. You might want to start out by changing the arguments of CreatePropertyBag2 and adding the IWICComponentFactory method. Then you can write tests first and remove todo's as you go. I don't think there's any reason to keep a constructor with the old set of arguments around.

1 0

winetestbot back online, for now
by Maarten Lankhorst Jan. 19, 2013

Jan. 19, 2013

Hey, It looks like the vmware licensing issues are resolved, so all testbot vm's are online again. I hope the replacement testbot is coming along nicely, because winetestbot is still scheduled to go permanently offline in may. ~Maarten

1 0

Truncate MIDI SysEx messages after termination byte
by Joerg-Cyril.Hoehle＠t-systems.com Jan. 18, 2013

Jan. 18, 2013

Hi, Johannes Kroll wrote: >+ for(i = 0; i < lpMidiOutHdr->dwBufferLength; i++) >+ if((unsigned char)lpMidiOutHdr->lpData[i] == 0xF7 && i < lpMidiOutHdr->dwBufferLength-1) What about for(i = 0; i+1 < lpMidiOutHdr->dwBufferLength, i++) without the redundant second if(&&) half? I'm always happy when somebody with real MIDI HW speaks up, because I'm not convinced that the Wine MIDI code does TRT about SysEx -- based on my readings, not experience, as I own no MIDI device... In particular, do you have any experience with multi-part SysEx messages (e.g. large data, such as uploads)? Still, I'm not persuaded that your patch is at the right place. I believe the midi* functions should be tiny wrappers around MODM_* messages, same for the wave* functions. Every time I see somebody attempt to violate this 1:1 mapping, I'm suspicious. Perhaps the logic that you're adding belongs to the individual wine*.drv/midi.c? Do you get to see this warning from winealsa.drv where it specifically checks for a terminating F7 (but not intermediate F7)? http://source.winehq.org/source/dlls/winealsa.drv/midi.c#L969 964 /* FIXME: MS doc is not 100% clear. Will lpData only contain system exclusive 965 * data, or can it also contain raw MIDI data, to be split up and sent to 966 * modShortData() ? 967 * If the latest is true, then the following WARNing will fire up 968 */ 969 if (lpData[0] != 0xF0 || lpData[lpMidiHdr->dwBufferLength - 1] != 0xF7) { 970 WARN("Alleged system exclusive buffer is not correct\n\tPlease report with MIDI file\n"); 971 lpNewData = HeapAlloc(GetProcessHeap(), 0, lpMidiHdr->dwBufferLength + 2); 972 } Therefore, I'd be happy if you could invest some more time and check, based on your real MIDI HW, and perhaps native w* machines, whether MODM_LONGDATA and midiOutLongMsg are equivalent or whether midiOut* does some additional processing. Regards, Jörg Höhle

3 6

Re: include: Make sure interlocked_cmpxchg128 is defined on ARM64
by André Hentschel Jan. 18, 2013

Jan. 18, 2013

Am 18.01.2013 18:05, schrieb André Hentschel: > --- > include/wine/port.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/wine/port.h b/include/wine/port.h > index bd6f513..d6904c0 100644 > --- a/include/wine/port.h > +++ b/include/wine/port.h > @@ -435,7 +435,7 @@ extern __int64 interlocked_cmpxchg64( __int64 *dest, __int64 xchg, __int64 compa > extern int interlocked_xchg( int *dest, int val ); > extern void *interlocked_xchg_ptr( void **dest, void *val ); > extern int interlocked_xchg_add( int *dest, int incr ); > -#ifdef _WIN64 > +#if defined(__x86_64__) || defined(__aarch64__) || defined(_WIN64) > extern unsigned char interlocked_cmpxchg128( __int64 *dest, __int64 xchg_high, > __int64 xchg_low, __int64 *compare ); > #endif > > > > oops that's not fully correct, I'll fix it next week, not a bad warning. -- Best Regards, André Hentschel

1 0

Re: qcap: Fix compilation on older systems.
by Damjan Jovanovic Jan. 18, 2013

Jan. 18, 2013

What does this patch do? libv4l1.h includes libv4l1-videodev.h which is a copy of linux/videodev.h. You should only need the one or the other. Are you saying there is a system where libv4l1.h is present but incomplete, and you need to include linux/videodev.h together with it? Thank you Damjan On Tue, Jan 15, 2013 at 6:47 AM, Dmitry Timoshkov <dmitry(a)baikal.ru> wrote: > --- > dlls/qcap/v4l.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/dlls/qcap/v4l.c b/dlls/qcap/v4l.c > index f5baaeb..2562c48 100644 > --- a/dlls/qcap/v4l.c > +++ b/dlls/qcap/v4l.c > @@ -47,7 +47,8 @@ > #endif > #ifdef HAVE_LIBV4L1_H > #include <libv4l1.h> > -#elif defined(HAVE_LINUX_VIDEODEV_H) > +#endif > +#ifdef HAVE_LINUX_VIDEODEV_H > #include <linux/videodev.h> > #endif > #ifdef HAVE_UNISTD_H > -- > 1.8.0.2 > > >

3 4

Re: configure.ac: check for a recent enough libv4l1.h
by Alexandre Julliard Jan. 18, 2013

Jan. 18, 2013

Damjan Jovanovic <damjan.jov(a)gmail.com> writes: > Changelog: > * configure.ac: check for a recent enough libv4l1.h > > libv4l1.h can only be used instead of linux/videodev.h as of version > 0.8.3, so check for the version using pkg-config (the only possible > way) and don't try to compile with older versions. This fixes > compilation on systems like FreeBSD 8.2, where there is no > linux/videodev.h, and libv4l1.h is too old. You should check for the definitions we need, not the version number. -- Alexandre Julliard julliard(a)winehq.org

1 0

Re: [PATCH] winebuild: Use Clang to assemble if its integrated assembler is being used. (try 2) (resend)
by Alexandre Julliard Jan. 18, 2013

Jan. 18, 2013

Charles Davis <cdavis5x(a)gmail.com> writes: > Try 2: Clarify that this is a Mac OS-only change. Don't use $CC as the assembler. Use the right flags to select 32/64-bit assembly with Clang. > > The configure script invokes the system compiler to figure out if .cfi > pseudo-ops are supported, on the assumption that it will invoke the > system assembler. Winebuild, on the other hand, invokes the system > assembler directly. This caused a problem on Mac OS hosts when compiling > with Clang: configure would detect that .cfi ops were supported, because > Clang's integrated assembler supports them; however, when winebuild then > emitted assembly with .cfi ops in them, the system assembler would then > choke. This patch makes winebuild invoke Clang's assembler if a) we are > on Mac OS, b) $CC is detected to be Clang, and c) it is using (whether > by request or by default) its built-in assembler. You can't use ifdefs in winebuild. This would have to be a runtime check. -- Alexandre Julliard julliard(a)winehq.org

1 0

Re: [PATCH 2/3] vbscript: Added VBScript.RegExp version 5.5 typelib
by Jacek Caban Jan. 18, 2013

Jan. 18, 2013

Hi Piotr, On 01/18/13 11:48, Piotr Caban wrote: > +#define DISPID_SUBMATCHES_ITEM 0 > +#define DISPID_SUBMATCHES_COUNT 1 > +#define DISPID_SUBMATCHES__NEWENUM -4 > + > +#define DISPID_MATCHCOLLECTION_ITEM 0 > +#define DISPID_MATCHCOLLECTION_COUNT 1 > +#define DISPID_MATCHCOLLECTION__NEWENUM -4 > + > +#define DISPID_MATCH_VALUE 0 Please use DISPID_VALUE and DISPID_NEWENUM instead. Jacek

1 0

Re: [PATCH 3/3] msvcrt: Added basic _popen tests (try2)
by Marvin Jan. 18, 2013

Jan. 18, 2013

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=23996 Your paranoid android. === WINEBUILD (build) === Patch failed to apply

1 0

Re: [PATCH 3/3] msvcrt: Added basic _popen tests
by Marvin Jan. 18, 2013

Jan. 18, 2013

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=23995 Your paranoid android. === WNT4WSSP6 (32 bit misc) === misc.c:340: Test failed: errno = -559038737 === W2KPROSP4 (32 bit misc) === misc.c:340: Test failed: errno = -559038737 === W2K3R2SESP2 (32 bit misc) === misc.c:340: Test failed: errno = -559038737

1 0

Re: [PATCH 3/3] d3dx9_36: Make variables static in xfile tests.
by Marvin Jan. 18, 2013

Jan. 18, 2013

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=23990 Your paranoid android. === WINEBUILD (build) === Patch failed to apply

1 0

Re: user32/tests: Test standard scrollbar's initialization (try 2).
by Sergey Guralnik Jan. 18, 2013

Jan. 18, 2013

On 2013-01-15 12:27, Sergey Guralnik wrote: > I have no feedback yet. Is it not clear enough? -- Sergey

1 0

Re: po: Update Russian translation
by Dmitry Timoshkov Jan. 18, 2013

Jan. 18, 2013

Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > #: jscript.rc:40 > msgid "Can't have 'break' outside of loop" > -msgstr "Выражение 'break' не может находится вне цикла" > +msgstr "Оператор 'break' не может находится вне цикла" > > #: jscript.rc:41 > msgid "Can't have 'continue' outside of loop" > -msgstr "Выражение 'continue' не может находится вне цикла" > +msgstr "Оператор 'continue' не может находится вне цикла" It would be also nice to fix obvious spelling errors, it really hurts the eyes :) - "не может находит[ь]ся", and run spell checker for the whole translation. -- Dmitry.

1 0