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

[Ken Thomases]

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