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
