Re: [PATCH] GNU/kFreeBSD detection
On 08/22/2011 07:11 PM, Robert Millan wrote: 2011/8/22 Alexandre Julliard: If it doesn't need a different behavior there's no need for a new platform, just use PLATFORM_FREEBSD. It may need different behaviour in the future. Anyhow, if you still prefer to share the same macro here's the new patch. Thanks You have to send your patch to wine-patches with (try 2) in the subject and if needed a little changelog in the header so that AJ can see what changed, why... There is one exception though : if you're unsure about a way you wrote a patch, you can send it here with RFC in the subject (meaning you're requesting comments). In this case, you want feedback from other developers hence the wine -devel.
Re: mshtml: Use wininet to get and parse the proxy (try 2)
André Hentschel wrote: > please ignore also that one. APPINFO_QueryOption needs to be fixed first And there's still a leak there. -- Dmitry.
Re: [5/5] ddraw: Emulate a 24 bit depth format without stencil or padding
On 23 August 2011 00:38, Stefan Dösinger wrote: >> How is WINED3DFMT_D24_UNORM different from WINED3DFMT_X8D24_UNORM? > It has a different application-visible ddraw format. I'm not sure that really justifies adding duplicate formats in wined3d. Also, calling this D24_UNORM violates wined3d format naming conventions. D24_UNORM would be a 3 byte format without padding.
Re: [5/5] ddraw: Emulate a 24 bit depth format without stencil or padding
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
Am 22.08.2011 um 23:50 schrieb Henri Verbeet:
> On 22 August 2011 21:16, Stefan Dösinger wrote:
>> +{WINED3DFMT_D24_UNORM, 0x0,0x0,0x0,
>> 0x0,4, 24, 0},
>> {WINED3DFMT_X8D24_UNORM,0x0,0x0,0x0,
>>0x0,4, 24, 0},
> How is WINED3DFMT_D24_UNORM different from WINED3DFMT_X8D24_UNORM?
It has a different application-visible ddraw format. The reason why I added the
new format is that we have to convert in both directions and make sure we never
tell the app a surface is D24X8 when it requested D24 or vice versa.
The alternative is to just change the current ddraw format of D24X8 into how my
patch describes D24, but Windows actually advertises both formats, and I think
we should do the same.
-BEGIN PGP SIGNATURE-
Version: GnuPG/MacGPG2 v2.0.17 (Darwin)
iQIcBAEBAgAGBQJOUtpOAAoJEN0/YqbEcdMwDgkP/AyRXdrfJeKJymAET+WAtKAo
vD0shbn3Ke8IMCHpHQSQMs3FqXWze8x2brQUPWdIhw8Vcz6j0gR0TUwxjM35YUVQ
mYiTH2qlq69TIMUFJDg4Df693tTE+NropbyzjfvXHhcRQc4bRaF9xFRIj4/ATtE9
98zU6SomKw2oQ2g8oNg2knLOIuRhQHhvYRg4o7e4RXrZfL1TrM5P6yuUDzdEBwRk
TQJdKcwCgJ0as/WwJqdb49uTBtYXyc3HljyJOFcBUChBiC2Kp3s9gq8DcxN1/v8o
KuZHfW3w62oElvLzoRoe6hk6RmDu02of6XpW4iQfA/rqBWeOyzzxqAxoT3KBRvTy
CXEifQatY7B4RvzfBXTBZTafs4LG8zYFah2IGAGCTMEUKIOsXYNf9KxJHH4EcGXv
TAMs15VE662NQji6FlGZbTIcBCdG+IvLmdgvIwEMJM0xbNpKHNZZDU2LdDuJNQCf
vg9B9KGPLB1wVr021Gi3VyM/eI9uGwCE2dTkxeJg+oDIQnLQy17OYGb9b+9Y+kv5
v4fexE7lCM++aCv75/cQyASK7E9TGEg6v50r2Ry2KL1PpnA8y1cxKt41vP1Oh4fr
0GHVwCY58xCqfxRzHsIaZBN95Tne0aDMijFneYmfuVnuRxoGuWEip1mEfNUfVDzs
XsuJZCY1v/hq62ohOsDX
=/DEZ
-END PGP SIGNATURE-
Re: tools/winegcc: support a trailing / in paths to winebuild
On Mon, Aug 22, 2011 at 7:02 PM, Jerome Leclanche wrote: > That's to be fixed in bash-completion, not winegcc... Uhm, what's wrong with bash-completation? -B takes a directory as an argument so bash adds a slash during autocomplete, that's ok. gcc itself can also deal with a trailing / for -B, it's kinda suprising, if this breaks in winegcc.
Re: [PATCH] GNU/kFreeBSD detection
2011/8/22 Alexandre Julliard :
> If it doesn't need a different behavior there's no need for a new
> platform, just use PLATFORM_FREEBSD.
It may need different behaviour in the future. Anyhow, if you still
prefer to share the same macro here's the new patch.
Thanks
--
Robert Millan
diff --git a/configure b/configure
index 1c453cf..1497bdb 100755
--- a/configure
+++ b/configure
@@ -6854,7 +6854,7 @@ eval ac_res=\$$as_ac_var
$as_echo "$ac_res" >&6; }
if test `eval 'as_val=${'$as_ac_var'};$as_echo "$as_val"'` = yes; then :
case $host_os in
- freebsd*) LDEXECFLAGS="$LDEXECFLAGS -Wl,--section-start,.interp=0x6400" ;;
+ freebsd* | kfreebsd*-gnu) LDEXECFLAGS="$LDEXECFLAGS -Wl,--section-start,.interp=0x6400" ;;
*) LDEXECFLAGS="$LDEXECFLAGS -Wl,--section-start,.interp=0x7bf00400" ;;
esac
diff --git a/configure.ac b/configure.ac
index e6bbb2a..11d22b9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -824,7 +824,7 @@ case $host_os in
*i[[3456789]]86* | x86_64)
WINE_TRY_CFLAGS([-Wl,--section-start,.interp=0x7bf00400],
[case $host_os in
- freebsd*) LDEXECFLAGS="$LDEXECFLAGS -Wl,--section-start,.interp=0x6400" ;;
+ freebsd* | kfreebsd*-gnu) LDEXECFLAGS="$LDEXECFLAGS -Wl,--section-start,.interp=0x6400" ;;
*) LDEXECFLAGS="$LDEXECFLAGS -Wl,--section-start,.interp=0x7bf00400" ;;
esac
])
diff --git a/libs/wine/ldt.c b/libs/wine/ldt.c
index 3098061..4e48b16 100644
--- a/libs/wine/ldt.c
+++ b/libs/wine/ldt.c
@@ -384,7 +384,7 @@ unsigned short wine_ldt_alloc_fs(void)
if (errno != ENOSYS) perror( "set_thread_area" );
}
else global_fs_sel = (ldt_info.entry_number << 3) | 3;
-#elif defined(__FreeBSD__)
+#elif defined(__FreeBSD__) || defined (__FreeBSD_kernel__)
global_fs_sel = GSEL( GUFS_SEL, SEL_UPL );
#endif
}
diff --git a/tools/winebuild/main.c b/tools/winebuild/main.c
index 55908d5..0eeeb04 100644
--- a/tools/winebuild/main.c
+++ b/tools/winebuild/main.c
@@ -65,7 +65,7 @@ enum target_cpu target_cpu = CPU_ARM;
#ifdef __APPLE__
enum target_platform target_platform = PLATFORM_APPLE;
-#elif defined(__FreeBSD__)
+#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
enum target_platform target_platform = PLATFORM_FREEBSD;
#elif defined(__sun)
enum target_platform target_platform = PLATFORM_SOLARIS;
Re: [5/5] ddraw: Emulate a 24 bit depth format without stencil or padding
On 22 August 2011 21:16, Stefan Dösinger wrote:
> +{WINED3DFMT_D24_UNORM, 0x0,0x0,0x0,
>0x0,4, 24, 0},
> {WINED3DFMT_X8D24_UNORM,0x0,0x0,0x0,
>0x0,4, 24, 0},
How is WINED3DFMT_D24_UNORM different from WINED3DFMT_X8D24_UNORM?
Re: [5/5] ddraw: Emulate a 24 bit depth format without stencil or padding
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=13562 Your paranoid android. === W2KPROSP4 (32 bit d3d) === d3d.c:: Test failed: IDirect3D7_EnumZBufferFormats failed, hr 0x88760154. d3d.c:4445: Test failed: Expected at least one supported Z Buffer format === WXPPROSP3 (32 bit d3d) === d3d.c:: Test failed: IDirect3D7_EnumZBufferFormats failed, hr 0x88760154. d3d.c:4445: Test failed: Expected at least one supported Z Buffer format === W2K3R2SESP2 (32 bit d3d) === d3d.c:: Test failed: IDirect3D7_EnumZBufferFormats failed, hr 0x88760154. d3d.c:4445: Test failed: Expected at least one supported Z Buffer format === WVISTAADM (32 bit d3d) === d3d.c:: Test failed: IDirect3D7_EnumZBufferFormats failed, hr 0x88760154. d3d.c:4445: Test failed: Expected at least one supported Z Buffer format === W2K8SE (32 bit d3d) === d3d.c:: Test failed: IDirect3D7_EnumZBufferFormats failed, hr 0x88760154. d3d.c:4445: Test failed: Expected at least one supported Z Buffer format === W7PRO (32 bit d3d) === d3d.c:: Test failed: IDirect3D7_EnumZBufferFormats failed, hr 0x88760154. d3d.c:4445: Test failed: Expected at least one supported Z Buffer format === W7PROX64 (32 bit d3d) === d3d.c:: Test failed: IDirect3D7_EnumZBufferFormats failed, hr 0x88760154. d3d.c:4445: Test failed: Expected at least one supported Z Buffer format
Re: mshtml: Use the last colon in proxy url as port separator
Am 22.08.2011 11:01, schrieb Jacek Caban: > On 08/19/11 22:10, André Hentschel wrote: >> Am 18.08.2011 22:50, schrieb Juan Lang: Is the ProxyServer specified as an URL? If yes then use the proper API to dissect the URL instead of cobbling something together. >>> Just to follow up in this idea, you could use InternetCrackUrl. >>> mshtml delay loads wininet, but it also loads urlmon, which also loads >>> wininet, so in effect wininet is always loaded and available. >>> >>> But a larger question is, why load the proxy settings from the >>> registry only? If you query wininet for them instead, then the >>> environment variable http_proxy is also checked. There was bug 5625 >>> about this, which I set to fixed based on commit >>> 80f02b82d68902f32578a7bcf6cfbaa715b724ce. I may have been mistaken. >>> Under what circumstances is the network.proxy.http gecko setting still >>> being used? Maybe for embedded content, e.g. IMG tags? If these were >>> set by querying wininet instead, then at least we wouldn't have >>> potentially different proxy settings for some HTTP requests than for >>> others. >>> --Juan >> just sent a patch doing that. >> I'm not sure for what winegecko uses it, but that function was added by >> intention and jacek already modified it a bit, so there must be a reason. > > set_proxy shouldn't be needed any more since we use urlmon for all > connections. I didn't remove it immediately because it was relatively easy to > restore the old behaviour for some time, but other pieces of the old code are > already killed now and it's about the last piece left from before-urlmon > times. Feel free to kill it. > > Cheers, > Jacek ok, just sent a patch. Still my previous attempt showed another wine bug and i'll try to follow it with tests and fix it. (size query per InternetQueryOption) -- Best Regards, André Hentschel
Re: [2/2] dinput8/tests: Tests for DIPROP_USERNAME property
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=13553 Your paranoid android. === W7PROX64 (64 bit device) === device.c:193: Test failed: GetProperty failed hr=80070057 device.c:194: Test failed: Username not set correctly expected=L"winetest", got=L"\0488W" device.c:193: Test failed: GetProperty failed hr=80070057 device.c:194: Test failed: Username not set correctly expected=L"winetest", got=L"\0488W" device.c:193: Test failed: GetProperty failed hr=80070057 device.c:194: Test failed: Username not set correctly expected=L"Ninja Brian", got=L"\0488W" device.c:193: Test failed: GetProperty failed hr=80070057 device.c:194: Test failed: Username not set correctly expected=L"Ninja Brian", got=L"\0488W"
Re: [1/2] dinput8/tests: Moved EnumDevicesBySemantics specific tests to dinput.c and added a couple more
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=13552 Your paranoid android. === W7PROX64 (64 bit dinput) === dinput.c:550: Test failed: This user should own no devices owned=2
Re: tools/winegcc: support a trailing / in paths to winebuild
That's to be fixed in bash-completion, not winegcc...
On Mon, Aug 22, 2011 at 5:31 PM, Bernhard Loos
wrote:
> On Mon, Aug 22, 2011 at 5:35 PM, Alexandre Julliard
> wrote:
>> Bernhard Loos writes:
>>
>>> diff --git a/tools/winegcc/winegcc.c b/tools/winegcc/winegcc.c
>>> index 284223e..3a7e93a 100644
>>> --- a/tools/winegcc/winegcc.c
>>> +++ b/tools/winegcc/winegcc.c
>>> @@ -1326,6 +1326,7 @@ int main(int argc, char **argv)
>>> {
>>> case 'B':
>>> str = strdup(option_arg);
>>> + if (strendswith(str, "/")) str[strlen(str) - 1] = 0;
>>
>> Why would you want to append a slash to winebuild?
>>
>> --
>> Alexandre Julliard
>> julli...@winehq.org
>>
>
> Bash autocompletation. If you run winegcc manually, and autocomplete
> the -B argument to the winebuild dir, you end up with a / at the end.
> It's kinda hard to figure out, what goes wrong at this point.
>
>
>
The sad state of audio GetPosition
Hi, Maarten Lankhorst wrote: >But it's not the same, wine doesn't officially handle exclusive mode. Eh? This is new to me! Back in June Andrew Eikum wrote: >These limitations might be true for exclusive mode, >which isn't really implemented in Wine. ... which is not the same in my ears. Andrew back in June: >I think it would be worth having a loud FIXME >when exclusive mode is requested, though. Maarten today: >Instead it might be better to just report failure and only allow shared mode. I prefer a FIXME in exclusive non-event mode and the error in exclusive event mode because that particular GetBuffer protocol is not supported in the current code. FIXME because exclusive non-event mode basically works except that we don't know yet when exactly native delivers events. As nobody else wrote that patch so far, I'll do. >On windows you're not required to support exclusive mode, >and you can disable exclusive mode in the audio control panel. I've read that games offer it as an option but believe that all apps will use the shared mode by default for the reason you say. Mixing has become a commodity. > http://msdn.microsoft.com/en-us/library/dd370844%28v=vs.85%29.aspx >I meant an entire period then, this describes how windows handles it What part of that document do you have in mind? Their example uses exclusive+EVENTCALLBACK, which my test doesn't. >my guess is that if you don't fill a period, >you get an underrun, no matter how you handle it. Sure. Yet I want to know how it affects GetPosition, e.g. bug #28039 IMHO I'm free to use GetBuffer+ReleaseBuffer twice or 3x within whatever MSDN calls a "buffer-processing period" if it pleases me. What matters is how much has accumulated once the period tick occurs. If period sizes would really matter, then the API would provide means to obtain the period size in frames. It does not. Initialize takes a period in milliseconds and MSDN does not guarantee what you actually get. So the average app simply does not know the period size in frames. (My tests do try to second-guess mmdevapi's exact behaviour). Actually, mmdevapi tries to abstract away from these parameters. BTW, a bug in MSDN: render.c:799: Test failed: BufferSize 21846 too small for duration at rate 44100 mmdevapi appears to use not ceiling(), but floor(duration, period_frames). The actual duration is *not* at least as large as asked for. (For the period, it uses ceiling). Regards, Jörg Höhle
Re: tools/winegcc: support a trailing / in paths to winebuild
On Mon, Aug 22, 2011 at 5:35 PM, Alexandre Julliard wrote:
> Bernhard Loos writes:
>
>> diff --git a/tools/winegcc/winegcc.c b/tools/winegcc/winegcc.c
>> index 284223e..3a7e93a 100644
>> --- a/tools/winegcc/winegcc.c
>> +++ b/tools/winegcc/winegcc.c
>> @@ -1326,6 +1326,7 @@ int main(int argc, char **argv)
>> {
>> case 'B':
>> str = strdup(option_arg);
>> + if (strendswith(str, "/")) str[strlen(str) - 1] = 0;
>
> Why would you want to append a slash to winebuild?
>
> --
> Alexandre Julliard
> julli...@winehq.org
>
Bash autocompletation. If you run winegcc manually, and autocomplete
the -B argument to the winebuild dir, you end up with a / at the end.
It's kinda hard to figure out, what goes wrong at this point.
Re: wine-devel Digest, Vol 73, Issue 58
Dear list, sorry for the accidental posting! Gah! I hit a premature ctrl+enter! ... Michael Ost On 08/22/2011 09:16 AM, Michael Ost wrote: Louis, I wonder if the patch referred to here is relevant for the device
Re: wine-devel Digest, Vol 73, Issue 58
Louis, I wonder if the patch referred to here is relevant for the device detection we need for the ilok...? I'm kinda shooting in the dark, without much background, but the message jumped out at me. - mo On 08/22/2011 08:06 AM, wine-devel-requ...@winehq.org wrote: Message: 3 Date: Mon, 22 Aug 2011 14:34:59 +0200 (CEST) From: Francois Gouget Subject: Re: [PATCH] mountmgr: Support the dbus service udisks for dynamic devices :-) [try 5] To: wine-devel@winehq.org Cc: Detlef Riekenberg, wine-patc...@winehq.org Message-ID: Content-Type: text/plain; charset="iso-8859-15" On Tue, 16 Aug 2011, Detlef Riekenberg wrote: [...] Recent distributions depend on udisks, so the libhal development package should be uninstalled or disabled when building Wine (--without-hal). I don't understand this recommendation to build with '--without-hal'. It should be possible to build a Wine binary that will run on both old HAL-based Linux distributions and the new usdisks-based ones. Such a binary would need to be built with both HAL and udisks support. Are you saying your patch makes that is impossible?
mmdevapi: After GetBuffer, refuse Reset, but Start/Stop is ok.
Hi, >> My tests also show that a GetBuffer/ReleaseBuffer pair is left >> undisturbed by Stop/Start in between. Fortunately, this has no effect >> on the current code (but it shows that Stop must not change >> This->held_frames or the write_pos). >Could you add tests to render.c showing this? Eventually all my tests will find their way into git, it's just that I need time to think about how I can make them work "mostly" in Wine without crashing and giving false results because of some bugs. Hopefully before Friday. It would take time too to trim my tests down to a subset that works in current Wine. For now the plan is to add fixes slowly until the point where only a few todo_wine will suffice, without crashes. You are welcome to try out some version of my tests attached to #27937, or more current as testbot job #13483, which includes the ordering tests, which some people on this list tested too (thank you again, guys). BTW, the test result is not in contradiction with MSDN. It's always the same once the tests become too elaborated. I've a fairly complex mcimidi testsuite which I'm not submitting yet because I'll first have to finish some of my mcimidi patches. todo_wine is not enough to get things working. Actually, the reason is more subtle: adding mcimidi tests now with lots of todo_wine and defenses against crashes would make it hard to find an ordering of patches that always removes todo_wine instead of temporarily adding some to tests that work by chance (or double error) rather than design. So it's much easier to write the tests first, then the code, then submit the code, then the tests. Which is what I'm doing. Regards, Jörg Höhle
Re: The sad state of audio GetPosition
On 08/22/2011 05:31 PM, joerg-cyril.hoe...@t-systems.com wrote: > Hi, > > Maarten Lankhorst wrote: >> exclusive mode wants you to fill the entire buffer at the same time. > You're confusing this with exclusive + EVENTCALLBACK mode, which I'm not > using. > There are 4 combinations of shared/exclusive and EVENTCALLBACK or not. > Only one of the 4 needs no GetCurrentPadding and uses a full buffer each time. > > So far I've only been testing without EVENTCALLBACK > (though test_event contains some non-rendering code using it). http://msdn.microsoft.com/en-us/library/dd370844%28v=vs.85%29.aspx I meant an entire period then, this describes how windows handles it, my guess is that if you don't fill a period, you get an underrun, no matter how you handle it. >> it wouldn't surprise me if that's why it drops things.. > I'd expect GetBuffer to return AUDCLNT_E_BUFFER_SIZE_ERROR as documented for > that case. Ah true. Event driven is slightly different. :) > > Last week I wrote: >> Somewhat I found the old behavior more consistent. > Upon reflection, the new behavior is better. It makes more sense to drop > old frames than to keep them around thus play ghost sounds from the past. > > However, I'd even more prefer shared and exclusive mode to behave the same > *and* not drop frames. > I believe shared mode does not drop partial fills (based solely on what > GetPosition returns) and would have expected exclusive mode to do that > too. I don't know why MS decided to do that but I now see > why they recommend that programs add silence at the end of their > sound: it works around that odd behaviour. But it's not the same, wine doesn't officially handle exclusive mode. Instead it might be better to just report failure and only allow shared mode. On windows you're not required to support exclusive mode, and you can disable exclusive mode in the audio control panel. > > Maarten also wrote: >> I prefer 0 tracking, and just check the return value of snd_pcm_writei. > I can understand that but then we need to throw rate-limiting into the > discussion. We are talking about GetPosition here; computing > GetPosition solely on the base of snd_pcm_writei (let's throw in > avail_update without delay) gives us already known bugs, > e.g. PulseAudio backend audio 2s off video sync. Ah true, I do think you only want to observe underruns, and not act on them in GetPosition and GetClock, just report that all data is played. If that still breaks pulseaudio, I think it's time to get a real pulseaudio driver.. > And without snd_pcm_delay, the sole culprit would be: Wine. > If, OTOH, we use it, we can hope other projects will fix their bugs. > snd_pcm_delay would map to clock, while snd_pcm_avail maps to position, they're not the same, so it would make sense to handle them separately. ~Maarten
Re: tools/winegcc: support a trailing / in paths to winebuild
Bernhard Loos writes:
> diff --git a/tools/winegcc/winegcc.c b/tools/winegcc/winegcc.c
> index 284223e..3a7e93a 100644
> --- a/tools/winegcc/winegcc.c
> +++ b/tools/winegcc/winegcc.c
> @@ -1326,6 +1326,7 @@ int main(int argc, char **argv)
> {
> case 'B':
> str = strdup(option_arg);
> + if (strendswith(str, "/")) str[strlen(str) - 1] = 0;
Why would you want to append a slash to winebuild?
--
Alexandre Julliard
julli...@winehq.org
The sad state of audio GetPosition
Hi, Maarten Lankhorst wrote: > exclusive mode wants you to fill the entire buffer at the same time. You're confusing this with exclusive + EVENTCALLBACK mode, which I'm not using. There are 4 combinations of shared/exclusive and EVENTCALLBACK or not. Only one of the 4 needs no GetCurrentPadding and uses a full buffer each time. So far I've only been testing without EVENTCALLBACK (though test_event contains some non-rendering code using it). >it wouldn't surprise me if that's why it drops things.. I'd expect GetBuffer to return AUDCLNT_E_BUFFER_SIZE_ERROR as documented for that case. Last week I wrote: >Somewhat I found the old behavior more consistent. Upon reflection, the new behavior is better. It makes more sense to drop old frames than to keep them around thus play ghost sounds from the past. However, I'd even more prefer shared and exclusive mode to behave the same *and* not drop frames. I believe shared mode does not drop partial fills (based solely on what GetPosition returns) and would have expected exclusive mode to do that too. I don't know why MS decided to do that but I now see why they recommend that programs add silence at the end of their sound: it works around that odd behaviour. Maarten also wrote: >I prefer 0 tracking, and just check the return value of snd_pcm_writei. I can understand that but then we need to throw rate-limiting into the discussion. We are talking about GetPosition here; computing GetPosition solely on the base of snd_pcm_writei (let's throw in avail_update without delay) gives us already known bugs, e.g. PulseAudio backend audio 2s off video sync. And without snd_pcm_delay, the sole culprit would be: Wine. If, OTOH, we use it, we can hope other projects will fix their bugs. Regards, Jörg Höhle
Re: ntdll: While requesting TokenGroups calculate required user buffer size in server (try2)
Nikolay Sivov writes:
> +SERVER_START_REQ( get_token_groups )
> {
> -need_more_memory = FALSE;
> +TOKEN_GROUPS *groups = tokeninfo;
> +void *buffer;
>
> -SERVER_START_REQ( get_token_groups )
> -{
> -TOKEN_GROUPS *groups = tokeninfo;
> +/* reply buffer is always shorter than output one */
> +buffer = tokeninfolength ? RtlAllocateHeap(GetProcessHeap(), 0,
> tokeninfolength) : NULL;
Please avoid complex operation like heap allocations inside a server
request block.
--
Alexandre Julliard
julli...@winehq.org
Re: [PATCH] GNU/kFreeBSD detection
Robert Millan writes: > diff --git a/tools/winebuild/build.h b/tools/winebuild/build.h > index 05adf31..cf389fb 100644 > --- a/tools/winebuild/build.h > +++ b/tools/winebuild/build.h > @@ -148,6 +148,7 @@ enum target_platform > PLATFORM_UNSPECIFIED, > PLATFORM_APPLE, > PLATFORM_FREEBSD, > +PLATFORM_GNUKFREEBSD, > PLATFORM_SOLARIS, > PLATFORM_WINDOWS > }; If it doesn't need a different behavior there's no need for a new platform, just use PLATFORM_FREEBSD. -- Alexandre Julliard julli...@winehq.org
Re: tools/wine.inf: Add registry key HKLM, %CurrentVersionNT%, "ProductName"
Louis Lenders writes: > Hi, this fixes http://bugs.winehq.org/show_bug.cgi?id=28065 This would need to be updated when the version is changed in winecfg. -- Alexandre Julliard julli...@winehq.org
Re: Hebrew: update
On 08/22/2011 07:25 AM, Francois Gouget wrote: >> msgid "DirectX Diagnostic Tool" >> -msgstr "" >> +msgstr "DirectX Diagnostic Tool" > > This is sort of a product name os it's always a bit dicey. Still, > wouldn't it be better if 'Diagnostic Tool' was translated? At > non-English speakers would understand what the purpose of the tool it. WTB screenshots of dxdiag.exe I obtained from the non-English XP machines some time ago show that this string is translated, so localizing this string would not be an unprecedented course of action. signature.asc Description: OpenPGP digital signature
Re: mmdevapi: After GetBuffer, refuse Reset, but Start/Stop is ok.
On Mon, Aug 22, 2011 at 10:56:29AM +0200, joerg-cyril.hoe...@t-systems.com wrote: > My tests also show that a GetBuffer/ReleaseBuffer pair is left > undisturbed by Stop/Start in between. Fortunately, this > has no effect on the current code (but it shows that Stop must > not change This->held_frames or the write_pos). > Could you add tests to render.c showing this? Andrew
Re: [1/3] wined3d: Constant load optimisations
On Sunday 21 August 2011 00:18:56 Henri Verbeet wrote: > Yes, that's somewhat suspicious, although it's possible that the > constants all being in a single array prevents that. I'd have to look > up if the spec has anything to say about this. I think that was about the attributes, not the uniforms. In my regression hunt I ran across a game that declared a vshader attribute and didn't actually use it, so the compiler optimized it away and glBindAttribLocationARB records a warning in the shader's info log. signature.asc Description: This is a digitally signed message part.
Re: hhctrl: IDTB_XXX resource strings
I see the strings being added to the toolbar with LPWSTR szBuf = HH_LoadString(buttons[dwIndex].idCommand); ... buttons[dwIndex].iString = (DWORD)SendMessageW(hToolbar, TB_ADDSTRINGW, 0, (LPARAM)szBuf); But from there it's not clear to me they whether they will be used or not. Did you specifically mean that only Jump1/2 would not be used or does that apply to the other IDTB_XXX strings too? I was referring specifically to Jump1/2 in this case. They do get used, particularly as it looks like we don't currently replace them with the custom text. However, the "IDTB" strings that we have are all noted as "unimplemented" in Microsoft's htmlhelp.h file, and in theory nobody should be using them. Considering we don't support any of them in TB_AddButtonsFromFlags, I think the resource entries for them can be removed. James does seem to have added support for creating the toolbar buttons for IDTB_TOC_NEXT/PREV, but I can't find any documentation on what they're meant to do, and he added no actual functionality, so they can probably go too. -- Owen Rudge http://www.owenrudge.net/
Re: hhctrl: IDTB_XXX resource strings
On Mon, 22 Aug 2011, Owen Rudge wrote: > > What's the difference between 'Jump1' and 'Jump2'? > > Both are, it appears, toolbar buttons that can be customised by the help file > developer and used to link to individual topics or URLs. The captions are > customisable. The strings "Jump1" and "Jump2" themselves, as far as I can > tell, should never be displayed to the user. I see the strings being added to the toolbar with LPWSTR szBuf = HH_LoadString(buttons[dwIndex].idCommand); ... buttons[dwIndex].iString = (DWORD)SendMessageW(hToolbar, TB_ADDSTRINGW, 0, (LPARAM)szBuf); But from there it's not clear to me they whether they will be used or not. Did you specifically mean that only Jump1/2 would not be used or does that apply to the other IDTB_XXX strings too? And if they are never shown to the user should be no issue with removing them, right? (or replacing them with hardcoded strings in the source) -- Francois Gouget http://fgouget.free.fr/ Computers are like airconditioners They stop working properly if you open WINDOWS
Re: hhctrl: IDTB_XXX resource strings
However a number of them make no sense at all and are a pain to translate. For instance: IDTB_NOTES "IDTB_NOTES" IDTB_BROWSE_FWD "IDTB_BROWSE_FWD" IDTB_BROWSE_BACK "IDT_BROWSE_BACK" With regards to these buttons, attempting to use those constants on Windows results in buttons (captioned "Notes", "Next", "Previous") with no apparent functionality whatsoever. Microsoft's own documentation suggests these are "not implemented". I expect we could remove those from Wine's hhctrl without any great consequences. -- Owen Rudge http://www.owenrudge.net/
Re: hhctrl: IDTB_XXX resource strings
What's the difference between 'Jump1' and 'Jump2'? Both are, it appears, toolbar buttons that can be customised by the help file developer and used to link to individual topics or URLs. The captions are customisable. The strings "Jump1" and "Jump2" themselves, as far as I can tell, should never be displayed to the user. -- Owen Rudge http://www.owenrudge.net/
Re: [PATCH] mountmgr: Support the dbus service udisks for dynamic devices :-) [try 5]
On Tue, 16 Aug 2011, Detlef Riekenberg wrote: [...] > Recent distributions depend on udisks, so the libhal development package > should be uninstalled or disabled when building Wine (--without-hal). I don't understand this recommendation to build with '--without-hal'. It should be possible to build a Wine binary that will run on both old HAL-based Linux distributions and the new usdisks-based ones. Such a binary would need to be built with both HAL and udisks support. Are you saying your patch makes that is impossible? -- Francois Gouget http://fgouget.free.fr/ Vote Electronique: Les gens qui votent ne décident rien. Ceux qui comptent les votes décident de tout.
Re: [3/3]usp10/test: test ScriptXtoX on an RTL set with differring cChars and cGlyphs
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=13546 Your paranoid android. === W2KPROSP4 (32 bit usp10) === usp10.c:1931: Test failed: ScriptCPtoX trailing: iCP=8 should return piX=0 not 0
Re: The sad state of audio GetPosition
On 08/22/2011 02:13 PM, joerg-cyril.hoe...@t-systems.com wrote:
> Hi,
>
> These exclusive mode test failures look like nothing:
> render.c:1086: Test failed: GetCurrentPadding returned 576, should be 0
> render.c:1092: Test failed: Position 90720 at end vs. 91296 played frames
> but they made me completely rework my model of how mmdevapi works.
>
> 91296 = 90720 + 576 remainder
> 90720 = 720 * 126
> 720 = 15ms period at 48000 frames/s
>
> It looks like only a multiple of period_size frames are rendered.
> There are also test runs which end with GCP > period_size despite underrun
>
> Only Vista and w2k8 present these 2 errors. w2k8R2 and w7 only
> present the second one. In shared mode, GetCurrentPadding (GCP)
> returns 0 and GetPosition returns all submitted ("played") frames.
> What happened?
>
> Possible explanations:
> Vista + w2k8 exclusive mode:
> a) Bug in native that forgets to account for sub-period_size data. Or
> b) Play only multiples of period size. Wait until period_size data
>accumulates before sending the next packet.
Have to go with this one, exclusive mode wants you to fill the entire buffer at
the same time..
> c) other...
>
> w2k8R2 + w7 exclusive mode:
> a) Bug in native that sets GCP to 0 (like in shared mode). Or
> b) At each period tick, only play data if a full period is available.
>Drop partially filled buffers (explains padding = 0) instead of
>playing them much later when the buffer fills up (no ghost sounds).
> c) Bug in native that forgets to increment Position yet plays data.
> d) other...
If I look at msdn, exclusive mode wants you to fill the entire buffer in 1 go,
doing partial data doesn't make sense, and it wouldn't surprise me if that's
why it drops things..
> shared mode:
> a) At each 10ms period tick, mix a period_size of all streams that
>provided a full period of data. Ignore other streams yet always
>decrement GetCurrentPadding and sum played/ignored frames.
> b) At each 10ms period tick, mix all streams. Handle streams with
>padding < period_size as follows:
>- if previous frame was full, mix at beginning of buffer (trailer)
>- otherwise mix at end (expect beginning of next sound)
> c) At each 10ms period tick, mix all streams, padding streams with
>less than period_size frames with silence (i.e. mix at beginning).
> d) other...
> If I were MS, I'd implement heuristic b).
>
> This is all unlike what Wine does...
I'm inclined to believe it's C
> I'll probably write audible audio tests in the next few weeks to be
> able to test what native does (e.g. emit a short beep < period size
> every few ms and listen to what happens).
>
> Regards,
> Jörg Höhle
>
>
Re: Hebrew: update
> #: crypt32.rc:182
> msgid "KeyID="
> -msgstr ""
> +msgstr "KeyID="
I think you should really translate 'KeyID' In French it was translated
to 'ID de clé'. So assuming there is a Hebrew word for 'key' then this
would not remain as is.
> -"\t/i {package|productcode} [property]\n"
> -"\t/package {package|productcode} [property]\n"
> +"\t/i {package|product_code} [property]\n"
> +"\t/package {package|product_code} [property]\n"
Ideally you should translate the user-replaceable strings like 'package'
and 'productcode'. But option names such as '/package' should remain as
is of course.
> msgid "DirectX Diagnostic Tool"
> -msgstr ""
> +msgstr "DirectX Diagnostic Tool"
This is sort of a product name os it's always a bit dicey. Still,
wouldn't it be better if 'Diagnostic Tool' was translated? At
non-English speakers would understand what the purpose of the tool it.
> "/UnixUse a Unix filename and start the file like windows
> explorer.\n"
> "/ProgIDOpen Open a document using the following progID.\n"
> "/L Show end-user license.\n"
> +"/? Display this help and exit.\n"
> "\n"
> "start.exe version 0.2 Copyright (C) 2003, Dan Kegel\n"
> "Start comes with ABSOLUTELY NO WARRANTY; for details run with /L option.\n"
Nothing is translated here. This whole msgstr should be removed.
> msgid "RENAME renames a file.\n"
> -msgstr "RENAME renames a file\n"
> +msgstr "RENAME renames a file.\n"
[...]
> msgid "VER displays the version of cmd you are running.\n"
> -msgstr "VER displays the version of cmd you are running\n"
> +msgstr "VER displays the version of cmd you are running.\n"
[...]
> msgstr ""
> +"ENDLOCAL ends localization of environment changes in a batch file\n"
> +"which were introduced by a preceding SETLOCAL.\n"
[...]
> -msgstr "%s : File Not Found\n"
> +msgstr "%s: File Not Found\n"
[...a lot more...]
These are not translated and do not have their place in a PO file. They
will prevent future translators working on this file from knowing what
has been translated.
These totally prevent the patch from going in as is. Please fix it and
resubmit.
--
Francois Gouget http://fgouget.free.fr/
Linux: It is now safe to turn on your computer.
The sad state of audio GetPosition
Hi,
These exclusive mode test failures look like nothing:
render.c:1086: Test failed: GetCurrentPadding returned 576, should be 0
render.c:1092: Test failed: Position 90720 at end vs. 91296 played frames
but they made me completely rework my model of how mmdevapi works.
91296 = 90720 + 576 remainder
90720 = 720 * 126
720 = 15ms period at 48000 frames/s
It looks like only a multiple of period_size frames are rendered.
There are also test runs which end with GCP > period_size despite underrun
Only Vista and w2k8 present these 2 errors. w2k8R2 and w7 only
present the second one. In shared mode, GetCurrentPadding (GCP)
returns 0 and GetPosition returns all submitted ("played") frames.
What happened?
Possible explanations:
Vista + w2k8 exclusive mode:
a) Bug in native that forgets to account for sub-period_size data. Or
b) Play only multiples of period size. Wait until period_size data
accumulates before sending the next packet.
c) other...
w2k8R2 + w7 exclusive mode:
a) Bug in native that sets GCP to 0 (like in shared mode). Or
b) At each period tick, only play data if a full period is available.
Drop partially filled buffers (explains padding = 0) instead of
playing them much later when the buffer fills up (no ghost sounds).
c) Bug in native that forgets to increment Position yet plays data.
d) other...
shared mode:
a) At each 10ms period tick, mix a period_size of all streams that
provided a full period of data. Ignore other streams yet always
decrement GetCurrentPadding and sum played/ignored frames.
b) At each 10ms period tick, mix all streams. Handle streams with
padding < period_size as follows:
- if previous frame was full, mix at beginning of buffer (trailer)
- otherwise mix at end (expect beginning of next sound)
c) At each 10ms period tick, mix all streams, padding streams with
less than period_size frames with silence (i.e. mix at beginning).
d) other...
If I were MS, I'd implement heuristic b).
This is all unlike what Wine does...
I'll probably write audible audio tests in the next few weeks to be
able to test what native does (e.g. emit a short beep < period size
every few ms and listen to what happens).
Regards,
Jörg Höhle
Re: server: correctly map mutex acces rights
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=13534 Your paranoid android. === W7PROX64 (64 bit sync) === sync.c:160: Test failed: WaitForSingleObject succeeded sync.c:177: Test failed: ReleaseMutex should have failed with ERROR_NOT_OWNER instead of 5
Re: [PATCH 9/9] wscript: add textstream.c; implement Host_get_StdOut
On Mon, Aug 22, 2011 at 1:54 PM, Michał Ziętek wrote:
> +static HRESULT WINAPI TextStream_QueryInterface(ITextStream *iface, REFIID
> riid, void **ppv)
> +{
> +WINE_TRACE("(%s %p)\n", wine_dbgstr_guid(riid), ppv);
> +
> +if(IsEqualGUID(&IID_IUnknown, riid)
> + || IsEqualGUID(&IID_IDispatch, riid)
> + || IsEqualGUID(&IID_IArguments2, riid)) {
Hello,
There's a copy-paste typo here.
Cheers,
Octavian
hhctrl: IDTB_XXX resource strings
The native hhctrl.ocx does not have any resrouce strings. My understanding is that we have them in Wine because we use them as tooltips for the toolbar items. That's good I guess. However a number of them make no sense at all and are a pain to translate. For instance: IDTB_NOTES "IDTB_NOTES" IDTB_BROWSE_FWD "IDTB_BROWSE_FWD" IDTB_BROWSE_BACK "IDT_BROWSE_BACK" or IDTB_JUMP1 "Jump1" IDTB_JUMP2 "Jump2" What's the difference between 'Jump1' and 'Jump2'? Also I have found no proper documentation on this. So does anyone know what to put there so we have strings that correctly describe what these toolbar buttons do? -- Francois Gouget http://fgouget.free.fr/ question = ( to ) ? be : ! be; -- Wm. Shakespeare
Re: The sad state of audio GetPosition
On 08/19/2011 05:10 PM, joerg-cyril.hoe...@t-systems.com wrote:
> Maarten Lankhorst wrote:
> [nice to hear from you]
>
>>> IMHO AudioClient_Stop must not map to
>>> snd_pcm_drop. It is more like snd_pcm_pause. Or perhaps simply lead
>>> ALSA into an underrun.
>> afaict pause, with reset mapped to drop,
> Indeed. But I believe I need a fallback because ALSA says that
> pause "works only on the hardware which supports" it.
>
>
>> I can't remember why pause didn't work, but if it works go for it.
> I was solely thinking aloud that pause is TRT, not tried out yet.
>
> However, I received test results from a "Windows 7 Ultimate" machine.
> It exhibits a similar bug -- in exclusive mode only:
> render.c:948: Test failed: Position 18191 too far after 100ms
> Shared mode works as my tests expect it (<= 48000/10 frames).
>
>
>>> +snd_pcm_status_alloca(&status);
>> HeapAlloc(GetProcessHeap(), HEAP_ZERO_FLAG, snd_pcm_status_sizeof())
>> or something like that if available please..
> Really? I don't want to go through the overhead of memory allocation
> when all I need is a stupid small amount of stack allocated memory.
>
>
>>> +if(!This->initted){
>>> +return AUDCLNT_E_NOT_INITIALIZED;
>> Unneeded part.
> Can't you obtain a handle to that COM object prior to
> calling Initialize which sets This->fmt?
No.
>> Follow that flow..
> I beg your pardon?
IAudioClock is not available until Initialize has succeeded, so the check above
is pointless.
>
>>> +if(0){
>>> +avail_frames = snd_pcm_status_get_avail(status);
>>> +delay_frames = snd_pcm_status_get_delay(status);
>> if 0 is bad...
> I tried out pcm_status because somebody in alsa-devel mentioned that
> it allows to grab avail + delay in one (sync'ed?).
> However, I found delay to be always 0 inside status!?!
>
> Also, I found out that I need to call avail_update and delay
> in a particular order, otherwise I get stale values from an old call
> prior to the last sleep...
>
>
>>> +if(avail_frames <= This->bufsize_alsa + MAX_LATE_SECONDS *
>>> This->fmt->nSamplesPerSec
>>> + && delay_frames > 0)
>> Isn't delay_frames < 0 the definition of underrun?
> Indeed.
> There are potentially N distinct underruns:
> - the front end -- what snd_pcm_avail_update knows about;
> - intermittent buffers (USB);
> - the speaker -- what snd_pcm_delay knows about.
> There could be a short front-end buffer underrun that
> goes unnoticed by the speaker if the TCP or USB in
> between buffers enough data *and* is able to speed up.
>
>> no point in adding MAX_LATE_SECONDS
> That is some form of guard against broken values. E.g. people reporting
> in alsa-devel that PA sometimes complains about avail ~ MAXINT and such weird
> values.
>
>> Getting an avail update again? Why?
> The theory is:
> position = written_frames(into ALSA) - delay
> and translates to:
> This->written_frames - This->held_frames - delay
>
> However sometimes I can't trust delay.
> I still need to figure out when.
> - IIRC after an underrun, snd_pcm_delay yields error X.
> - or was it before starting?
> - ...
>
> The upper bound on position is always:
> This->written_frames(ReleaseBuffer) - This->held_frames - ALSA_padding
> (what ALSA's front end has not yet processed, in absence of underrun).
>
> Perhaps that would be robust:
> 1. Compute upper bound
> 2. position = clamp(0, delay > 0 ? written-delay : written, upper_bound);
> 2b. except when not yet started ...
> 2c. except while stopped ...
>
> I was even considering:
> 3. if position < This->previous_position stick to previous...
>
> Yet perhaps it's better to allow intermittent garbage values
> than to stick to garbage!
>
> OTOH, the delay values I see in the logs are subject to such variation (with
> PA)
> that I'm considering going with a clock instead, or perhaps:
> - query delay once per tick (e.g. 10ms) => last_pos
> - when asked, compute position from last_pos + time since tick * rate
>
> The last_pos slot may be needed anyway once stopped in pause mode.
>
Don't worry about underruns and checking the way you do is fragile, I prefer 0
tracking, and just check the return value of snd_pcm_writei. Experience taught
me this is the most stable way of doing underrun handling in alsa. Remove all
the checks you're doing please, they will just break things more.
Substained 10ms periods in Wine audio?
Hi,
>>> IMHO AudioClient_Stop must not map to snd_pcm_drop. It is more like
>>> snd_pcm_pause. Or perhaps simply lead ALSA into an underrun.
>>afaict pause, with reset mapped to drop,
>Indeed.
IMHO, the confusion around which of pause/reset/drop/drain applies
arises because Wine does not behave like mmdevapi closely enough.
All my tests converge on showing that native's processing occurs on
period ticks only(?/mostly) and that it sends data to the underlying HW devices
in chunks of period size frames. In particular, it does not send
megabytes of data down the pipe in advance.
If Wine were to write no more than a period worth of data to
ALSA/OSS/CoreAudio, then IAudioClient_Stop could do exactly what MSDN
says: "This method stops data from streaming through the client's
connection with the audio engine." IOW, just let the underlying
device enter an underrun, which the code must handle anyway.
There would be no need for any of snd_pcm_pause/drop/drain!
The need for pause etc. arises in Wine because it may feed a whole
buffer (200 times! period size, e.g. 2s vs 10ms) to ALSA/OSS/CoreAudio.
Ensues complexity and deviant behaviour as snd_pcm_drop is not
applicable since Stop is defined *not* to throw these 2s of data away,
snd_pcm_drain is not callable in non-blocking mode, snd_pcm_pause may
not be supported, etc.
OTOH, I recognize that sending 2s of data down the UNIX pipe lets one
sleep well and without glitches.
The question becomes: can Wine act like native's mmdevapi? Can it be
guaranteed sustained rates of wake-ups every 10ms? Was too tight
timing a known cause of glitches in the past such that we're very
happy that mmdevapi allows a much larger buffer than period size?
Is 100 wake-ups per second too much for a simple winmm:PlaySound on a
laptop runing on batteries?
Note that no game will probably use 2s buffers except for BGM. It
will expect tight timing and little latency (I'd say < 80ms, e.g. 2
video frames at 24fps).
On native, the question does not arise: mmdevapi is the low-level
interface to audio, part of the OS. The OS will make tremendous
efforts such as drive its audio engine(s) at the highest priority
("Audio Pro") (+ a couple other measures out of topic) so as to
guarantee a wake-up every 10ms.
Benefits I see in equating mmdevapi periods with ALSA/OSS periods:
- Get as close to ALSA/OSS recommended programming practices as
possible, e.g. may even end up using
a) blocking mode in the player;
b) snd_pcm_wait(period) to sleep for the next relevant event instead
of using a timer unrelated to actual audio throughput.
- Hopefully get low response times from Linux as sleeping on an event
then doing little processing fast is exactly what the kernel people
recommend for threads to be scheduled timely.
Benefits I see in throwing large data in advance at ALSA/OSS:
- Don't worry about glitches, let others care.
- Don't care about 10ms periods for BGM, PlaySound or MCIWAVE.
Regards,
Jörg Höhle
Re: mshtml: Use the last colon in proxy url as port separator
On 08/19/11 22:10, André Hentschel wrote: Am 18.08.2011 22:50, schrieb Juan Lang: Is the ProxyServer specified as an URL? If yes then use the proper API to dissect the URL instead of cobbling something together. Just to follow up in this idea, you could use InternetCrackUrl. mshtml delay loads wininet, but it also loads urlmon, which also loads wininet, so in effect wininet is always loaded and available. But a larger question is, why load the proxy settings from the registry only? If you query wininet for them instead, then the environment variable http_proxy is also checked. There was bug 5625 about this, which I set to fixed based on commit 80f02b82d68902f32578a7bcf6cfbaa715b724ce. I may have been mistaken. Under what circumstances is the network.proxy.http gecko setting still being used? Maybe for embedded content, e.g. IMG tags? If these were set by querying wininet instead, then at least we wouldn't have potentially different proxy settings for some HTTP requests than for others. --Juan just sent a patch doing that. I'm not sure for what winegecko uses it, but that function was added by intention and jacek already modified it a bit, so there must be a reason. set_proxy shouldn't be needed any more since we use urlmon for all connections. I didn't remove it immediately because it was relatively easy to restore the old behaviour for some time, but other pieces of the old code are already killed now and it's about the last piece left from before-urlmon times. Feel free to kill it. Cheers, Jacek
Re: [PATCH 5/5] shell32: Implement a default property sheet for file system objects
On 8/21/2011 19:21, Jay Yang wrote:
+LoadStringA(shell32_hInstance,IDS_PROPSHEET_VARIOUS,
+various_str,sizeof(various_str));
+LoadStringW(shell32_hInstance,IDS_PROPSHEET_COUNT_FORMAT,
+count_format,sizeof(count_format)/sizeof(WCHAR));
Why -A call here?
+WCHAR count_str[64];
Again some magic.
+static INT_PTR DefaultPropSheet_OnNotify(HWND hwndDlg,WPARAM wParam, NMHDR
*header)
+{
+switch(header->code)
+{
+case PSN_APPLY:
+{
+UINT index;
+PROPSHEETPAGEW *page;
+DefaultPropSheet *propsheet;
+WCHAR name[MAX_PATH];
+UINT readonly_state, hidden_state;
+UINT i;
+index =
SendMessageW(GetParent(hwndDlg),PSM_HWNDTOINDEX,(WPARAM)hwndDlg,0);
+page =
(PROPSHEETPAGEW*)SendMessageW(GetParent(hwndDlg),PSM_INDEXTOPAGE,index,0);
+propsheet = (DefaultPropSheet*)page->lParam;
+
+/*set name*/
+if(propsheet->cidl==1)
+{
+BOOL is_folder;
+LPCITEMIDLIST pidl_child;
+LPITEMIDLIST pidl,pidl_new,pidl_child_new,temp;
+IShellFolder *folder;
+WCHAR old_name[MAX_PATH];
+
GetDlgItemTextW(hwndDlg,IDC_PROPSHEET_NAME,name,sizeof(name)/sizeof(WCHAR));
+pidl = ILCombine(propsheet->folder_pidl,propsheet->apidl[0]);
+is_folder = _ILIsFolder(pidl);
+temp = ILClone(pidl);
+ILRemoveLastID(temp);
+
SHBindToParent(pidl,&IID_IShellFolder,(void**)&folder,&pidl_child);
+ILGetDisplayNameExW(folder, pidl_child, old_name,SHGDN_NORMAL);
+if(strcmpW(old_name,name)!=0)
+{
+
if(SUCCEEDED(IShellFolder_SetNameOf(folder,hwndDlg,pidl_child,name,SHGDN_NORMAL,&pidl_child_new)))
+{
+pidl_new = ILCombine(temp,pidl_child_new);
+SHChangeNotify(is_folder ? SHCNE_RENAMEFOLDER :
SHCNE_RENAMEITEM,SHCNF_IDLIST,pidl,pidl_new);
+ILFree(pidl_child_new);
+ILFree(pidl_new);
+}
+}
+ILFree(temp);
+ILFree(pidl);
+IShellFolder_Release(folder);
+}
+/*set readonly/hidden*/
+readonly_state =
IsDlgButtonChecked(hwndDlg,IDC_PROPSHEET_READONLY);
+hidden_state = IsDlgButtonChecked(hwndDlg,IDC_PROPSHEET_HIDDEN);
+for(i=0;icidl;i++)
+{
+LPITEMIDLIST pidl;
+WCHAR path[MAX_PATH];
+DWORD attributes;
+pidl = ILCombine(propsheet->folder_pidl,propsheet->apidl[0]);
+SHGetPathFromIDListW(pidl,path);
+ILFree(pidl);
+attributes = GetFileAttributesW(path);
+switch(readonly_state)
+{
+case BST_CHECKED:
+attributes |= FILE_ATTRIBUTE_READONLY;
+break;
+case BST_UNCHECKED:
+attributes&= ~FILE_ATTRIBUTE_READONLY;
+break;
+}
+switch(hidden_state)
+{
+case BST_CHECKED:
+attributes |= FILE_ATTRIBUTE_HIDDEN;
+break;
+case BST_UNCHECKED:
+attributes&= ~FILE_ATTRIBUTE_HIDDEN;
+break;
+}
+SetFileAttributesW(path,attributes);
+}
+break;
+}
+case PSN_RESET:
+{
+UINT index;
+PROPSHEETPAGEW *page;
+DefaultPropSheet *propsheet;
+index =
SendMessageW(GetParent(hwndDlg),PSM_HWNDTOINDEX,(WPARAM)hwndDlg,0);
+page =
(PROPSHEETPAGEW*)SendMessageW(GetParent(hwndDlg),PSM_INDEXTOPAGE,index,0);
+propsheet = ((DefaultPropSheet*)page->lParam);
+IShellPropSheetExt_Release(&propsheet->IShellPropSheetExt_iface);
+}
Missed 'break' probably?
+default:
+return FALSE;
+}
+return TRUE;
+}
Re: [PATCH 3/5] shell32: Implement SHOpenPropSheet
On 8/21/2011 19:20, Jay Yang wrote:
---
dlls/shell32/shell32.spec |3 ++
dlls/shell32/shellord.c | 88 +
include/shlobj.h |4 ++
3 files changed, 95 insertions(+), 0 deletions(-)
It's not clear for me why you placed this in shellord.c, it's supposed
to contain exported by ordinal calls only, as I understand it. If
Julliard approves new file for propsheets then you should place this in
it. IMO it's ok to place all of this in shlmenu.c for example.
+BOOL WINAPI SHOpenPropSheetA(LPCSTR pszCaption, HKEY *ahkey, UINT ckeys, const
CLSID *pclsidDef, IDataObject *pdtobj, IShellBrowser *psb, LPCSTR pStartPage)
+{
+BOOL ret;
+LPWSTR wszCaption,wszStartPage;
+UINT length;
+length = MultiByteToWideChar(CP_ACP, 0, pszCaption, -1, NULL, 0);
+wszCaption = HeapAlloc(GetProcessHeap(),0,sizeof(WCHAR)*length);
+length = MultiByteToWideChar(CP_ACP, 0, pszCaption, -1, wszCaption,
length);
+length = MultiByteToWideChar(CP_ACP, 0, pStartPage, -1, NULL, 0);
+wszStartPage = HeapAlloc(GetProcessHeap(),0,sizeof(WCHAR)*length);
+length = MultiByteToWideChar(CP_ACP, 0, pStartPage, -1, wszStartPage,
length);
+ret =
SHOpenPropSheetW(wszCaption,ahkey,ckeys,pclsidDef,pdtobj,psb,wszStartPage);
+HeapFree(GetProcessHeap(),0,wszCaption);
+HeapFree(GetProcessHeap(),0,wszStartPage);
+return ret;
+}
Hmm, actually there's HEAP_strdupAtoW() for that (you need to make in
non-static though).
+ ZeroMemory(&psh, sizeof(PROPSHEETHEADERW));
+ psh.dwSize = sizeof (PROPSHEETHEADERW);
+ psh.hwndParent = NULL;
+ psh.dwFlags = PSH_PROPTITLE;
+if(pStartPage)
+{
+psh.dwFlags |= PSH_USEPSTARTPAGE;
+psh.u2.pStartPage = pStartPage;
+}
Please don't mix tabs with spaces, use 4 spaces everywhere.
+if(pszCaption)
+psh.pszCaption = pszCaption;
+else
+psh.pszCaption = empty_str;
Is it really needed? I mean pszCaption == NULL is not the same as empty
string in this context?
+IShellPropSheetExt *def_ext;
+IShellExtInit *init_iface;
+PROPSHEETHEADERW psh;
+ HPROPSHEETPAGE pages[99];
It's not clear again where this size comes from, just enough for anyone?
Also I think it's better to reduce scope as much as possible, def_ext
doesn't need to be global for example.
Re: [PATCH 2/5] shell32: Use SHCreateDefaultContextMenu to create context menus (try 2)
On 8/21/2011 19:20, Jay Yang wrote:
Changes from last time:
Factored out the code to find the registry keys for the extensions.
---
dlls/shell32/classes.c| 47 +
dlls/shell32/shell32_main.h |1 +
dlls/shell32/shfldr.h |9 +++
dlls/shell32/shfldr_desktop.c |8 ++
dlls/shell32/shfldr_fs.c |7 +
dlls/shell32/shfldr_mycomp.c |4 +--
dlls/shell32/shfldr_unixfs.c | 11 ++---
dlls/shell32/shlmenu.c| 40 ++
dlls/shell32/shlview.c| 44 +-
9 files changed, 139 insertions(+), 32 deletions(-)
+HRESULT SHELL_CreateItemContextMenu(HWND hwnd, IShellFolder *folder,
+LPITEMIDLIST folder_pidl,
+LPCITEMIDLIST *apidl, UINT cidl,
+REFIID riid, void **ppv)
+{
+DEFCONTEXTMENU menu_info =
{hwnd,NULL,folder_pidl,folder,cidl,apidl,NULL,0,NULL};
+UINT cKeys = 0;
+HKEY aKeys[16] = {NULL};
Where size of 16 comes from?
+UINT i;
+HRESULT hres;
+cKeys = HCR_GetExtensionsKeysForPidl(apidl[0],aKeys);
+menu_info.cKeys=cKeys;
+menu_info.aKeys=aKeys;
+hres = SHCreateDefaultContextMenu(&menu_info,riid,ppv);
+for(i=0;i
RegCloseKey already check for NULL handle.
+hr =
IShellFolder_CreateViewObject(This->pSFParent,This->hWndParent,&IID_IContextMenu,(void**)&pCM);
+if(FAILED(hr)){
+LPITEMIDLIST folder_pidl;
+IPersistFolder2 *persist;
+
IShellFolder_QueryInterface(This->pSFParent,&IID_IPersistFolder2,(void**)&persist);
+IPersistFolder2_GetCurFolder(persist,&folder_pidl);
+IPersistFolder2_Release(persist);
+hr=
SHELL_CreateBackgroundContextMenu(This->hWndParent,This->pSFParent,
+
folder_pidl,&IID_IContextMenu,
+ (void**)&pCM);
+ILFree(folder_pidl);
+}
I'm not sure about that, but it looks like it deserves some check for
'persist' pointer. Or better a return value being S_OK (from query).
Re: [PATCH 1/5] shell32: Implement loading of context menu shell extensions (try 3)
On 8/21/2011 19:20, Jay Yang wrote:
The first two patches in the sequence relate to context menus and the last
three to property sheets,
but some of the changes in the last three depend on a function I created in the
second patch so I've
sent them as a sequence.
Changes from last time (for this file):
cleaned up the code.
---
dlls/shell32/shlmenu.c | 170 ++-
1 files changed, 166 insertions(+), 4 deletions(-)
+HKEY *new_keys=NULL;
+new_keys = HeapAlloc(GetProcessHeap(),0,sizeof(HKEY)*cKeys);
No need to initialize that explicitly.
+ULONG status =
RegOpenKeyExW(aKeys[i],cmenu_handler_key,0,KEY_READ,&new_key);
+if(status==ERROR_SUCCESS)
This call return LSTATUS which is signed LONG. A common way to test
return value is to use (!RegOpen*()).
+CLSID *clsid_table=NULL;
...
+clsid_table =
HeapAlloc(GetProcessHeap(),HEAP_ZERO_MEMORY,sizeof(CLSID)*handler_count);
Same here, no need to NULL it.
+static const CLSID zero_clsid = {0};
+CLSID *curr_clsid = clsid_table+j;
+if(IsEqualCLSID(curr_clsid,&zero_clsid))
There's CLSID_NULL for that.
+TRACE("Loading shell extension with clsid
%s\n",shdebugstr_guid(&id));
I'm not sure shdebugstr_guid() is supposed to know shell extension names.
+if(new_keys[i])
+RegCloseKey(new_keys[i]);
This null check is redundant.
