Re: macdrv: implement getting and setting the screen saver state.
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
Re: [3/6] windowscodecs: Implement tiff encoder options (TiffCompressionMethod and CompressionQuality)
Ludger Sprenker wrote: > +read_hr = IPropertyBag2_Read(pIEncoderOptions, 2, pb, NULL, v, hr); > +if (SUCCEEDED(hr)) Looks like a typo. -- Dmitry.
Re: [3/5] comctl32/listview: always use large icon size when calculating icon spacing
Looks like the testbot is still having problems... 2013/1/20 Marvin : > Hi, > > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > http://testbot.winehq.org/JobDetails.pl?Key=24024 > > Your paranoid android. > > > === WINEBUILD (build) === > Patch failed to apply
Re: [3/5] comctl32/listview: always use large icon size when calculating icon spacing
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=24024 Your paranoid android. === WINEBUILD (build) === Patch failed to apply
Re: [4/5] comctl32/listview: do not touch icon spacing if set explicitly
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=24025 Your paranoid android. === WINEBUILD (build) === Patch failed to apply
Re: [5/5] comctl32/listview: fix LVM_SETICONSPACING on 64bit machines
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=24026 Your paranoid android. === WINEBUILD (build) === Patch failed to apply
Re: User driver function conventions
Although, looking through the X11 Driver, there is a function that does check for the screen saver activity, it's just not two functions: SystemParametersInfo. I'll see if I can't get a patch put out. Also, what would be a good way to do this? Should I split the code into a new file or put it in macdrv_main.c? It does include a new header, as well as declare a static variable (Tell me if that's a no-no and I'll see if I can work around it). Also, the Mac driver would have to link to IOKit. On Jan 19, 2013, at 1:27 PM, "C.W. Betts" wrote: > I'm sorry if my intention was not clear: I want to implement this on vanilla > Wine, I just did the initial work on CodeWeaver's code. But if the functions > aren't on Vanilla, I guess there's no use pushing the patch here. > On Jan 19, 2013, at 1:23 PM, Charles Davis wrote: > >> >> On Jan 19, 2013, at 1:09 PM, C.W. Betts wrote: >> >>> I'm wondering what the user driver functions GetScreenSaveActive and >>> SetScreenSaveActive are supposed to do. I have an implementation against >>> CodeWeaver's changes to Wine and I just want to make sure that I'm making >>> the functions properly. >> How should we know? (Well, those of us who don't work at CW anyway.) USER >> drivers don't even have those in (unmodified) Wine. >> >> I'm sorry if I come across as rude, but this mailing list is for plain Wine. >> If you have a question about CrossOver Wine, you should ask CodeWeavers. I'm >> sure that somewhere on their website, they have an email or a mailing list >> you can contact them with. >> >> Chip >> >> > > > >
Re: User driver function conventions
I'm sorry if my intention was not clear: I want to implement this on vanilla Wine, I just did the initial work on CodeWeaver's code. But if the functions aren't on Vanilla, I guess there's no use pushing the patch here. On Jan 19, 2013, at 1:23 PM, Charles Davis wrote: > > On Jan 19, 2013, at 1:09 PM, C.W. Betts wrote: > >> I'm wondering what the user driver functions GetScreenSaveActive and >> SetScreenSaveActive are supposed to do. I have an implementation against >> CodeWeaver's changes to Wine and I just want to make sure that I'm making >> the functions properly. > How should we know? (Well, those of us who don't work at CW anyway.) USER > drivers don't even have those in (unmodified) Wine. > > I'm sorry if I come across as rude, but this mailing list is for plain Wine. > If you have a question about CrossOver Wine, you should ask CodeWeavers. I'm > sure that somewhere on their website, they have an email or a mailing list > you can contact them with. > > Chip > >
Re: User driver function conventions
On Jan 19, 2013, at 1:09 PM, C.W. Betts wrote: > I'm wondering what the user driver functions GetScreenSaveActive and > SetScreenSaveActive are supposed to do. I have an implementation against > CodeWeaver's changes to Wine and I just want to make sure that I'm making the > functions properly. How should we know? (Well, those of us who don't work at CW anyway.) USER drivers don't even have those in (unmodified) Wine. I'm sorry if I come across as rude, but this mailing list is for plain Wine. If you have a question about CrossOver Wine, you should ask CodeWeavers. I'm sure that somewhere on their website, they have an email or a mailing list you can contact them with. Chip
User driver function conventions
I'm wondering what the user driver functions GetScreenSaveActive and SetScreenSaveActive are supposed to do. I have an implementation against CodeWeaver's changes to Wine and I just want to make sure that I'm making the functions properly. First is GetScreenStateActive. Does it check for if the screen saver is disabled for the whole system or just the app. And SetScreenStateActive. Is it supposed to set the activity for the whole system (I.E. if two apps call SetScreenStateActive to yes, then one to no, does the screen saver run) or is it per-app?
Re: [1/6] windowscodecs: Implementation of IPropertyBag2
You'll probably have to split this into smaller patches.
+return E_FAIL; /* FIXME: Function is not atomar on error,
but MSDN does not say anything about it */
I don't understand. Are you saying native leaks memory on error, and
if so how do you know this?
+FIXME("Application tried to set the unknown option %s. "
+"This may be an application error but is probably
caused by an incomplete implementation in wine\n",
+debugstr_w(pPropBag[i].pstrName));
I don't think the extra explanation of why this is a FIXME adds anything.
+if (This->properties[idx].vt != V_VT(pvarValue+i))
+return E_FAIL;
Are you sure this shouldn't be converted?
+while (TRUE)
+{
+CoTaskMemFree( pPropBag[--i].pstrName );
+if(i == 0)
+break;
+}
This could be written more simply as a do/while loop.
+extern HRESULT CreateEncoderPropertyBag(PROPBAG2 *options, UINT
count, IPropertyBag2 **property) DECLSPEC_HIDDEN;
This adds code that's not really used. You might want to start out by
changing the arguments of CreatePropertyBag2 and adding the
IWICComponentFactory method. Then you can write tests first and remove
todo's as you go. I don't think there's any reason to keep a
constructor with the old set of arguments around.
winetestbot back online, for now
Hey, It looks like the vmware licensing issues are resolved, so all testbot vm's are online again. I hope the replacement testbot is coming along nicely, because winetestbot is still scheduled to go permanently offline in may. ~Maarten
