On Jan 20, 2013, at 12:38 AM, Ken Thomases <k...@codeweavers.com> 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 > >