Re: 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

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(a)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

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

[Ken Thomases]

On Jan 21, 2013, at 9:31 AM, C.W. Betts wrote:
> On Jan 20, 2013, at 11:06 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
> > 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 can't be certain, because Apple hasn't documented this API very well, but I believe that NoDisplaySleep is deprecated because Apple doesn't like its semantics.  It's not because it's just an old name for PreventUserIdleDisplaySleep; it's because PreventUserIdleDisplaySleep has better semantics.
> I don't think kIOPMAssertionTypePreventUserIdleDisplaySleep will do anything on 10.6

Right.

I wasn't suggesting that you use PreventUserIdleDisplaySleep on 10.6.  The code itself is fine.  I'm just saying that your comments were misleading because they suggest there are two different names for the same thing.

-Ken