On Jan 20, 2013, at 12:38 AM, Ken Thomases <[email protected]> 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
>
>
