Re: macdrv: implement getting and setting the screen saver state.

[Ken Thomases]

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

macdrv: implement getting and setting the screen saver state.

[C.W. Betts]

On Jan 20, 2013, at 12:38 AM, Ken Thomases <ken(a)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

macdrv: implement getting and setting the screen saver state.

[Ken Thomases]

On Jan 20, 2013, at 11:47 AM, C.W. Betts wrote:
> On Jan 20, 2013, at 12:38 AM, Ken Thomases <ken(a)codeweavers.com> wrote:
> > On Jan 19, 2013, at 4:08 PM, C.W. Betts wrote:
> > > +                    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.

I understand why you're checking both.  My question was why the code wasn't similar for checking each case.  You check them both, which is good, but you check each in a slightly different way, which is confusing and inconsistent.  It's also error-prone for the possible future where CFDictionaryGetValue(..., kIOPMAssertionTypeNoDisplaySleep) might return NULL.  Should that happen, CFNumberGetValue() would crash.

-Ken