Re: server: Android has struct user in asm/user.h

2013-09-24 Thread Huw Davies

 On 24 Sep 2013, at 17:57, André Hentschel n...@dawncrow.de wrote:
 
 is that for the warning about the missing casts?

No, this actually introduces more :-)

The cast warning is about the 4th param of ptrace.

Huw.



Re: [PATCH 2/3] iphlpapi: Add interface enumeration for Android.

2013-09-19 Thread Huw Davies

On 19 Sep 2013, at 15:07, Bruno Jesus wrote:
 Hi, Huw. I'm not sure this is an issue but in the android
 implementation of get_interface_indexes you are not setting
 
 +if (table) *table = NULL;

Hi Bruno,

Good catch!  Resent.

Thanks,
Huw.





Re: fonts: Add some box-type glyphs to wingdings.

2013-09-13 Thread Huw Davies
On Fri, Sep 13, 2013 at 11:07:19AM +0100, Huw Davies wrote:
 ---
  fonts/wingding.sfd | 86 +

Please ignore this one and use 'try 2'.

I had the points of one glyph selected in the outline editor which
apparently gets saved to the .sfd .

Huw.




Re: riched20: Use \ulnone instead of \ul0 for no underline.

2013-08-20 Thread Huw Davies
On Mon, Aug 19, 2013 at 05:38:13PM -0500, Vincent Povirk wrote:
 Testing on Windows (by saving rtf documents with wordpad and viewing
 them with notepad) shows that it uses \ulnone instead of \ul0 to end
 underline. Wine and Windows are both able to read either one.

I think these are symonyms and indeed Windows seems to use \ulnone, so
this looks good to me.

Huw.




Re: gdi32: Printer drivers don't use the character extra spacing if lpdx is supplied.

2013-03-26 Thread Huw Davies
On Tue, Mar 26, 2013 at 04:30:14PM +0100, Alexandre Julliard wrote:
 Huw Davies h...@codeweavers.com writes:
 
  Ideally this would be done in wineps.drv, but by that point the conversion 
  to device units would cause rounding errors.
 
 What about GetTextExtent functions?  Do they need this too?  Some test
 cases would be nice.

Note this only applies when lpDx != NULL, when it's NULL then
char_extra is still applied.  So, yes the current GetTextExtent
behaviour is correct.

Huw.





Re: gdi32: Printer drivers don't use the character extra spacing if lpdx is supplied.

2013-03-26 Thread Huw Davies
On Tue, Mar 26, 2013 at 04:50:03PM +0100, Alexandre Julliard wrote:
 Ah OK, I guess that makes it a PITA to test...

Some kind of print-scan magic comes to mind ;-)




Re: winspool.drv: Fallback to cupsGetPPD if cupsGetPPD3 is missing or failed.

2013-02-11 Thread Huw Davies
On Mon, Feb 11, 2013 at 02:03:26PM +0800, Dmitry Timoshkov wrote:
 -if (pcupsGetPPD3) return pcupsGetPPD3( http, name, modtime, buffer, 
 bufsize );
 +if (pcupsGetPPD3)
 +{
 +http_status_t status;
 +
 +TRACE( Calling cupsGetPPD3\n );
 +status = pcupsGetPPD3( http, name, modtime, buffer, bufsize );
 +if (status == HTTP_OK) return HTTP_OK;
 +TRACE(cupsGetPPD3 failed (status %d)\n, status);
 +}
  

Under what conditions does cupsGetPPD succeed if cupsGetPPD3 fails?

Huw.




Re: winspool.drv: Fallback to cupsGetPPD if cupsGetPPD3 is missing or failed.

2013-02-11 Thread Huw Davies
On Mon, Feb 11, 2013 at 06:12:57PM +0800, Dmitry Timoshkov wrote:
 Huw Davies h...@codeweavers.com wrote:
  Under what conditions does cupsGetPPD succeed if cupsGetPPD3 fails?
 
 I don't have full understanding of what is going on, but sometimes (!)
 cupsGetPPD3 returns 404, while cupsGetPPD works just fine here. Perhaps
 that's just another Ubuntu quirk.

Seems odd as internally cupsGetPPD just calls cupsGetPPD3.

Huw.




Re: riched20: ME_StrDup() is no longer used so remove it.

2013-01-29 Thread Huw Davies
On Tue, Jan 29, 2013 at 07:05:13AM +0100, Francois Gouget wrote:
 ---
  dlls/riched20/editor.h |1 -
  dlls/riched20/string.c |5 -
  2 files changed, 6 deletions(-)

I'm about to send in a patch that uses this again, so please hold off
on this one.

Huw.




Re: usp10: Implement GPOS MarkToLigature Attachment Positioning Subtable

2013-01-09 Thread Huw Davies
On Wed, Jan 09, 2013 at 11:36:51AM -0600, Aric Stewart wrote:
 +typedef struct {
 +WORD LigatureAnchor[1];
 +} GPOS_ComponentRecord;
 +
 +typedef struct {
 +WORD ComponentCount;
 +GPOS_ComponentRecord ComponentRecord[1];
 +} GPOS_LigatureAttach;
 +

 +for (i = 0; i  component_count  !offset; i++)
 +{
 +int k;
 +for (k = 0; k  class_count  !offset; k++)
 +offset = 
 GET_BE_WORD(lt-ComponentRecord[i].LigatureAnchor[k]);
 +}

Hi Aric,

This can't work as ComponentRecord is a variable-sized struct.

Huw.




Re: [PATCH 3/3] usp10: Add support for format 2 pair adjustments.

2012-12-20 Thread Huw Davies
On Thu, Dec 20, 2012 at 10:12:38PM +0800, Dmitry Timoshkov wrote:
 Huw Davies h...@codeweavers.com wrote:
 
   typedef struct {
  +WORD PosFormat;
  +WORD Coverage;
  +WORD ValueFormat1;
  +WORD ValueFormat2;
  +WORD ClassDef1;
  +WORD ClassDef2;
  +WORD Class1Count;
  +WORD Class2Count;
  +WORD Class1Record[1];
  +} GPOS_PairPosFormat2;
 
 Shouldn't this structure be 2 bytes aligned?

I don't see why.  They're all WORDs.

Huw.




Re: [PATCH 3/3] usp10: Add support for format 2 pair adjustments.

2012-12-20 Thread Huw Davies
On Thu, Dec 20, 2012 at 11:05:48PM +0800, Dmitry Timoshkov wrote:
 Huw Davies h...@codeweavers.com wrote:
 
 typedef struct {
+WORD PosFormat;
+WORD Coverage;
+WORD ValueFormat1;
+WORD ValueFormat2;
+WORD ClassDef1;
+WORD ClassDef2;
+WORD Class1Count;
+WORD Class2Count;
+WORD Class1Record[1];
+} GPOS_PairPosFormat2;
   
   Shouldn't this structure be 2 bytes aligned?
  
  I don't see why.  They're all WORDs.
 
 There are 9 elements of 2 bytes in size each, 18 bytes in total,
 the compiler will align this structure to 4 bytes, making its
 size 20 bytes.

The size is irrelevant - it's variable sized anyway.  We just care
about the offsets of the elements.




Re: [PATCH 3/3] usp10: Add support for format 2 pair adjustments.

2012-12-20 Thread Huw Davies
On Thu, Dec 20, 2012 at 11:26:36PM +0800, Dmitry Timoshkov wrote:
 Huw Davies h...@codeweavers.com wrote:
  The size is irrelevant - it's variable sized anyway.  We just care
  about the offsets of the elements.
 
 Anyway, in order to avoid any confusion or bugs in future with allocating
 an array of such structures or using sizeof() on them I'd suggest to
 explicitely specify alignment.

Using an array of these structures or using sizeof() is wrong anyway
as they are variable-sized...




Re: winex11: Add a few 'fall through' comments.

2012-11-27 Thread Huw Davies
On Tue, Nov 27, 2012 at 06:44:35PM +0800, Dmitry Timoshkov wrote:
 case DirectColor:
   X11DRV_PALETTE_PaletteFlags |= X11DRV_PALETTE_VIRTUAL;
 /* fall through */
 case GrayScale:
 
 would be more clear IMHO. Right now it's not obvious to which case statement
 'fall through' belongs to.

Resent, thanks.

Huw.




Re: [PATCH 1/2] oleaut32/tests: Add test for ITypeInfo2_fnGetContainingTypeLib [try 2]

2012-11-08 Thread Huw Davies
On Thu, Nov 08, 2012 at 06:13:28PM +0400, Tatyana Fokina wrote:
 +hr = CreateTypeLib2(SYS_WIN32, jsdeb, (ICreateTypeLib2 **)ctl2);
 +ok_ole_success(hr, CreateTypeLib2);
 +
 +hr = ICreateTypeLib2_CreateTypeInfo(ctl2, jsname, TKIND_DISPATCH, cti);
 +ok_ole_success(hr, ICreateTypeLib2_CreateTypeInfo);

