macdrv: implement getting and setting the screen saver state, version 2

[C.W. Betts]

On Jan 20, 2013, at 11:06 PM, Ken Thomases <ken at codeweavers.com> wrote:

> 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.
Oops, forgot about that. Fixed.
> 
> 
>> +                    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.)
Fixed
> 
> 
>> +                    if (longCount || longCount2 )
> 
> There's an extraneous space before the closing parenthesis.
Fixed
> 
> 
>> +                    //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.
Done
> 
> 
>> +                    //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.
From the header for kIOPMAssertionTypeNoDisplaySleep: Deprecated in 10.7; Please use assertion type kIOPMAssertionTypePreventUserIdleDisplaySleep. From Apple Docs, kIOPMAssertionTypePreventUserIdleDisplaySleep is only available on 10.7 or later. I don't think kIOPMAssertionTypePreventUserIdleDisplaySleep will do anything on 10.6
> 
> 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.
Oops, I'll look into getting rid of those.
> 
> Thanks,
> Ken
> 
>