Re: macdrv: implement getting and setting the screen saver state, version 2
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
Re: macdrv: implement getting and setting the screen saver state.
On Jan 20, 2013, at 11:47 AM, C.W. Betts wrote: > On Jan 20, 2013, at 12:38 AM, Ken Thomases wrote: > >> On Jan 19, 2013, at 4:08 PM, C.W. Betts wrote: >> >>> +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. > On Lion and later, kIOPMAssertionTypeNoDisplaySleep is deprecated in favor of > kIOPMAssertionTypePreventUserIdleDisplaySleep. This catches both: I don't > know if OS X Lion and later automatically maps > kIOPMAssertionTypeNoDisplaySleep to > kIOPMAssertionTypePreventUserIdleDisplaySleep, so this takes care of both. I understand why you're checking both. My question was why the code wasn't similar for checking each case. You check them both, which is good, but you check each in a slightly different way, which is confusing and inconsistent. It's also error-prone for the possible future where CFDictionaryGetValue(..., kIOPMAssertionTypeNoDisplaySleep) might return NULL. Should that happen, CFNumberGetValue() would crash. -Ken
Re: wiki
Our server is up, but it looks like the winehq DNS has a problem: Hmm. I'm not seeing that; if I use dig on all 3 of the winehq.org DNS servers, I get the appropriate record, and a dig against 4.4.4.4 and 8.8.8.8 get the cname as well... Cheers, Jeremy
Re: wiki
On 13-01-20 11:52 AM, André Hentschel wrote: Am 20.01.2013 17:21, schrieb Rico Schüller: Hi, is anyone able to get the wiki up again? Somehow the wiki (http://wiki.winehq.org/) is not reachable from here. Cheers Rico Forwarding to Dimi, but i would not expect much on a sunday ;) Our server is up, but it looks like the winehq DNS has a problem: orion:Downloads dimi$ ping wiki.winehq.org ping: cannot resolve wiki.winehq.org: Unknown host orion:Downloads dimi$ ping winehq.org PING winehq.org (209.46.25.134): 56 data bytes 92 bytes from 192.168.3.1: Redirect Host(New addr: 192.168.3.250) Vr HL TOS Len ID Flg off TTL Pro cks Src Dst 4 5 00 0054 7e22 0 3f 01 4e74 192.168.3.182 209.46.25.134 64 bytes from 209.46.25.134: icmp_seq=0 ttl=48 time=97.656 ms 64 bytes from 209.46.25.134: icmp_seq=1 ttl=48 time=71.616 ms 64 bytes from 209.46.25.134: icmp_seq=2 ttl=48 time=74.423 ms -- Dimi Paun Lattica, Inc.
Re: macdrv: implement getting and setting the screen saver state.
On Jan 20, 2013, at 12:38 AM, Ken Thomases wrote:
> 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".
Fixed.
>
>
>> +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.
On Lion and later, kIOPMAssertionTypeNoDisplaySleep is deprecated in favor of
kIOPMAssertionTypePreventUserIdleDisplaySleep. This catches both: I don't know
if OS X Lion and later automatically maps kIOPMAssertionTypeNoDisplaySleep to
kIOPMAssertionTypePreventUserIdleDisplaySleep, so this takes care of both.
>
>
> 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.
Done.
>
>
>> +if(success != kIOReturnSuccess)
>
> Another style nitpick: please put a space between "if" and the condition.
> That applies to the "if(count2)" above, too.
Done.
>
>
>> +/*
>> + 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.
Done.
>
>> +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.
I'll look into it.
>
>
>> +break;
>
> This seems extraneous.
If you don't think there should be a break after a function returns, that's
okay: I just prefer to do that. Either way, done.
>
> Cheers,
> Ken
>
>
Re: [3/6] windowscodecs: Implement tiff encoder options (TiffCompressionMethod and CompressionQuality)
Am Sonntag, 20. Januar 2013, 12:53:56 schrieb Dmitry Timoshkov: > Ludger Sprenker wrote: > > +read_hr = IPropertyBag2_Read(pIEncoderOptions, 2, pb, NULL, v, > > hr); +if (SUCCEEDED(hr)) > > Looks like a typo. Yes, it is typo. I am checking SUCCEEDED(read_hr) in the jpeg and png encoder. I will create a new patch set only for the PropertyBag class (based on the suggestions from Vincent). After these changes are in the master branch I will resubmit the encoder patches.
Re: wiki
Am 20.01.2013 17:21, schrieb Rico Schüller: > Hi, > > is anyone able to get the wiki up again? Somehow the wiki > (http://wiki.winehq.org/) is not reachable from here. > > Cheers > Rico > > Forwarding to Dimi, but i would not expect much on a sunday ;) -- Best Regards, André Hentschel
wiki
Hi, is anyone able to get the wiki up again? Somehow the wiki (http://wiki.winehq.org/) is not reachable from here. Cheers Rico