Please don't use obscure strings like 'jsdeb.tlb' use something
like 'test.tlb'.  You don't need the casts here or further down.

 +
 +hr = ICreateTypeInfo_QueryInterface(cti, IID_ITypeInfo2, (void**)ti2);
 +ok_ole_success(hr, ICreateTypeInfo2_QueryInterface);
 +if (SUCCEEDED(hr))
 +{

There's no need to put this inside an if block.  QI isn't going to fail.
Also your indentation is first four spaces, then two - keep it at four.

Huw.




Re: [PATCH 2/2] oleaut32: If pointers (ppTLib or pIndex in ITypeInfo2_fnGetContainingTypeLib) are null, we simply ignore it.[resend]

2012-11-08 Thread Huw Davies
On Thu, Nov 08, 2012 at 06:13:33PM +0400, Tatyana Fokina wrote:
 +if(ppTLib)
 +{
  *ppTLib = (ITypeLib *)This-typelib-ITypeLib2_iface;
  ICreateTypeLib_AddRef((ICreateTypeLib*)This-typelib);
 +}
 +
 +if(pIndex)
  *pIndex = This-typeinfo-typekind  16;

Please indent.




Re: vbscript/tests: Skip some of the weekday tests if the first day of the week is not Sunday.

2012-10-31 Thread Huw Davies
On Wed, Oct 31, 2012 at 12:34:49PM +0100, Jacek Caban wrote:
 Did you see those failures on Windows? test.winehq.org shows only some
 Linux failures, which suggests that the problem is in implementation,
 not tests.

Hi Jacek,

You're right.  The current tests all pass on my Windows box with the
exception of
WeekDayName(1, true, 0) = Mon
which is the system default.  Not sure how to cleanly skip this one.

I'll send in a patch for the implementation.

Thanks,
Huw.




Re: ole32: Add CoGetDefaultContext stub

2012-09-19 Thread Huw Davies
On Wed, Sep 19, 2012 at 09:09:55AM +0200, Michael Stefaniuc wrote:
  +/***
  + *   CoGetContextToken [OLE32.@]
 copy and paste error ^^^
 
  + */
  +HRESULT CoGetDefaultContext(APTTYPE type, REFIID riid, LPVOID *ppv)

...and a missing WINAPI.

Huw.




Re: localspl: Only store the file part of the filenames in the registry

2012-05-16 Thread Huw Davies

On 16/05/2012 17:22, Detlef Riekenberg wrote:

We have only filenames in the registry.
The current code works fine with the adobe postscript driver
and various other driver for PDF printers.



No, the current code copies the entire contents of the DRIVER_INFO struct's
filename entries - this isn't what Windows does.  For example it clearly
doesn't make sense the copy the entire path if APD_COPY_FROM_DIRECTORY
is specified...  I've written tests to confirm this, but they can't
be added to the test suite, since we'd need a real driver file for this
to succeed.

Huw.







Re: [1/2] gdi32: GetGlyphOutline should fail for a bitmap font.

2012-04-30 Thread Huw Davies
On Sat, Apr 28, 2012 at 05:52:13PM +0900, Dmitry Timoshkov wrote:
 ---
  dlls/gdi32/freetype.c   |6 ++
  dlls/gdi32/tests/font.c |   17 -
  2 files changed, 18 insertions(+), 5 deletions(-)

This will break the display of bitmap fonts in winex11.drv .

Do you have an app that depends on this?

Huw.




Re: [2/2] gdi32: Enumerated font size should not be too large.

2012-04-23 Thread Huw Davies

On 23/04/2012 15:32, Dmitry Timoshkov wrote:

This reverts commit f4625d1ae1109ee9a30faa8254b10779853f0ac2.
---
  dlls/gdi32/freetype.c   |2 +-
  dlls/gdi32/tests/font.c |1 +
  2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/dlls/gdi32/freetype.c b/dlls/gdi32/freetype.c
index 3875e4f..401cfd1 100644
--- a/dlls/gdi32/freetype.c
+++ b/dlls/gdi32/freetype.c
@@ -5048,7 +5048,7 @@ static void GetEnumStructs(Face *face, LPENUMLOGFONTEXW 
pelf,
  font = alloc_font();

  if(face-scalable) {
-height = -2048; /* 2048 is the most common em size */
+height = 100;
  width = 0;
  } else {
  height = face-size.y_ppem  6;


You'll need to fix the ntmTm.ntmCellHeight and ntmTm.ntmAvgWidth values.

Huw.




Re: [2/2] gdi32: Enumerated font size should not be too large.

2012-04-23 Thread Huw Davies

On 23/04/2012 16:07, Dmitry Timoshkov wrote:

Huw Daviesh...@codeweavers.com  wrote:

You'll need to fix the ntmTm.ntmCellHeight and ntmTm.ntmAvgWidth values.


As far as I can see that code always have been that way, and reverted patch
didn't touch it either. But I see your point, and will add the tests (and
a fix if the tests prove it's correct) in a later patch.


Perhaps the original commit was added to work around this issue?

Note, you'll probably have to retrieve the metrics at the design size (2048)
in order to get accurate values for these members.  Simply up-scaling the values
obtained at 100 ppem will lead to rounding errors.  As you say, tests are 
needed.

Huw.





Re: gdi32: Add support for emulating bold faces of bitmap fonts.

2012-04-16 Thread Huw Davies
On Mon, Apr 16, 2012 at 04:37:39PM +0900, Dmitry Timoshkov wrote:
 diff --git a/dlls/gdi32/freetype.c b/dlls/gdi32/freetype.c
 index 0a5d3c3..eb3f9b5 100644
 --- a/dlls/gdi32/freetype.c
 +++ b/dlls/gdi32/freetype.c
 @@ -5669,12 +5669,27 @@ static inline BYTE get_max_level( UINT format )
  return 255;
  }
  
 -static const BYTE masks[8] = {0x80, 0x40, 0x20, 0x10, 0x08, 0x04, 0x02, 
 0x01};
 +static void emulate_bold_glyph(BYTE *buf, int pitch, int height)
 +{
 +int x, y;
 +BYTE *p = buf;
 +
 +for (y = 0; y  height; y++)
 +{
 +for (x = pitch - 1; x = 0; x--)
 +{
 +p[x] |= p[x]  1;
 +if (x  0) p[x] |= p[x - 1]  7;
 +}
 +p += pitch;
 +}
 +}
  

Don't we need to add some bytes if glyph_width % 32 == 0 ?

Huw.




Re: gdi32: Add support for emulating bold faces of bitmap fonts.

2012-04-16 Thread Huw Davies
On Mon, Apr 16, 2012 at 05:51:20PM +0900, Dmitry Timoshkov wrote:
 Huw Davies h...@codeweavers.com wrote:
 
  Don't we need to add some bytes if glyph_width % 32 == 0 ?
 
 Well, I didn't see in my tests a need for that.

That may be because your tests are just looking at the font metrics
and not at the glyphs themselves...




Re: (try 2)quartz: add control.idl as a typelib resource

2012-04-05 Thread Huw Davies
On Thu, Apr 05, 2012 at 07:51:13AM -0500, Aric Stewart wrote:
 
 revised based on comments from Jacek Caban
 ---
  dlls/quartz/Makefile.in|1 +
  dlls/quartz/control_v2.idl |   17 +
  2 files changed, 18 insertions(+), 0 deletions(-)
  create mode 100644 dlls/quartz/control_v2.idl

Calling this control_v2.idl seems a little odd.  control_tlb.idl may
be better.

Huw.




Re: [PATCH 3/4] winspool/tests: Add some tests for OpenPrinter with non-NULL defaults.

2012-04-03 Thread Huw Davies
On Tue, Apr 03, 2012 at 12:50:18PM +0200, Marvin wrote:
 === WINEBUILD (build) ===
 Can't determine base name

A winspool vs winspool.drv thing perhaps?

Huw.




Re: gdi32: Add support for loading scalable font resources.

2012-03-30 Thread Huw Davies
On Fri, Mar 30, 2012 at 07:31:54PM +0900, Dmitry Timoshkov wrote:
 Huw Davies h...@codeweavers.com wrote:
 
  +if (size.u.LowPart  sizeof( *dos )) goto fail;
 
 Testing LowPart only could not work for large sizes.

Of course, but we don't care if very large files fail, they won't be
genuine resource files anyway.

Huw.




Re: 80135: [PATCH 05/10] gdi32: Implement SetPixel.

2011-10-19 Thread Huw Davies
On Wed, Oct 19, 2011 at 06:46:49AM -0700, Dan Kegel wrote:
 Patches 5-7 failed on WINEDEBUG=warn+heap here:
 
 ../../../tools/runtest -q -P wine -M gdi32.dll -T ../../.. -p
 gdi32_test.exe.so palette.c  touch palette.ok
 ...
 palette.c:104: Test failed: getColor=0028
 
 Might be flaky; patch 8 passed.  patches 9 and 10 aren't done testing yet.

It's because I wanted to move SetPixel and GetPixel in separate
patches.  I added a temporary todo_wine in patch 5 (SetPixel) to
prevent this test failure, which then got removed in patch 8
(GetPixel).

Huw.




Re: 80135: [PATCH 05/10] gdi32: Implement SetPixel.

2011-10-19 Thread Huw Davies
On Wed, Oct 19, 2011 at 07:04:02AM -0700, Dan Kegel wrote:
 Sounds like your temporary todo_wine didn't quite do the trick...?

Ah, I see.  I was getting a failure on 111 and not 104, presumably due
to different XServers.  Anyway they all pass again after the entire
series is applied.

Huw.




Re: patch for dlls/gdi32/dib.c: fixes crash

2011-07-28 Thread Huw Davies
On Thu, Jul 28, 2011 at 04:14:56PM +0200, Wolfgang Walter wrote:
 I think that
 
   bitmapinfo_from_user_bitmapinfo()
 
 is the real culprit. I think colors gets to big ( 256) and therefore the 
 size 
 for the memcpy.

Hopefully http://source.winehq.org/patches/data/77076 should fix this.

Huw.




Re: Statur of DIB Engine

2011-07-25 Thread Huw Davies
On Sun, Jul 24, 2011 at 07:10:46PM +0300, Octavian Voicu wrote:
 Disclaimer: these comments are based only on what I gather from
 following commits and looking at the code, so can't guarantee it's
 100% accurate; Huw or Alexandre would know better.

This is a good summary of where we're at - nicely done Octavian.

The next step is cross-device blitting, which is what Alexandre and I
are working on at the moment.

Huw.




Re: ole32/ole2: Don't call IDropTarget::QueryInterface() in RegisterDragDrop(). [whitespace fixed]

2011-05-26 Thread Huw Davies
On Wed, May 25, 2011 at 12:45:15PM -0500, Adam Martinson wrote:
 On 05/25/2011 12:36 PM, Adam Martinson wrote:
 The AddRef is done in WrapDropTarget(), seems like the appropriate
 place for it.  Quite right on the rest though, thanks.
 
 Oops no, the window prop should be pDropTarget, not the wrapper.
 The wrapper is only used for the marshalling.

That window prop could probably go away, I only put it there because
that's what Windows does, I doubt there's an app that relies on it
though.  However if it stays you do need the AddRef!

Huw.




Re: ole32/ole2: Don't call IDropTarget::QueryInterface() in RegisterDragDrop(). [whitespace fixed]

2011-05-26 Thread Huw Davies
On Thu, May 26, 2011 at 06:51:13AM -0500, Adam Martinson wrote:
 On 05/26/2011 06:47 AM, Huw Davies wrote:
 On Wed, May 25, 2011 at 12:45:15PM -0500, Adam Martinson wrote:
 On 05/25/2011 12:36 PM, Adam Martinson wrote:
 The AddRef is done in WrapDropTarget(), seems like the appropriate
 place for it.  Quite right on the rest though, thanks.
 Oops no, the window prop should be pDropTarget, not the wrapper.
 The wrapper is only used for the marshalling.
 That window prop could probably go away, I only put it there because
 that's what Windows does, I doubt there's an app that relies on it
 though.  However if it stays you do need the AddRef!
 
 Huw.
 Best to mirror Windows as close as possible.  The AddRef() is done
 in WrapDropTarget().  On failure IDropTarget::Release() is done on
 the wrapper, which removes the ref on the original.

You're storing a 2nd instance of the ptr (the first being in the
wrapper), conventionally you take a 2nd ref.  When you remove the
window prop you release and when you destroy the wrapper you release
again.

Huw.




Re: ole32/ole2: Don't call IDropTarget::QueryInterface() in RegisterDragDrop(). [whitespace fixed]

2011-05-24 Thread Huw Davies
 +static IDropTarget* WrapDropTarget(IDropTarget* inner)
 +{
 +DropTargetWrapper* This = HeapAlloc(GetProcessHeap(), 0, sizeof(*This));
 +
 +if (This)
 +{
 +This-iface = (IDropTarget){ DropTargetWrapper_VTbl };

This-iface.lpVtbl = DropTargetWrapper_VTbl;

 -  hr = CoMarshalInterface(stream, IID_IDropTarget, unk, MSHCTX_LOCAL, NULL, 
 MSHLFLAGS_TABLESTRONG);
 -  IUnknown_Release(unk);
 +  hr = CoMarshalInterface(stream, IID_IDropTarget, (IUnknown*)wrapper, 
 MSHCTX_LOCAL, NULL, MSHLFLAGS_TABLESTRONG);
  
if(SUCCEEDED(hr))
{
  hr = create_map_from_stream(stream, map);
  if(SUCCEEDED(hr))
  {
 -  IDropTarget_AddRef(pDropTarget);
SetPropW(hwnd, prop_oledroptarget, pDropTarget);

You probably want to set the window prop to the wrapper here.  Either
way you still need the AddRef (on the wrapper if you set that), it
gets released in RevokeDragDrop.

Huw.




Re: gdi32/dibdrv: The dib debug channel is unused so remove it.

2011-04-11 Thread Huw Davies
On Sun, Apr 10, 2011 at 06:45:51PM +0200, Francois Gouget wrote:
 It might get used soon but then it's easy to add back anyway.

Very soon in fact - it gets used in the PatBlt patch I just sent ;-)




Re: gdi32/tests: Start of a framework for writing dib driver tests.

2011-04-05 Thread Huw Davies
On Tue, Apr 05, 2011 at 03:31:23PM +0100, Huw Davies wrote:
 
 Try2.  Using the A_SHA functions to generate the hashes.

Although somewhat neater than the first version it doesn't work on
nt4/win2k as they don't export these functions.  So we're better off
using the original.

Huw.




Re: [PATCH 2/5] shdocvw: Added IShellBrowser interface stub

2011-03-14 Thread Huw Davies
On Mon, Mar 14, 2011 at 05:03:37PM +0100, Piotr Caban wrote:
 +static HRESULT WINAPI ShellBrowser_QueryInterface(
 +IShellBrowser* iface,
 +REFIID riid,
 +void **ppvObject)
 +{
 +ShellBrowser *This = impl_from_IShellBrowser(iface);
 +*ppvObject = NULL;
 +
 +if(IsEqualGUID(IID_IShellBrowser, riid))
 +*ppvObject = This-IShellBrowser_iface;
 +
 +if(*ppvObject) {
 +TRACE(%p %s %p\n, This, debugstr_guid(riid), ppvObject);
 +IUnknown_AddRef((IUnknown*)*ppvObject);
 +return S_OK;
 +}
 +

You probably want to add support for IID_IUnknown here too.

Huw.




Re: oleaut32: Implement ITypeInfo_GetNames Stub/Proxy (resend)

2011-02-22 Thread Huw Davies
On Tue, Feb 22, 2011 at 09:02:38PM +1100, Alistair Leslie-Hughes wrote:
 From 6aa7bfa0c06b746ee64daf56f2b02e6f38542d54 Mon Sep 17 00:00:00 2001
 From: Alistair Leslie-Hughes leslie_alist...@hotmail.com
 Date: Fri, 18 Feb 2011 12:04:56 +1100
 Subject: [PATCH] Implement ITypeInfo_GetNames Stub/Proxy
 To: wine-patches wine-patc...@winehq.org
 
 ---
  dlls/oleaut32/usrmarshal.c |   10 ++
  1 files changed, 6 insertions(+), 4 deletions(-)

This looks good to me.  I'm not sure why this function is marked as
[call_as], I expect they originally had planned to allow for a NULL
pcNames if cMaxNames == 1 (rather like IEnum*_Next).  However a quick
test shows that this doesn't appear to be the case.

Huw.




Re: widl: Added support for registering alternative progids.

2010-12-17 Thread Huw Davies
On Fri, Dec 17, 2010 at 04:14:24PM +0100, Jacek Caban wrote:
 diff --git a/tools/widl/parser.y b/tools/widl/parser.y
 index c924a66..28cb1da 100644
 --- a/tools/widl/parser.y
 +++ b/tools/widl/parser.y
 @@ -190,7 +190,7 @@ static statement_list_t 
 *append_statement(statement_list_t *list, statement_t *s

 -%token tAGGREGATABLE tALLOCATE tANNOTATION tAPPOBJECT tASYNC tASYNCUUID
 +%token tAGGREGATABLE tALLOCATE tANNOTATION tAPPOBJECT tASYNC tASYNCUUID 
 tALTPROGID


Hi Jacek,

This breaks alphabetical order in this line.

Cheers,
Huw.




Re: [PATCH] ole32: check for interface NULL which happens with e.g. Abiword

2010-09-15 Thread Huw Davies
On Wed, Sep 15, 2010 at 08:04:51PM +0900, Dmitry Timoshkov wrote:
 Marcus Meissner mar...@jet.franken.de wrote:
 
  +  if (!unk) {
  +  FIXME(hr was %d, but unk is NULL?\n, hr);
  +  return E_FAIL;
  +  }
 
 IDropTarget_QueryInterface() should be fixed instead.

The implementation is in abiword, so that would be tricky.

Marcus, you're leaking a stream in this error path.  Better
to do something like:
   if(FAILED(hr) || !unk)

Huw.




Re: [PATCH] ole32: check for interface NULL which happens with e.g. Abiword

2010-09-15 Thread Huw Davies
On Wed, Sep 15, 2010 at 01:34:06PM +0200, Marcus Meissner wrote:
 On Wed, Sep 15, 2010 at 12:22:57PM +0100, Huw Davies wrote:
  On Wed, Sep 15, 2010 at 08:04:51PM +0900, Dmitry Timoshkov wrote:
   Marcus Meissner mar...@jet.franken.de wrote:
   
+  if (!unk) {
+  FIXME(hr was %d, but unk is NULL?\n, hr);
+  return E_FAIL;
+  }
   
   IDropTarget_QueryInterface() should be fixed instead.
  
  The implementation is in abiword, so that would be tricky.
  
  Marcus, you're leaking a stream in this error path.  Better
  to do something like:
 if(FAILED(hr) || !unk)
 
 yeah, but what error to return :/

Yeah, sorry.  Something like:
if(!unk) hr = E_NOINTERFACE;
right after the QI call.

Huw.




Re: Use SUBLANG_NEUTRAL for the Gaelic resources?

2010-08-24 Thread Huw Davies
On Tue, Aug 24, 2010 at 09:59:51AM +0200, Francois Gouget wrote:
 
 Are SUBLANG_GAELIC, SUBLANG_GAELIC_SCOTTISH and SUBLANG_GAELIC_MANX very 
 different languages?
 
 If not it would be better to use SUBLANG_NEUTRAL instead of 
 SUBLANG_GAELIC. For instance:

I think they're different enough to warrant falling back to English
rather than Irish (which is essentially what you're asking).

See:
http://en.wikipedia.org/wiki/Manx_language#Background

Huw.




Re: ole32: Implement cross-process drag and drop.

2010-07-20 Thread Huw Davies
On Mon, Jul 19, 2010 at 04:42:40PM +0100, Huw Davies wrote:
 Fix a typo in a comment spotted by Detlef.
 ---
  dlls/ole32/ole2.c |  199 
  1 files changed, 168 insertions(+), 31 deletions(-)

Sorry, this is still broken, please ignore.

Huw.




Re: ole32: Implement cross-process drag and drop.

2010-07-19 Thread Huw Davies
On Mon, Jul 19, 2010 at 01:58:07PM +0100, Huw Davies wrote:
 ---
  dlls/ole32/ole2.c |  199 
  1 files changed, 168 insertions(+), 31 deletions(-)

Please use the updated version which has fixed a typo in a comment
(spotted by Detlef).




Re: sane.ds: Change ns to ms in resource files

2010-07-07 Thread Huw Davies
On Wed, Jul 07, 2010 at 11:28:21AM +0100, Ken Sharp wrote:
 On 7/7/2010 10:30 AM, Alexandre Julliard wrote:
 Ken Sharpkennyb...@o2.co.uk  writes:

 Apparently the English resource file should show ms (microseconds)
 instead of ns.  This error has been copied too all the .rc files.

 ms doesn't mean microseconds.


 What does it mean?  I couldn't find a definition.

http://en.wikipedia.org/wiki/Millisecond

For microsecond you want 'µs' or failing that, 'us'.




Re: sane.ds: Change ns to ms in resource files

2010-07-07 Thread Huw Davies
On Wed, Jul 07, 2010 at 11:45:59AM +0100, Ken Sharp wrote:
 On 7/7/2010 11:39 AM, Huw Davies wrote:
 On Wed, Jul 07, 2010 at 11:28:21AM +0100, Ken Sharp wrote:
 On 7/7/2010 10:30 AM, Alexandre Julliard wrote:
 Ken Sharpkennyb...@o2.co.uk   writes:

 Apparently the English resource file should show ms (microseconds)
 instead of ns.  This error has been copied too all the .rc files.

 ms doesn't mean microseconds.


 What does it mean?  I couldn't find a definition.

 http://en.wikipedia.org/wiki/Millisecond

 For microsecond you want 'µs' or failing that, 'us'.



 Well, yes, but that's what I was told.  If it should be ms then the  
 patch is ok.  I don't understand what seconds are doing in a scanning  
 DLL anyway, I don't ever recall it being used.

Huh?  I told you yesterday that the string should be micro seconds,
that's 'µs' and not 'ms'.




Re: sane.ds: Add Welsh resource

2010-07-06 Thread Huw Davies
On Mon, Jul 05, 2010 at 06:51:36PM +0100, Ken Sharp wrote:
 +{
 + 0 
 + 1 px
 + 2 b  /* What is b ? */
 + 3 mm
 + 4 dpi /* dotiau fesul modfedd */
 + 5 %
 + 6 ns /* What is ns ? */
 +}

See the SANE_UNIT_ defines in /usr/include/sane/sane.h

'b' is bits and 'ns' should be microseconds, so it looks like the
English resource is incorrect too.

Huw.
PS Happy to see the Welsh translations coming in!




Re: IDL trouble (patch: adding IEnumShellItems interface declaration.)

2010-06-11 Thread Huw Davies

On 11 Jun 2010, at 02:42, David Hedberg david.hedb...@gmail.com wrote:


Hello everyone,

I'm having some trouble with this simple patch. Specifically, it  
breaks compiling of dlls/actxprxy, producing the message:


*

/home/david/Projects/wine/git/dlls/actxprxy/actxprxy_shobjidl_p.c: 
6507: undefined reference to `IEnumShellItems_Next_Stub'
actxprxy_shobjidl_p.o:(.data.rel.ro+0xf0): undefined reference to  
`IEnumShellItems_Next_Proxy'

*

It compiles if I remove the [call_as] attribute, but I guess that  
would kind of break the interface. Any obvious mistakes in the  
patch, or grammar not supported by widl?


You need to add an implementation of IEnumShellItems_Next_Proxy/Stub  
to usrmarshal.c


Huw.




Re: [PATCH 2/5] include: Add IEnumShellItems interface declaration.

2010-06-11 Thread Huw Davies

On 11 Jun 2010, at 12:17, David Hedberg david.hedb...@gmail.com wrote:


---
dlls/actxprxy/usrmarshal.c |   21 +
include/shobjidl.idl   |   30 ++
2 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/dlls/actxprxy/usrmarshal.c b/dlls/actxprxy/usrmarshal.c
index f2aacf8..85e534c 100644
--- a/dlls/actxprxy/usrmarshal.c
+++ b/dlls/actxprxy/usrmarshal.c
+
+HRESULT CALLBACK IEnumShellItems_Next_Proxy(
+IEnumShellItems *This,
+ULONG celt,
+IShellItem **rgelt,
+ULONG *pceltFetched)
+{
+TRACE((%p)\n, This);
+return IEnumShellItems_RemoteNext_Proxy( This, celt, rgelt,  
pceltFetched );

+}


pceltFetched could be a NULL ptr here, so you should pass the address  
of a local variable in that case. See ole32/usrmarshal.c for examples  
on how to marshal IEnum*_Next


Huw.





Re: rpcrt4: Add tests for multi-dimensional conformant arrays.

2010-02-12 Thread Huw Davies
On Fri, Feb 12, 2010 at 11:53:30AM +0100, Paul Vriens wrote:
 On 02/11/2010 05:18 PM, Huw Davies wrote:
 +ptr = NdrSimpleStructMarshall(StubMsg, (unsigned char *)memsrc,
 +fmtstr_complex_array[32] );

 Hi Huw,

 This particular test crashes on Win9x, WinMe and NT4.

 They all show:

 ndr_marshall.c:1973: Test failed: length 96

 just before the crash so maybe that would be our means to bail out?

Hi Paul,

Yes, looks like it's broken on those platforms; the
NdrSimpleStructMarshall call is crashing.  Are you happy to send in a
patch?

Huw.




Re: rpcrt4: Add tests for multi-dimensional conformant arrays.

2010-02-12 Thread Huw Davies
On Fri, Feb 12, 2010 at 11:27:38AM +, Huw Davies wrote:
 On Fri, Feb 12, 2010 at 11:53:30AM +0100, Paul Vriens wrote:
  On 02/11/2010 05:18 PM, Huw Davies wrote:
  +ptr = NdrSimpleStructMarshall(StubMsg, (unsigned char *)memsrc,
  +fmtstr_complex_array[32] );
 
  Hi Huw,
 
  This particular test crashes on Win9x, WinMe and NT4.
 
  They all show:
 
  ndr_marshall.c:1973: Test failed: length 96
 
  just before the crash so maybe that would be our means to bail out?
 
 Hi Paul,
 
 Yes, looks like it's broken on those platforms; the
 NdrSimpleStructMarshall call is crashing.  Are you happy to send in a
 patch?
 

Btw, I have a fix for the 64 bit failures.  It'll conflict with your
patch, so I'll send it after yours is committed.

Huw.




Re: shell32: Dynamically load an ordinal only export.

2010-01-22 Thread Huw Davies
On Fri, Jan 22, 2010 at 02:01:03PM +0100, Alexandre Julliard wrote:
 Huw Davies h...@codeweavers.com writes:
 
  ---
   dlls/shell32/tests/shlfileop.c |   18 +-
   1 files changed, 9 insertions(+), 9 deletions(-)
 
 This shouldn't be needed, import by ordinal works just fine.

I get a failure with make crosstest:

../../../tools/winegcc/winegcc -b i586-mingw32msvc -B../../../tools/winebuild 
--sysroot=../../.. appbar.cross.o autocomplete.cross.o generated.cross.o 
progman_dde.cross.o shelllink.cross.o shellpath.cross.o shfldr_special.cross.o 
shlexec.cross.o shlfileop.cross.o shlfolder.cross.o string.cross.o 
systray.cross.o rsrc.res testlist.cross.o -o shell32_crosstest.exe -lshell32 
-lole32 -loleaut32 -luser32 -ladvapi32 -lkernel32   
shlfileop.cross.o: In function `test_shlmenu':
/home/daviesh/wine/dlls/shell32/tests/shlfileop.c:2232: undefined reference to 
`_shell_mergeme...@24'
/home/daviesh/wine/dlls/shell32/tests/shlfileop.c:2234: undefined reference to 
`_shell_mergeme...@24'

Huw.




Re: shell32: Dynamically load an ordinal only export.

2010-01-22 Thread Huw Davies
On Fri, Jan 22, 2010 at 02:51:04PM +0100, Alexandre Julliard wrote:
 Huw Davies h...@codeweavers.com writes:
 
  On Fri, Jan 22, 2010 at 02:01:03PM +0100, Alexandre Julliard wrote:
  Huw Davies h...@codeweavers.com writes:
  
   ---
dlls/shell32/tests/shlfileop.c |   18 +-
1 files changed, 9 insertions(+), 9 deletions(-)
  
  This shouldn't be needed, import by ordinal works just fine.
 
  I get a failure with make crosstest:
 
 You have to run make crosstest from the top level for the import
 libraries to be built.

Ah! Thanks!

Huw.




Re: Patch feedback requested for OleCreatePropertyFrame()

2010-01-04 Thread Huw Davies
On Sun, Jan 03, 2010 at 03:00:12PM -0800, Geoffrey Hausheer wrote:
 I found a patch from 2001 written by TAKESHIMA Hidenori that
 was posted to wine-patches
 (http://www.winehq.org/pipermail/wine-patches/2001-October/001229.html)
 but never committed.

Actually it was commited (2843934af5515c7f2b8370324aa98d3964a40324)
but was then removed (140e7222e0d7ce76068cddc64c68105c2e569257) at
his request.

Huw.




Re: gdiplus: Fix a memory leak in the tests.

2009-12-15 Thread Huw Davies
On Tue, Dec 15, 2009 at 02:40:26PM +0300, Nikolay Sivov wrote:
 On 12/15/2009 14:35, Huw Davies wrote:
 Found by Valgrind.
 ---
   dlls/gdiplus/tests/customlinecap.c |1 +
   1 files changed, 1 insertions(+), 0 deletions(-)
 Hi, Huw.

 This leak will be fixed with corrected  GdipCreateCustomLineCap().  
 Return code should never
 be Ok here.

 P.S. Same with previous patch for  GdipCreateFontFamilyFromNam().

Yes I know.  And when it's fixed the GdipDelete* calls could be
removed along with the todo_wine.  At the moment it's easier to
get them out of the Valgrind logs so that the real leaks are more
obvious.

Huw.




Re: ole32: Fix invalid memory access in storage32

2009-12-06 Thread Huw Davies

Nathan Gallaher wrote:


In StorageImpl_construct():
For pwcsName strings shorter than DIRENTRY_NAME_BUFFER_LEN-1, an invalid
read would be noted by valgrind as the memcpy wanders off the end of the
string.  Do the needful to calculate the required string length.


By the way, in storage32.h the filename member of struct StorageBaseImpl is 
declared as
WCHAR filename[DIRENTRY_NAME_BUFFER_LEN];
shouldn't it be
WCHAR filename[DIRENTRY_NAME_MAX_LEN];
?

Huw.




Re: [PATCH 1/5] msdaps: Use 2 byte structure packing.

2009-08-24 Thread Huw Davies
On Mon, Aug 24, 2009 at 01:51:24PM +0200, Alexandre Julliard wrote:
 Huw Davies h...@codeweavers.com writes:
 
  diff --git a/dlls/msdaps/Makefile.in b/dlls/msdaps/Makefile.in
  index d6f71a0..cebf659 100644
  --- a/dlls/msdaps/Makefile.in
  +++ b/dlls/msdaps/Makefile.in
  @@ -5,6 +5,7 @@ VPATH = @srcdir@
   MODULE= msdaps.dll
   IMPORTS   = oleaut32 rpcrt4 kernel32 ntdll
   EXTRADEFS = -DREGISTER_PROXY_DLL -DPROXY_CLSID_IS={ 0x06210e88, 0x01f5, 
  0x11d1, { 0xb5, 0x12, 0x00, 0x80, 0xc7, 0x81, 0xc3, 0x84 } }
  +EXTRAIDLFLAGS = -a2
 
 Won't this break the 64-bit build?

Yes, good point.  64-bit should be left to default to 8 byte
alignment.  How do I do that?

Huw.




Re: msctf: fully clear the Variant before setting it.

2009-07-01 Thread Huw Davies
On Wed, Jul 01, 2009 at 10:29:22AM -0500, Aric Stewart wrote:
 diff --git a/dlls/msctf/compartmentmgr.c b/dlls/msctf/compartmentmgr.c
 index 3afcfda..e1c7187 100644
 --- a/dlls/msctf/compartmentmgr.c
 +++ b/dlls/msctf/compartmentmgr.c
 @@ -537,6 +537,7 @@ static HRESULT WINAPI Compartment_GetValue(ITfCompartment 
 *iface,
  if (!pvarValue)
  return E_INVALIDARG;
  
 +memset(pvarValue,0,sizeof(VARIANT));
  pvarValue-n1.n2.vt = VT_EMPTY;
  if (!This-variant.n1.n2.vt == VT_EMPTY)
  hr = VariantCopy(pvarValue,This-variant);
 


This looks wrong; there's something strange going on here.  Note also
there's VariantInit() which is a cleaner way to set vt to VT_EMPTY.

Huw.




Re: [PATCH 4/4] gdi32: Add tests for the GetWinMetaFileBits MFCOMMENT record.

2009-06-30 Thread Huw Davies
On Tue, Jun 30, 2009 at 12:40:54PM +0200, Paul Vriens wrote:
 Huw Davies wrote:
 ---
  dlls/gdi32/tests/metafile.c |   80 
 +++
  1 files changed, 80 insertions(+), 0 deletions(-)


 


 Hi Huw,

 Could you use CreateEnhMetaFileA here:

 +SetRect(rc, 1000,2000, 3000, 6000);
 +emf_dc = CreateEnhMetaFileW(display_dc, NULL, rc, NULL);

 The W-version is not implemented on Win9x so this test would crash.

Hi Paul,

Yup, resent. Thanks!

Huw.




Re: oleaut32/tests: Add some tests for [lcid] parameters.

2009-06-17 Thread Huw Davies
On Wed, Jun 17, 2009 at 03:58:33PM +0200, Paul Vriens wrote:
 Huw Davies wrote:
 ---
  dlls/oleaut32/tests/tmarshal.c |   53 
 +++-
  dlls/oleaut32/tests/tmarshal.idl   |6 +++
  dlls/oleaut32/tests/tmarshal_dispids.h |1 +
  3 files changed, 59 insertions(+), 1 deletions(-)


 


 In an effort to get a green NT4, I ran your new tests on NT4:

 tmarshal.c:1376: Test failed: ITypeInfo_Invoke failed with error 0x80010105
 tmarshal.c:1377: Test failed: got 0
 tmarshal.c:1378: Test failed: got 0

 On W2K3:

 tmarshal.c:1376: Test failed: ITypeInfo_Invoke failed with error 0x80010105
 tmarshal.c:1377: Test failed: got 0
 tmarshal.c:1378: Test failed: got 0

 and a maybe unrelated on W2K3 (wasn't here before):
 tmarshal.c:605: Test failed: Struct parameter passed by pointer corrupted

 On W98:

 tmarshal.c:1376: Test failed: ITypeInfo_Invoke failed with error 0x80010105
 tmarshal.c:1377: Test failed: got 0
 tmarshal.c:1378: Test failed: got 0

 Am I doing something wrong here?

Hi Paul,

Did you apply the widl patch too?

Huw.




Re: ntdll/tests: Mark return value of RtlUnicodeStringToInteger(, 16) as broken for nt4.

2009-05-19 Thread Huw Davies
On Tue, May 19, 2009 at 03:39:07PM +0200, Alexandre Julliard wrote:
 Paul Vriens paul.vriens.w...@gmail.com writes:
 
  I don't think this is correct. This test has always succeeded until
  March 9th. There haven't been any changes to the test in that period,
  but I remember an upgrade of winehq in that weekend.
 
  In other words, there is more to it. Testing a simple cross compiled
  version of this test doesn't yield an error on NT4.
 
 My feeling is that it's returning uninitialized data, so the results
 depend on the stack layout that the compiler is using. There's probably
 no sense in checking for a specific value, just consider this case
 broken no matter what it returns.

Ok, I'll resend without the test on the returned value.

Huw.




Re: [2/2] winex11.drv: export copied images as image/bmp

2009-05-01 Thread Huw Davies
On Fri, May 01, 2009 at 01:25:33PM +0200, Alexandre Julliard wrote:
 Vincent Povirk vinc...@codeweavers.com writes:
 
  +if (dibinfo-biBitCount  16)
  +numcolors = dibinfo-biClrUsed || 1dibinfo-biBitCount;
 
 You have been doing too much Perl ;-)

Hi Vincent,

You may find bitmap_info_size() useful here.

Huw.




Re: [PATCH 1/2] ole32: Register the clipboard formats at load time.

2009-04-23 Thread Huw Davies
On Thu, Apr 23, 2009 at 03:40:11PM +0200, Alexandre Julliard wrote:
 Huw Davies h...@codeweavers.com writes:
 
  ---
   dlls/ole32/clipboard.c   |   54 
  +
   dlls/ole32/compobj.c |2 +
   dlls/ole32/compobj_private.h |   14 +++
   3 files changed, 54 insertions(+), 16 deletions(-)
 
 It would be nicer to do this when needed, so that we don't have to
 initialize the graphics driver just because ole32 was imported.

Good point, actually having them registered in OleInitialise is
fine.  Resent.

Thanks,
Huw.




Re: [PATCH 5/5] user32/tests: Fix tests on win9x.

2009-04-21 Thread Huw Davies
On Tue, Apr 21, 2009 at 11:34:04AM +0200, Paul Vriens wrote:
 Huw Davies wrote:
 On Tue, Apr 21, 2009 at 11:22:29AM +0200, Paul Vriens wrote:
 Huw Davies wrote:
 ---
  dlls/user32/tests/clipboard.c |7 +--
  1 files changed, 5 insertions(+), 2 deletions(-)


 


 Was just about to sent a patch along those lines.

 Isn't it better to mark that win9x (and winME as well btw) one as broken()?

 I was debating with myself whether to mark it as broken or not.  My
 feeling was that it's not actually broken behaviour rather missing
 functionality.  But I don't really care.

 Huw.

 Maybe it's not broken() behavior but marking it as broken() makes sure  
 we (Wine) will have CF_UNICODETEXT. Without that broken(),  
 CF_UNICODETEXT can go missing without us knowing.

Yeah, fair enough.  I guess the point is it's broken behaviour for
Wine.  I'll resend, thanks.

Huw.




Re: [PATCH 5/5] user32/tests: Fix tests on win9x.

2009-04-21 Thread Huw Davies
On Tue, Apr 21, 2009 at 11:22:29AM +0200, Paul Vriens wrote:
 Huw Davies wrote:
 ---
  dlls/user32/tests/clipboard.c |7 +--
  1 files changed, 5 insertions(+), 2 deletions(-)


 


 Was just about to sent a patch along those lines.

 Isn't it better to mark that win9x (and winME as well btw) one as broken()?

I was debating with myself whether to mark it as broken or not.  My
feeling was that it's not actually broken behaviour rather missing
functionality.  But I don't really care.

Huw.




Re: Problem with compiling ole32 crosstest

2009-02-13 Thread Huw Davies
On Fri, Feb 13, 2009 at 02:01:35PM +0300, Vitaly Perov wrote:
 Hi!
 
 Does anybody have a problem compilling the crosstest of ole32?
 Or maybe it's only my fault?
 
 I have:
 usrmarshal.cross.o: In function `func_usrmarshal':
 /srv/vitperov/Projects/wine-dev/dlls/ole32/tests/usrmarshal.c:445: undefined 
 reference to `_wdtpinterfacepointer_users...@20'
 /srv/vitperov/Projects/wine-dev/dlls/ole32/tests/usrmarshal.c:448: undefined 
 reference to `_wdtpinterfacepointer_usermars...@20'
 ...

Does this help?

cd dlls/ole32
make libole32.a

-- 
h...@codeweavers.com




Re: gdi32/tests: Use the ANSI text metrics so the tests work on win9x.

2009-02-12 Thread Huw Davies
On Thu, Feb 12, 2009 at 01:40:54PM +0100, Paul Vriens wrote:
 Huw Davies wrote:
 ---
  dlls/gdi32/tests/font.c |   20 ++--
  1 files changed, 10 insertions(+), 10 deletions(-)


 


 Looks a lot like:

 http://www.winehq.org/pipermail/wine-patches/2009-February/069297.html

 Maybe we should use yours as it has less changes to the code.

Doh, I just saw that patch after I sent mine!

Huw.




Re: Huw Davies : opengl/tests: Skip tests if we can't find a pixel format.

2008-12-09 Thread Huw Davies
On Mon, Dec 08, 2008 at 01:44:41PM +0100, Alexandre Julliard wrote:
 Huw Davies [EMAIL PROTECTED] writes:
 
  Hi Alexandre,
 
  This wasn't what I intended my patch to do.  The problem is that
  Wine's ChoosePixelFormat() rets 0 on XServers without glX.  The
  win_skip results in a test failure.  My patch was intended to mark
  this as a todo_wine.
 
 We could have a todo_wine around the win_skip, though I'm not quite
 convinced that having the opengl32 test succeed without opengl support
 is the right thing to do.

Ok, I'll send in a patch with todo_wine and let you decide.

My feeling was that without opengl support we should skip the opengl32
tests, hence the skip in the original patch.  The todo_wine was to
flag Wine's ChoosePixelFormat bug.

Huw.
-- 
[EMAIL PROTECTED]




Re: Huw Davies : opengl/tests: Skip tests if we can't find a pixel format.

2008-12-08 Thread Huw Davies
On Fri, Dec 05, 2008 at 11:04:07AM -0600, Alexandre Julliard wrote:
 Module: wine
 Branch: master
 Commit: e86ff2a3128f4a0157ecfa10fee31d1416312c71
 URL:
 http://source.winehq.org/git/wine.git/?a=commit;h=e86ff2a3128f4a0157ecfa10fee31d1416312c71
 
 Author: Huw Davies [EMAIL PROTECTED]
 Date:   Fri Dec  5 14:19:25 2008 +
 
 opengl/tests: Skip tests if we can't find a pixel format.
 
 ---
 
  dlls/opengl32/tests/opengl.c |9 -
  1 files changed, 8 insertions(+), 1 deletions(-)
 
 diff --git a/dlls/opengl32/tests/opengl.c b/dlls/opengl32/tests/opengl.c
 index 35eb8ae..a3ac3af 100644
 --- a/dlls/opengl32/tests/opengl.c
 +++ b/dlls/opengl32/tests/opengl.c
 @@ -555,7 +555,12 @@ START_TEST(opengl)
  hdc = GetDC(hwnd);
  
  iPixelFormat = ChoosePixelFormat(hdc, pfd);
 -ok(iPixelFormat  0, No pixelformat found!\n); /* This should 
 never happen as ChoosePixelFormat always returns a closest match */
 +if(iPixelFormat == 0)
 +{
 +/* This should never happen as ChoosePixelFormat always returns 
 a closest match, but currently this fails in Wine if we don't have glX */
 +win_skip(Unable to find pixel format.\n);
 +goto cleanup;
 +}
  
  /* We shouldn't be able to create a context from a hdc which doesn't 
 have a pixel format set */
  hglrc = wglCreateContext(hdc);


Hi Alexandre,

This wasn't what I intended my patch to do.  The problem is that
Wine's ChoosePixelFormat() rets 0 on XServers without glX.  The
win_skip results in a test failure.  My patch was intended to mark
this as a todo_wine.

Huw.
-- 
[EMAIL PROTECTED]




Re: opengl/tests: Skip tests if we can't find a pixel format.

2008-12-05 Thread Huw Davies
On Fri, Dec 05, 2008 at 03:25:56PM +0100, Paul Vriens wrote:
 This looks a bit strange:
 
 +if(iPixelFormat == 0)
 +{
 +todo_wine ok(iPixelFormat  0,
 
 
 Is it your intention to just throw up a failed message in all cases?

Hi Paul,

It was my intention to throw a todo if ChoosePixelFormat rets 0 when
run under Wine.

Huw.




Re: [RESEND] Send page size to cups with lpr command on printing (bug 15942)

2008-11-12 Thread Huw Davies
Massimo Del Fedele wrote:
 Steven Edwards ha scritto:
 On Wed, Nov 12, 2008 at 12:51 PM, Massimo Del Fedele [EMAIL PROTECTED] 
 wrote:
 The best way would be, of course, replace ALL LPR code with direct cups
 calls. It's feasible, but it requires some refactoring of all printing
 code. For example, what's now
  exec(|lpr -P myPrinter)
 which I replaced with
  exec(|lpr -P myPrinter -o myPaperSize)
 should become
CupsCreateJob()
CupsStartDocument()
CupsWriteRequestData()
CupsFinishDocument()

 That would require some amount of work and, over all, I'm not sure it
 could be accepted to go into wine code.
 You should check if cups is present at compile time (which I guess we
 already do) and then dlopen the cups libraries if present (again I
 assume we already do this) and yes use the cups api if possible rather
 than directly invoking lpr.

 
 So, replace ALL existing execs to lpr with direct cups calls ? Mixing 
 them for a single print job is not possible, AFAIK.

The correct way is to fix winspool and friends to use schedule_cups() 
which in turn uses cupsPrintFile().   Detlef could probably comment on 
how far away we are from using this.

Huw.







Re: inetcomm: Add an implementation of IVirtualStream.

2008-11-07 Thread Huw Davies
On Fri, Nov 07, 2008 at 02:20:04PM +0100, Hans Leidekker wrote:
 On Friday 07 November 2008 13:48:29 Huw Davies wrote:
   This interface is undocumented but fortunately its name was enough
   of a hint to create a sensible implementation. Used by Outlook when
   retrieving or sending a mail.
  
  We already have MimeOleCreateVirtualStream() which is implemented on top 
  of CreateStreamOnHGlobal()...
 
 Hmm, that may work too. I wonder if they are fully compatible though,
 the Read method is called with cb set to -1, which appears to mean that
 the stream size should be taken. Do you know off hand if that's supported
 by IStream?

Well cb is ULONG, so it's not -1 but a big number.  This should work
fine.

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: inetcomm: Add an implementation of IVirtualStream.

2008-11-07 Thread Huw Davies
Hans Leidekker wrote:
 This interface is undocumented but fortunately its name was enough
 of a hint to create a sensible implementation. Used by Outlook when
 retrieving or sending a mail.

We already have MimeOleCreateVirtualStream() which is implemented on top 
of CreateStreamOnHGlobal()...

Huw.





Re: gdi32: Add EMR_GDICOMMENT to the list of records producing output.

2008-10-24 Thread Huw Davies
On Thu, Oct 23, 2008 at 09:59:44PM +0900, Dmitry Timoshkov wrote:
 Also, do you have an idea why some EMF
 records are not in that list (EMR_FILLPATH, EMR_STROKEANDFILLPATH,
 EMR_STROKEPATH and probably some others)?

No idea, they probably just got left out by mistake.  Clearly we need
more exhaustive tests here.

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: ole32: implement IEnumFORMATETC_Next_Proxy (1/2) try 2

2008-10-24 Thread Huw Davies
On Fri, Oct 24, 2008 at 04:45:46AM -0500, Austin English wrote:
 diff --git a/dlls/ole32/usrmarshal.c b/dlls/ole32/usrmarshal.c
 index b8487c3..dba69ca 100644
 --- a/dlls/ole32/usrmarshal.c
 +++ b/dlls/ole32/usrmarshal.c
 @@ -2398,8 +2398,11 @@ HRESULT CALLBACK IEnumFORMATETC_Next_Proxy(
  FORMATETC *rgelt,
  ULONG *pceltFetched)
  {
 -FIXME(:stub\n);
 -return E_NOTIMPL;
 +ULONG fetched;
 +if (!pceltFetched)
 +pceltFetched = fetched;
 +return IEnumFORMATETC_RemoteNext_Proxy(This,
 +celt, rgelt, pceltFetched);

That's still not right.  It gives the impression that the 'return' is
part of the 'if' block.  The 'return' line should be indented by four
spaces not eight.

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: ole32: implement IEnumFORMATETC_Next_Proxy (1/2)

2008-10-24 Thread Huw Davies
On Thu, Oct 23, 2008 at 01:35:55PM -0500, Austin English wrote:
 diff --git a/dlls/ole32/usrmarshal.c b/dlls/ole32/usrmarshal.c
 index b8487c3..be9ebff 100644
 --- a/dlls/ole32/usrmarshal.c
 +++ b/dlls/ole32/usrmarshal.c
 @@ -2398,8 +2398,13 @@ HRESULT CALLBACK IEnumFORMATETC_Next_Proxy(
  FORMATETC *rgelt,
  ULONG *pceltFetched)
  {
 -FIXME(:stub\n);
 -return E_NOTIMPL;
 +  ULONG fetched;
 +  if (!pceltFetched)
 +pceltFetched = fetched;
 +return IEnumFORMATETC_RemoteNext_Proxy(This,
 +   celt,
 +   rgelt,
 +   pceltFetched);

There's some strange indentation going on here.  Could you make it all
4-space based?

Thanks,
Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: ole32: quiet a ridiculously noisy fixme (resend)

2008-10-23 Thread Huw Davies
On Wed, Oct 22, 2008 at 08:34:49PM -0500, Austin English wrote:
 Reduces Open Office 3 application's output from infinity to 7 lines.
 Last patch had an unnecessary whitespace change.
 
 -- 
 -Austin

 From 0dead16d2588bce8330ed862f8e5b6d3c6bb71a7 Mon Sep 17 00:00:00 2001
 From: Austin English [EMAIL PROTECTED]
 Date: Wed, 22 Oct 2008 20:28:37 -0500
 Subject: [PATCH] ole32: quiet a ridiculously noisy fixme
 
 ---
  dlls/ole32/usrmarshal.c |6 +-
  1 files changed, 5 insertions(+), 1 deletions(-)
 
 diff --git a/dlls/ole32/usrmarshal.c b/dlls/ole32/usrmarshal.c
 index b8487c3..6886dfb 100644
 --- a/dlls/ole32/usrmarshal.c
 +++ b/dlls/ole32/usrmarshal.c
 @@ -2398,7 +2398,11 @@ HRESULT CALLBACK IEnumFORMATETC_Next_Proxy(
  FORMATETC *rgelt,
  ULONG *pceltFetched)
  {
 -FIXME(:stub\n);
 +static int quietfixme = 0;
 +if (quietfixme == 0) {
 +FIXME(:stub\n);
 +quietfixme = 1;
 +}
  return E_NOTIMPL;
  }
  

The real implementation of this would actually result in fewer lines
of code!

See IEnumVARIANT_Next_Proxy / Stub (in oleaut32/usrmarshal.c) for an
example.

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: gdi32: Add EMR_GDICOMMENT to the list of records producing output.

2008-10-23 Thread Huw Davies
On Thu, Oct 23, 2008 at 09:32:43PM +0900, Dmitry Timoshkov wrote:
 This completely fixes the print previews generated by MS Access in a Win9x
 mode. Access has a custom handler for an EMR_GDICOMMENT record, and GDI APIs
 it calls from it don't work properly due to wrong DC mapping.
 ---
  dlls/gdi32/enhmetafile.c |1 +
  1 files changed, 1 insertions(+), 0 deletions(-)
 
 diff --git a/dlls/gdi32/enhmetafile.c b/dlls/gdi32/enhmetafile.c
 index e998c60..22451d2 100644
 --- a/dlls/gdi32/enhmetafile.c
 +++ b/dlls/gdi32/enhmetafile.c
 @@ -692,6 +692,7 @@ static BOOL emr_produces_output(int type)
  case EMR_LINETO:
  case EMR_ARCTO:
  case EMR_POLYDRAW:
 +case EMR_GDICOMMENT:
  case EMR_FILLRGN:
  case EMR_FRAMERGN:
  case EMR_INVERTRGN:

Maybe it's time to rename this function to something like
emr_updates_transform() since EMR_GDICOMMENT clearly doesn't produce
any output.

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: [PATCH 3/3] ole32: Call the appropriate storage function when we run the object.

2008-10-22 Thread Huw Davies
On Wed, Oct 22, 2008 at 12:46:39PM +0100, Rob Shearman wrote:
 2008/10/21 Huw Davies [EMAIL PROTECTED]:
  @@ -1728,6 +1767,10 @@ static HRESULT WINAPI 
  DefaultHandler_IPersistStorage_HandsOffStorage(
   if(SUCCEEDED(hr)  object_is_running(This))
   hr = IPersistStorage_HandsOffStorage(This-pPSDelegate);
 
  +old_stg = InterlockedExchangePointer((void**)This-storage, NULL);
 
 IIRC, the default handler is apartment-threaded only, meaning that a
 default handler object cannot be used on a thread other than the one
 it was created on.

That's true I believe.

 Adding constructs like this introduce confusion to
 future developers about whether the code is or isn't thread-safe and
 so it should be avoided and a non-thread-safe construct used instead.

I don't buy this.  Besides we're already using
Interlocked{In,De}crement().

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: [PATCH 3/3] ole32: Call the appropriate storage function when we run the object.

2008-10-22 Thread Huw Davies
On Wed, Oct 22, 2008 at 02:03:26PM +0100, Rob Shearman wrote:
 2008/10/22 Huw Davies [EMAIL PROTECTED]:
  On Wed, Oct 22, 2008 at 12:46:39PM +0100, Rob Shearman wrote:
  2008/10/21 Huw Davies [EMAIL PROTECTED]:
   @@ -1728,6 +1767,10 @@ static HRESULT WINAPI 
   DefaultHandler_IPersistStorage_HandsOffStorage(
if(SUCCEEDED(hr)  object_is_running(This))
hr = IPersistStorage_HandsOffStorage(This-pPSDelegate);
  
   +old_stg = InterlockedExchangePointer((void**)This-storage, NULL);
 
  IIRC, the default handler is apartment-threaded only, meaning that a
  default handler object cannot be used on a thread other than the one
  it was created on.
 
  That's true I believe.
 
  Adding constructs like this introduce confusion to
  future developers about whether the code is or isn't thread-safe and
  so it should be avoided and a non-thread-safe construct used instead.
 
  I don't buy this.  Besides we're already using
  Interlocked{In,De}crement().
 
 The AddRef and Release code that uses Interlocked{In,De}crement() is
 cookie cutter code that is pretty much the same in Wine for all COM
 objects and so it doesn't look out of place. This does.

I'm still not convinced, but there are more interesting things to have
debates about than this ;-)

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: oleaut32: Add support for loading typelibs from NE files.

2008-09-04 Thread Huw Davies
On Wed, Sep 03, 2008 at 12:51:36PM +0100, Rob Shearman wrote:
 2008/9/2 Huw Davies [EMAIL PROTECTED]:
  --- a/dlls/oleaut32/typelib.c
  +++ b/dlls/oleaut32/typelib.c
  @@ -64,6 +64,7 @@
   #include winnls.h
   #include winreg.h
   #include winuser.h
  +#include wine/winbase16.h
 
   #include wine/unicode.h
   #include objbase.h
 
 I don't think we should be including a 16-bit header file and using
 16-bit functions in a 32-bit source file if we can avoid it. In this
 case, we can avoid it.

Hi Rob,

How do you suggest we avoid it?

Cheers,
Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: [Gdiplus] Implement GdipCloneImage

2008-08-18 Thread Huw Davies
On Sat, Aug 16, 2008 at 11:26:42PM -0400, Adam Petaccia wrote:
 This seems like the right thing to do, but OleLoadImage fails.
 Comments welcome.
 ---
  dlls/gdiplus/image.c   |   42 --
  dlls/gdiplus/tests/image.c |2 +-
  2 files changed, 41 insertions(+), 3 deletions(-)
 
 diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c
 index 197665d..a74003f 100644
 --- a/dlls/gdiplus/image.c
 +++ b/dlls/gdiplus/image.c
 @@ -242,11 +242,49 @@ GpStatus WINGDIPAPI GdipBitmapUnlockBits(GpBitmap* 
 bitmap,
  
  GpStatus WINGDIPAPI GdipCloneImage(GpImage *image, GpImage **cloneImage)
  {

 +hr = OleLoadPicture(stream, size, FALSE, IID_IPicture,
 +(LPVOID*)(*cloneImage)-picture);

This can't be right.  You need to pass the address of the interface
ptr.

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: wininet: default to keep-alive when the server is HTTP/1.1, not when it isn't

2008-08-14 Thread Huw Davies
Vincent Povirk wrote:
 This is a fix to a problem I introduced in
 fd5b97bc4d12adc69085b739061c56e9107f8d1f. The expression that checked
 the HTTP version was true for HTTP/1.0 servers, and I treated it as if
 it were true for HTTP/1.1 servers. That means we defaulted to keep-alive
 for HTTP/1.0 servers only, the exact opposite of what we want.

This was a problem with the old code too, but what happens when we get
HTTP  1.1 ?

Huw.





Re: DIB engine status

2008-08-05 Thread Huw Davies
On Fri, Aug 01, 2008 at 06:23:43PM +0400, Сергей Новосёлов wrote:
 We'd like to continue develop your version. When are you going
 to remain this project?

Great!  Please send me patches and I'll apply them.  I'll probably
merge with WineHQ on every WineHQ release.

I'm not planning on doing any work on it for a while, but will
probably return to it at some point in the future.

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: [Gdiplus try3 02/16] Implement GdipCreateRegion

2008-07-22 Thread Huw Davies
Hi Adam,

Some comments inlined below:

On Tue, Jul 22, 2008 at 01:22:56AM -0400, Adam Petaccia wrote:
 diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h
 index e7ca874..9c0246b 100644
 --- a/dlls/gdiplus/gdiplus_private.h
 +++ b/dlls/gdiplus/gdiplus_private.h
 @@ -196,4 +198,40 @@ struct GpFontFamily{
  WCHAR FamilyName[LF_FACESIZE];
  };
  
 +typedef struct RegionElement
 +{
 +DWORD type; /* Rectangle, Path, SpecialRectangle, or CombineMode */
 +union
 +{
 +GpRectF rect;
 +struct
 +{
 +GpPath* path;
 +struct
 +{
 +DWORD size;
 +DWORD magic;
 +DWORD pointCount;
 +DWORD storageFlags;
 +} pathHeader;
 +} pathData;
 +struct
 +{
 +struct RegionElement *left;  /* the original region */
 +struct RegionElement *right; /* what *left was combined with */
 +} combine;
 +} elementData;
 +} RegionElement;

It would be better to avoid the mixed upper/lower case names, since
this makes it look like they're win32 api structures.  Something like
typedef struct region_element region_element is nicer.
Same goes for the element names like pathHeader etc.

 +struct GpRegion{
 +struct
 +{
 +DWORD size;
 +DWORD checksum;
 +DWORD magic;
 +DWORD numOps;

A better name for numOps would be num_child_nodes.  This is the total
number of children under the root node (and is equal to twice the
number of ops).  I should update my comment at the beginning of
region.c to reflect this.

 +} header;
 +RegionElement *node;

How about actually embedding the structure here rather than a pointer to
it?  Since you'll always have a root node there's no need to allocate
it explicitly.  And let's call it 'root' not 'node'.
Something like region_element root 

 diff --git a/include/gdiplusenums.h b/include/gdiplusenums.h
 index 53401bd..f3110c9 100644
 --- a/include/gdiplusenums.h
 +++ b/include/gdiplusenums.h
 @@ -308,6 +308,15 @@ enum CombineMode
  CombineModeComplement
  };
  
 +enum RegionType
 +{
 +RegionDataRect  = 0x1000,
 +RegionDataPath  = 0x1001,
 +RegionDataEmptyRect = 0x1002,
 +RegionDataInfiniteRect  = 0x1003,
 +};
 +
 +
  enum FlushIntention
  {
  FlushIntentionFlush = 0,
 @@ -353,6 +362,7 @@ typedef enum HotkeyPrefix HotkeyPrefix;
  typedef enum PenAlignment GpPenAlignment;
  typedef enum ImageCodecFlags ImageCodecFlags;
  typedef enum CombineMode CombineMode;
 +typedef enum RegionType RegionType;
  typedef enum FlushIntention FlushIntention;
  typedef enum CoordinateSpace CoordinateSpace;
  

Are you sure RegionType is in the win32 api?  I can't find this is my
gdiplusenums.h.  If it isn't then it should be put in
gdiplus_private.h (or even just in region.c if nothing else uses it)
and renamed to enum region_type.

Again, I think you'd be better off submitting a couple of patches at a
time.

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: [Gdiplus try3 02/16] Implement GdipCreateRegion

2008-07-22 Thread Huw Davies
Adam Petaccia wrote:
 On Tue, 2008-07-22 at 11:11 +0100, Huw Davies wrote:

 +} RegionElement;
 It would be better to avoid the mixed upper/lower case names, since
 this makes it look like they're win32 api structures.  Something like
 typedef struct region_element region_element is nicer.
 Same goes for the element names like pathHeader etc.
 
 Will fix. Question: would it be better to declare these structures
 outside, or are inline declarations acceptable? 

inline is fine.


 +DWORD numOps;
 A better name for numOps would be num_child_nodes.  This is the total
 number of children under the root node (and is equal to twice the
 number of ops).  I should update my comment at the beginning of
 region.c to reflect this.
 
 Will do. How about just num_children?

Works for me.

 
 +} header;
 +RegionElement *node;
 How about actually embedding the structure here rather than a pointer to
 it?  Since you'll always have a root node there's no need to allocate
 it explicitly.  And let's call it 'root' not 'node'.
 Something like region_element root 
 
 The CombineRegion* code takes advantage over the fact that they are
 pointers and can be shuffled around, rather than
 shuffling/cloning/memcpy'ing potentially large amounts of data around.
 
 Also, because of the recursive nature of CombineMode, I found it much
 easier to deal with nodes separately (and recursively when necessary)
 than the whole Region at a time.

It should only be a question of how you start your recursion.  Obviously 
the CombineRegion stuff will have a alloc and memcpy the old root node 
to 'left', but then you don't need to alloc the new root node.

 Are you sure RegionType is in the win32 api?  I can't find this is my
 gdiplusenums.h.  If it isn't then it should be put in
 gdiplus_private.h (or even just in region.c if nothing else uses it)
 and renamed to enum region_type.
 
 I'll move it to region.c.

Great.

 Again, I think you'd be better off submitting a couple of patches at a
 time.
 
 Sorry, its kind of painful when the work is mostly done, but the problem
 is getting it pushed. I'm slightly behind where I want to be on my GSoC
 schedule, and I was trying to catch up. I'll slow down.

I think you'll actually find it quicker in the long run to have smaller 
patch dumps, that way you'll spend less time re-doing a whole load of 
patches many times.

Huw.





Re: [Gdiplus try2 02/16] Implement GdipCreateRegion

2008-07-21 Thread Huw Davies
Adam Petaccia wrote:
 @@ -226,6 +277,11 @@ GpStatus WINGDIPAPI GdipSetEmpty(GpRegion*region)
 
  if(!(calls++))
  FIXME(not implemented\n);
 +TRACE(%p\n, region);
 +
 +GdipDeleteRegion(region);
 +GdipCreateRegion(region);
 +region-node-type = RegionDataEmptyRect;
 
  return NotImplemented;
  }

This is still wrong.  You probably don't even want to implement 
GdipSetEmpty in this patch anyway.

In general you may be better off submitting a smaller patch series 
rather than 16 patches in one go.

Huw.




Re: [Gdiplus 02/15] Implement GdipCreateRegion

2008-07-19 Thread Huw Davies
Adam Petaccia wrote:
  @@ -226,6 +277,11 @@ GpStatus WINGDIPAPI GdipSetEmpty(GpRegion*region)
 
   if(!(calls++))
   FIXME(not implemented\n);
  +TRACE(%p\n, region);
  +
  +GdipDeleteRegion(region);
  +GdipCreateRegion(region);
  +region-node-type = RegionDataEmptyRect;
 
   return NotImplemented;
   }

This can't work.

You don't want to free the memory pointed to by region, simply free the 
nodes underneath it.

Huw.






Re: [Gdiplus 2/3] Add a test for a floating-point triangle

2008-07-10 Thread Huw Davies
Hi Adam,

This is mainly a brain-dump, there's nothing wrong the patch.


On Thu, Jul 10, 2008 at 12:44:55AM -0400, Adam Petaccia wrote:
 ---
  dlls/gdiplus/tests/region.c |   38 ++
  1 files changed, 38 insertions(+), 0 deletions(-)
 
 diff --git a/dlls/gdiplus/tests/region.c b/dlls/gdiplus/tests/region.c
 index 1b1b047..7b1eba5 100644
 --- a/dlls/gdiplus/tests/region.c
 +++ b/dlls/gdiplus/tests/region.c
 @@ -361,6 +361,44 @@ todo_wine
  expect(Ok, status);
  status = GdipDeleteRegion(region);
  expect(Ok, status);

 +status = GdipGetRegionDataSize(region, needed);
 +expect(Ok, status);
 +ok(needed == 72, Expected 72, got %d\n, needed);
 +status = GdipGetRegionData(region, (BYTE*)buf, sizeof(buf), needed);
 +expect(Ok, status);
 +expect_dword(buf, 64);
 +trace(buf[1] = %08x\n, buf[1]);
 +expect_dword(buf + 2, RGNDATA_MAGIC);
 +expect_dword(buf + 3, 0);
 +expect_dword(buf + 4, RGNDATA_PATH);
 +
 +expect_dword(buf + 5, 48);
 +expect_dword(buf + 6, RGNDATA_MAGIC);
 +expect_dword(buf + 7, 4);
 +expect_dword(buf + 8, 0);
 +expect_float(buf + 9, 5.6);
 +expect_float(buf + 10, 6.2);
 +expect_float(buf + 11, 7.2);
 +expect_float(buf + 12, 8.9);
 +expect_float(buf + 13, 8.1);
 +expect_float(buf + 14, 1.6);
 +expect_float(buf + 15, 5.6);
 +expect_float(buf + 16, 6.2);

There's one more DWORD left here (buf + 17).

I realised after I sent in my last lot of tests that this is the BYTE
array of PathPointType*s packed into DWORDs (hence the 0x81.. vs
0x01.. that you noted in an earlier patch).

As you've also noticed there are two ways of storing paths, if all the
co-ords are short ints then they get stored that way and the
0x4000 bit of buf + 8 is set.  Otherwise the co-ords are stored as floats.

In addition, the 0x2000 bit of that same DWORD corresponds to the FillMode
of the path.

Anyway, good work!

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: [Gdiplus 07/15] Stub GdipFillRegion

2008-07-09 Thread Huw Davies
On Wed, Jul 09, 2008 at 03:33:47AM -0400, Adam Petaccia wrote:
 ---
  dlls/gdiplus/gdiplus.spec |2 +-
  dlls/gdiplus/region.c |   11 +++
  2 files changed, 12 insertions(+), 1 deletions(-)
 
 diff --git a/dlls/gdiplus/region.c b/dlls/gdiplus/region.c
 index 3d95723..717119f 100644
 --- a/dlls/gdiplus/region.c
 +++ b/dlls/gdiplus/region.c
 @@ -156,6 +156,17 @@ GpStatus WINGDIPAPI GdipDeleteRegion(GpRegion *region)
  return NotImplemented;
  }
  
 +GpStatus WINGDIPAPI GdipFillRegion(GpGraphics* graphics, GpBrush* brush,
 +GpRegion* region)
 +{
 +if (!(graphics  brush  region))
 +return InvalidParameter;
 +
 +FIXME((%p, %p, %p): stub\n, graphics, brush, region);
 +
 +return NotImplemented;
 +}
 +

Msdn lists this in the 'graphics' section of the gdiplus flat api, so I'd
suggest it goes in graphics.c

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: [Gdiplus 11/15] Implement GdipCreateRegionRect

2008-07-09 Thread Huw Davies
On Wed, Jul 09, 2008 at 03:34:01AM -0400, Adam Petaccia wrote:
 ---
  dlls/gdiplus/region.c   |   28 
  dlls/gdiplus/tests/region.c |8 
  2 files changed, 28 insertions(+), 8 deletions(-)
 
 diff --git a/dlls/gdiplus/region.c b/dlls/gdiplus/region.c
 index 59e2fa6..de86ce1 100644
 --- a/dlls/gdiplus/region.c
 +++ b/dlls/gdiplus/region.c
 @@ -129,12 +129,32 @@ GpStatus WINGDIPAPI GdipCreateRegionPath(GpPath *path, 
 GpRegion **region)
  return Ok;
  }
  
 -GpStatus WINGDIPAPI GdipCreateRegionRect(GDIPCONST GpRectF *rect, GpRegion 
 **region)
 +GpStatus WINGDIPAPI GdipCreateRegionRect(GDIPCONST GpRectF *rect,
 +GpRegion **region)
  {
 -FIXME((%p, %p): stub\n, rect, region);
 +GpPath *path;
 +GpStatus stat;
  
 -*region = NULL;
 -return NotImplemented;
 +if (!(rect  region))
 +return InvalidParameter;
 +
 +TRACE(%p, %p\n, rect, region);
 +
 +*region = GdipAlloc(sizeof(GpRegion*));
 +if(!*region)
 +return OutOfMemory;
 +
 +stat = GdipCreatePath(FillModeAlternate, path);
 +if (stat != Ok) { GdipDeleteRegion(*region); return stat;}
 +stat = GdipAddPathRectangle(path, rect-X, rect-Y,
 +rect-Width, rect-Height);
 +if (stat != Ok) { GdipDeleteRegion(*region); return stat;}
 +stat = GdipCreateRegionPath(path, region);
 +if (stat != Ok) { GdipDeleteRegion(*region); return stat;}
 +
 +GdipDeletePath(path);
 +
 +return Ok;
  }


This doesn't look right.  See the GdipGetRegionData tests (and extend
them to add paths), these are supposed to help understanding how
regions are stored.  It looks to me that a region is stored as a
sequence of rects and paths that are combined with various CombineMode
ops.

I'll send a patch that adds paths to the test in a bit.

Huw. 
-- 
Huw Davies
[EMAIL PROTECTED]




Re: [Gdiplus 11/15] Implement GdipCreateRegionRect

2008-07-09 Thread Huw Davies
On Wed, Jul 09, 2008 at 11:58:23AM +0100, Huw Davies wrote:
 This doesn't look right.  See the GdipGetRegionData tests (and extend
 them to add paths), these are supposed to help understanding how
 regions are stored.  It looks to me that a region is stored as a
 sequence of rects and paths that are combined with various CombineMode
 ops.

Hi Adam,

In fact you want to store the region elements as a binary tree,
something like this:

typedef enum element_type
{
rect_element = 0x1000,
path_element,
empty_element,
infinite_element
} element_type_t;

typedef struct region_element
{
DWORD type_or_op; /* One of the CombineModes or element_type_t */
union
{
GpRectF rect;
GpPath *path;
struct
{
struct region_element *first;
struct region_element *second;
} operands;
} elem_data;
} region_element_t;

struct GpRegion
{
region_element_t root;
};

A region element is either a rect, path or a combining op (in which
case it has two children, 'first' and 'second').  The data returned by
GdipGetRegionData is then basically what happens if you walk the tree,
favouring the 'first' node at every branch.

Does that make sense?

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: [Gdiplus 11/15] Implement GdipCreateRegionRect

2008-07-09 Thread Huw Davies
On Wed, Jul 09, 2008 at 10:56:48AM -0400, Adam Petaccia wrote:
 I didn't mean to imply that I ignored the region.c header, but I didn't
 use any of its information for the bounds tests yet, so I didn't add
 them to the struct (and I've learned to add things as needed).

The bounds checks are really the second stage.  I'd get the
CreateRegion functions working correctly first of all, and check that
you're happy that the GetRegionData stuff could be made to work (even
if you don't actually code it).  Once you have that, you know you're
storing the data in a similar way to native gdiplus.  Then you can go
on to work on the bounds (which would appear to be non-trivial - good
luck!).

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: user32: sizeof DDEPOKE and DDEDATA is six, which is probably not what is expected, so use offsetof instead.

2008-07-08 Thread Huw Davies
James Hawkins wrote:
 2008/7/8 Huw Davies [EMAIL PROTECTED]:
 Fixes a todo_wine and a Valgrind warning.
 
 @@ -699,10 +699,7 @@ static HDDEDATA CALLBACK
 server_ddeml_callback(UINT uType, UINT uFmt, HCONV hcon
 
  ptr = (LPSTR)DdeAccessData(hdata, size);
  ok(!lstrcmpA(ptr, poke data\r\n), Expected 'poke
 data\\r\\n', got %s\n, ptr);
 -todo_wine
 -{
 -ok(size == 14, Expected 14, got %d\n, size);
 -}
 +ok(size == 12, Expected 12, got %d\n, size);
  DdeUnaccessData(hdata);
 
  size = DdeQueryStringA(server_pid, hsz2, str, MAX_PATH, CP_WINANSI);
 
 You changed the value of the test.  Did you mean to do that?

Yes.  See 'size' in create_poke().

Huw.





Re: gdiplus: Add a test to show that bitmap fonts aren't used for fontfamilies.

2008-07-08 Thread Huw Davies
Adam Petaccia wrote:
 On Tue, 2008-07-08 at 13:20 +0100, Huw Davies wrote:
 ---
  dlls/gdiplus/tests/font.c |7 +++
  1 files changed, 7 insertions(+), 0 deletions(-)
 
 On my Windows system I can't find MSSansSerif, only Microsoft Sans
 Serif. I'm not sure if that's a valid test to prove that point. 

It's a bitmap font.  The (latin version of the) font file is called 
sserife.fon .  Basically the FontFamily stuff relies on scalable 
outlines, so gdiplus seems to simply ignore bitmap fonts.

It's also a hint that Wine's GdipGetGenericFontFamilySansSerif() is 
incorrect - in the absence of 'Microsoft Sans Serif' then 'Tahoma' may 
be a better choice.

Huw.




Re: [Gdiplus 4/4] Implement GdipCloneImage

2008-07-02 Thread Huw Davies
On Tue, Jul 01, 2008 at 09:41:22PM -0400, Adam Petaccia wrote:
 ---
  dlls/gdiplus/image.c   |   19 +--
  dlls/gdiplus/tests/image.c |6 --
  2 files changed, 17 insertions(+), 8 deletions(-)
 
 diff --git a/dlls/gdiplus/image.c b/dlls/gdiplus/image.c
 index bf2ac35..59ea837 100644
 --- a/dlls/gdiplus/image.c
 +++ b/dlls/gdiplus/image.c
 @@ -244,9 +244,24 @@ GpStatus WINGDIPAPI GdipCloneImage(GpImage *image, 
 GpImage **cloneImage)

 +memcpy((*cloneImage)-picture, image-picture, sizeof(IPicture));
 +
 +return Ok;
  }
  

You can't simply copy an interface ptr like this.

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: Gdiplus [1/3] Improve accuracy in calculating height

2008-06-27 Thread Huw Davies
On Fri, Jun 27, 2008 at 10:16:41AM -0400, Adam Petaccia wrote:
 The previous patch I sent incorrectly calculated font-height. Now its
 correctly calculated using points_per_inch and the user's DPI setting.
 

 diff --git a/dlls/gdiplus/font.c b/dlls/gdiplus/font.c
 index 86a3223..85df905 100644
 --- a/dlls/gdiplus/font.c
 +++ b/dlls/gdiplus/font.c
 @@ -34,6 +34,7 @@ WINE_DEFAULT_DEBUG_CHANNEL (gdiplus);
  #include gdiplus_private.h
 
  static const REAL mm_per_pixel = 25.4;
 +static const REAL points_per_inch = 1/72;
  

I know there are several definitions of a point, but none are this big!

points_per_inch should be around 72.


 @@ -50,7 +51,7 @@ static inline REAL get_dpi (void)
  
  static inline REAL point_to_pixel (REAL point)
  {
 -return point * 1.5;
 +return point * (get_dpi() * points_per_inch);

And this then becomes points * get_dpi() / points_per_inch;

[Hint check the units:
 pixels (aka dots) = points * (dots / inch) / (points / inch)
]

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: Gdiplus [1/3] Improve accuracy in calculating height

2008-06-27 Thread Huw Davies
On Fri, Jun 27, 2008 at 03:27:44PM +0100, Huw Davies wrote:
 On Fri, Jun 27, 2008 at 10:16:41AM -0400, Adam Petaccia wrote:
  --- a/dlls/gdiplus/font.c
  +++ b/dlls/gdiplus/font.c
  @@ -34,6 +34,7 @@ WINE_DEFAULT_DEBUG_CHANNEL (gdiplus);
   #include gdiplus_private.h
  
   static const REAL mm_per_pixel = 25.4;

Ah and I missed this one.  Please rename to mm_per_inch !

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: Memory leak patch

2008-06-26 Thread Huw Davies
On Thu, Jun 26, 2008 at 03:25:30AM -0700, Joris Huizer wrote:
 

 dlls/gdiplus/font.c |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/dlls/gdiplus/font.c b/dlls/gdiplus/font.c
index 9ef8edc..86122da 100644
--- a/dlls/gdiplus/font.c
+++ b/dlls/gdiplus/font.c
@@ -42,6 +42,7 @@ static inline REAL get_dpi (void)
 HDC hdc = GetDC(0);
 GdipCreateFromHDC (hdc, graphics);
 GdipGetDpiX(graphics, dpi);
+GdipFree(graphics);
 ReleaseDC (0, hdc);
 
 return dpi;

This should be GdipDeleteGraphics.

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




Re: memory leak fix

2008-06-26 Thread Huw Davies
On Thu, Jun 26, 2008 at 02:05:15AM -0700, Joris Huizer wrote:
 Untested, but this seems trivial/obvious, unless I overlooked something
 
 
 
   
 From 4026f68e4404c4b1822eefe0b3419db4f896e055 Mon Sep 17 00:00:00 2001
 From: Joris Huizer [EMAIL PROTECTED](none)
 Date: Thu, 26 Jun 2008 11:07:11 +0200
 Subject: [PATCH] memory leak fix
 
 ---
  dlls/gdiplus/font.c |1 +
  1 files changed, 1 insertions(+), 0 deletions(-)
 
 diff --git a/dlls/gdiplus/font.c b/dlls/gdiplus/font.c
 index 0bb9fc7..9ef8edc 100644
 --- a/dlls/gdiplus/font.c
 +++ b/dlls/gdiplus/font.c
 @@ -378,6 +378,7 @@ GpStatus WINGDIPAPI 
 GdipCreateFontFamilyFromName(GDIPCONST WCHAR *name,
  ffamily-FamilyName = GdipAlloc(LF_FACESIZE * sizeof (WCHAR));
  if (!ffamily-FamilyName)
  {
 +GdipFree(ffamily-tmw);
  GdipFree(ffamily);
  return OutOfMemory;
  }

That's a good start, but there's more.  The function leaks an hfont
both on success and in this failure case.  Also there should be a
ReleaseDC in the failure case.

In addition it seems to me that the tmw element may as well be an
embedded structure rather than a structure pointer.  That'll save
having to worry about alloc'ing/free'ing it.

Huw.
-- 
Huw Davies
[EMAIL PROTECTED]




  1   2   >