Re: [PATCH 1/2] user32: Add tests for MapWindowPoints, ClientToScreen and ScreenToClient. (try 3)
Le 13/11/2012 23:07, Marvin a écrit : 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=22927 Your paranoid android. === WXPPROSP3 (32 bit win) === win.c:881: Test failed: wrong dwStyle: 84cb != 94cb for 000E0138 in hook HCBT_ACTIVATE Not related to my patch. Altough it's not related it's curious because the test bot never shows failure in win.c when I submitted all previous versions of the patch and I didn't see any change in win.c since the last one (2 days ago).
Re: [PATCH 1/2] user32: Add tests for MapWindowPoints, ClientToScreen and ScreenToClient. (try 3)
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=22927 Your paranoid android. === WXPPROSP3 (32 bit win) === win.c:881: Test failed: wrong dwStyle: 84cb != 94cb for 000E0138 in hook HCBT_ACTIVATE
Re: [PATCH 2/2] usp10: Fixed wgBlank, wgDefault, wgInvalid and wgKashida.
Hello,
Thanks for comment too!
On Wed, Nov 14, 2012 at 3:25 AM, Nikolay Sivov wrote:
> I think it's better to avoid macros if possible.
>>
Got it.
>> +if (!sc->sfnt)
>> +{
>> +if (GetTextMetricsW(hdc, &tmW))
>> +sc->sfp.wgBlank = tmW.tmBreakChar;
>> +else
>> +{
>> +ERR("Get wgBlank failed, fallback to Numeric_space.\n");
>> +sc->sfp.wgBlank = Numeric_space;
>> +}
>
> If it's a normal fallback case it's not an error I guess.
>
It's not a normal fallback, GetTextMetricsW() should not fail, and I
didn't see fail here for all fonts I tested.
Should I remove the else{} block?
>
>> +{
>> +#define Zero_width_space 0x200b /* Zero Width Space */
>> +#define Invalid_char3 0xf71b /* Unknow, found by black box testing */
>> +#define NON_EXIST_INDEX 0x /* Default non exist char index */
>> +WCHAR invalid_chars[3] = {Numeric_space, Zero_width_space,
>> Invalid_char3};
>
> I don't see why this can't be hardcoded without macros and one line comment
> about meaning of this.
Thanks, will change that.
>>
>> +#define Kashida_char0x0640 /* Kashida */
>> +WCHAR kashida_chars[] = {Kashida_char};
>
> Same here, also naming is a bit strange. I think it's enough to name it
> 'kashida' and make it 'static const WCHAR'.
Good point, thanks Nikolay.
Re: [PATCH 2/2] usp10: Fixed wgBlank, wgDefault, wgInvalid and wgKashida.
Hello, On Wed, Nov 14, 2012 at 2:22 AM, Alexandre Julliard wrote: > There's no reason to add #defines for that kind of thing, just put the > values in the string directly. Thanks for comment, will remove these #defines. -- Regards, Qian Hong - Sent from Ubuntu http://www.ubuntu.com/
Re: [PATCH 2/2] usp10: Fixed wgBlank, wgDefault, wgInvalid and wgKashida.
On 11/13/2012 18:20, Qian Hong wrote:
Thanks Aric!
---
dlls/usp10/usp10.c | 133 +--
dlls/usp10/usp10_internal.h |1 +
2 files changed, 128 insertions(+), 6 deletions(-)
I think it's better to avoid macros if possible.
+if (!sc->sfnt)
+{
+if (GetTextMetricsW(hdc, &tmW))
+sc->sfp.wgBlank = tmW.tmBreakChar;
+else
+{
+ERR("Get wgBlank failed, fallback to Numeric_space.\n");
+sc->sfp.wgBlank = Numeric_space;
+}
If it's a normal fallback case it's not an error I guess.
+{
+#define Zero_width_space 0x200b /* Zero Width Space */
+#define Invalid_char3 0xf71b /* Unknow, found by black box testing */
+#define NON_EXIST_INDEX 0x /* Default non exist char index */
+WCHAR invalid_chars[3] = {Numeric_space, Zero_width_space, Invalid_char3};
I don't see why this can't be hardcoded without macros and one line
comment about meaning of this.
+#define Kashida_char0x0640 /* Kashida */
+WCHAR kashida_chars[] = {Kashida_char};
Same here, also naming is a bit strange. I think it's enough to name it
'kashida' and make it 'static const WCHAR'.
Re: [PATCH 2/2] usp10: Fixed wgBlank, wgDefault, wgInvalid and wgKashida.
Qian Hong writes:
> +static inline void set_cache_invalid_char(const HDC hdc, ScriptCache *sc)
> +{
> +#define Zero_width_space 0x200b /* Zero Width Space */
> +#define Invalid_char3 0xf71b /* Unknow, found by black box testing */
> +#define NON_EXIST_INDEX 0x /* Default non exist char index */
> +WCHAR invalid_chars[3] = {Numeric_space, Zero_width_space,
> Invalid_char3};
There's no reason to add #defines for that kind of thing, just put the
values in the string directly.
--
Alexandre Julliard
julli...@winehq.org
Re: Missing dependencies on static libraries
Francois Gouget writes: > On Mon, 12 Nov 2012, Alexandre Julliard wrote: >> In theory yes, but that would have to be auto-generated. In practice >> only strmbase should matter. > > I'm not sure how to auto-generate it. Would something that looks like > this be ok: > > In Makefile.in: > > STATICIMPORTS = strmiids strmbase uuid > IMPORTS = $(STATICIMPORTS) ole32 advapi32 > > And in Makedll.rules.in: > > $(MODULE).so: $(STATICIMPORTS:%=$(top_builddir)/dlls/%/lib%.a) > > Or maybe: > > $(MODULE).so: $(STATICIMPORTS:%=../dlls/%/lib%.a) In theory it should depend on all the import libs of course, but either way I don't think you can do that with standard makefile rules. > Or should explicit rules be added to the relevant makefiles by > update_makefiles() in tools/make_makefiles. It would have to be done by a script, but I don't think we want to have to put that sort of thing in the Makefile.in. I'd suggest to ignore the issue for now. -- Alexandre Julliard julli...@winehq.org
Re: [PATCH 3/6] dsound: create a primary_pwfx separately from pwfx
Hey, Op 19-10-12 15:29, Andrew Eikum schreef: > Patches 1 and 2 in this series look fine. > > I have a series of patches similar to this one in my dsound > multichannel branch. This patch seems to do too much at once. > > When I did this cleanup, I split it up into four patches: > 1) Allocate the device format in the Device struct > 2) Load the default format from the IAC, not hard-coded in dsound > 3) Put SetFormat() calls into their own WFX that we don't really care > about > 4) Always use WAVE_FORMAT_EXTENSIBLE for the device format Tried to revisit it and split it up, but there's no sense in keeping it separate. It's in fact quite impossible to do so because of how dsound works. What I'm doing is the smallest possible thing that makes sense. I want to have writeprimary mode separately, so the commit does pretty much that. When toggling writeprimary mode you reopen the device, those parts are in dsound.c, and you need to change the wave format. Those parts in primary.c do that. It may look like a patch, but all the parts are needed. Splitting this up will just break it, or end up with something that doesn't make sense, some of the changes look big but it's not that big a change. 1. DSOUND_ReopenDevice format changes: - 3 cases, in !writeprimary mode it tries to allocate GetMixFormat as float, and clips channels to 2. If writeprimary, it will try to allocate a waveformatextensible for PCM and IEEE float if possible, and use that. If not possible, it just copies format unmodified. Then calls isformatsupported to get the proper format, and initializes with that. 2. primarybuffer_SetFormat changes: - some removal of code, if !writeprimary, format is not modified, but copied from the internal format instead. if writeprimary, device is reopened like before. Some format recalculations have been moved to PrimaryOpen. 3. SetCooperativeLevel changes: - Just reopen device when toggling writeprimary, straightforward. I would love to split this patch up further, but I ended up with patches that didn't make sense from that. ~Maarten
Re: Missing dependencies on static libraries
On Mon, 12 Nov 2012, Alexandre Julliard wrote: [...] > > Would somthing like the patch below be acceptable? > > Should it be generalized to all the dependencies on uuid, dxguid, etc? > > In theory yes, but that would have to be auto-generated. In practice > only strmbase should matter. I'm not sure how to auto-generate it. Would something that looks like this be ok: In Makefile.in: STATICIMPORTS = strmiids strmbase uuid IMPORTS = $(STATICIMPORTS) ole32 advapi32 And in Makedll.rules.in: $(MODULE).so: $(STATICIMPORTS:%=$(top_builddir)/dlls/%/lib%.a) Or maybe: $(MODULE).so: $(STATICIMPORTS:%=../dlls/%/lib%.a) Or should explicit rules be added to the relevant makefiles by update_makefiles() in tools/make_makefiles. -- Francois Gouget http://fgouget.free.fr/ In theory, theory and practice are the same, but in practice they're different.
Re: [PATCH 2/2] gdi32: Added GetGlyphOutlineW tests on glyph that contains empty contour (try3)
Piotr Caban writes: > On 11/13/12 13:10, Dmitry Timoshkov wrote: >> Piotr Caban wrote: >> >>>create mode 100644 dlls/gdi32/tests/test_empty_contour.sfd >>>create mode 100644 dlls/gdi32/tests/test_empty_contour.ttf >> >> Is it possible to add the desired glyph to an already existing test font? >> > Yes, I was adding it first to wine_test.ttf. In this case > GetGlyphOutline tests needs to be added in the middle of > CreateScalableFontResource tests (at least as long as > RemoveFontResource is not implemented). > > I can change it to work this way if it's preferred. If it's possible, that's certainly better. -- Alexandre Julliard julli...@winehq.org
Re: [PATCH 2/2] gdi32: Added GetGlyphOutlineW tests on glyph that contains empty contour (try3)
On 11/13/12 13:10, Dmitry Timoshkov wrote: Piotr Caban wrote: create mode 100644 dlls/gdi32/tests/test_empty_contour.sfd create mode 100644 dlls/gdi32/tests/test_empty_contour.ttf Is it possible to add the desired glyph to an already existing test font? Yes, I was adding it first to wine_test.ttf. In this case GetGlyphOutline tests needs to be added in the middle of CreateScalableFontResource tests (at least as long as RemoveFontResource is not implemented). I can change it to work this way if it's preferred.
Re: [PATCH 2/2] gdi32: Added GetGlyphOutlineW tests on glyph that contains empty contour (try3)
Piotr Caban wrote: > create mode 100644 dlls/gdi32/tests/test_empty_contour.sfd > create mode 100644 dlls/gdi32/tests/test_empty_contour.ttf Is it possible to add the desired glyph to an already existing test font? -- Dmitry.
Re: Any advice please... Console input issue
> I tested the following
> HANDLE hConRW = CreateFileA("CONIN$"
> , GENERIC_READ | GENERIC_WRITE,
> FILE_SHARE_READ, NULL, OPEN_EXISTING,
> FILE_ATTRIBUTE_NORMAL, 0);
> if (hConRW == INVALID_HANDLE_VALUE) hConRW = handle;
> ret = bare_console_fetch_input(hConRW, fd, timeout);
> if (hConRW!=handle) CloseHandle(hConRW);
>
> It works a charm, although feels inefficient for the times handle was
> already r/w - (it feels slightly odd that the 'handle' we are working on is
> unrelated to the handle passed in (I was worried about stdin redirection,
> but that seems to work fine even after the change).
>
> Was this what you meant or have I gone off on a tangent?
more or less. I was thinking to add a helper functions for the
writeconsoleinput calls in bare_console_fetch_input.
and this helper function would :
- try to call writeconsoleinput with the current console input handle
- if it fails with status_access_denied, then create the right handle as you did
- from your code, please use CreateFileW instead of CreateFileA
A+
--
--
Eric Pouech
Re: [PATCH] wininet: handle failing create_netconn_socket (Coverity)
Marcus Meissner writes: > A failing here will free the netconn structure and we should > not use it afterwards. It probably shouldn't free it, this can break in other places too. -- Alexandre Julliard julli...@winehq.org
