On Jan 20, 2013, at 11:06 PM, Ken Thomases <k...@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
>
>
