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

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

21 Jan '13

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 20 Jan '13

20 Jan '13

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 20 Jan '13

20 Jan '13

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 20 Jan '13

20 Jan '13

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 20 Jan '13

20 Jan '13

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 20 Jan '13

20 Jan '13

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 19 Jan '13

19 Jan '13

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 19 Jan '13

19 Jan '13

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 19 Jan '13

19 Jan '13

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 18 Jan '13

18 Jan '13

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