Re: Patch feedback requested for OleCreatePropertyFrame()
2010/1/8 Wolfram Sang : > Geoffrey Hausheer wrote: > >> Would someone mind reviewing this for style/content? I still have no > > Can't say much about the content, but stylewise the pointer handling > could be improved. For example, > > + if ( (iface==0) || (ppvObject==0) ) > > should at least use NULL instead of 0 as both are pointers, but !ptr is > even better here. Accordingly iface shouldn't generally be checked for NULL unless a program depends on it and a testcase shows this behaviour in native. -- Rob Shearman
Re: Patch feedback requested for OleCreatePropertyFrame()
Geoffrey Hausheer wrote:
> Would someone mind reviewing this for style/content? I still have no
Can't say much about the content, but stylewise the pointer handling
could be improved. For example,
+if ( (iface==0) || (ppvObject==0) )
should at least use NULL instead of 0 as both are pointers, but !ptr is
even better here. Accordingly
+*ppvObject = 0;
should assign NULL.
Also pointer declaration is not consistent, note the asterisk:
+ OLEPropertyPageSite *this = (OLEPropertyPageSite *)iface;
[...]
+ IPropertyPageSite* iface,
(and there are two spaces here)
I don't know which way is preferred, but for readability it should be
the same throughout the file.
+if (IsEqualGUID(&IID_IUnknown, riid))
+*ppvObject = iface;
+else if (IsEqualGUID(&IID_IPropertyPageSite, riid))
+*ppvObject = iface;
if (a || b) ?
+static ULONG WINAPI PPS_Release(IPropertyPageSite* iface)
+{
+ OLEPropertyPageSite *this = (OLEPropertyPageSite *)iface;
+ ULONG ret;
+ TRACE("(%p)->(ref=%d)\n", this, this->ref);
+
+ /* Decrease the reference count for current interface */
+ ret = InterlockedDecrement(&this->ref);
+
+ return ret;
+}
ret is not needed here.
+/* This was expected to work but didn't:
+LONG baseunits = GetDialogBaseUnits();
+basex = LOWORD(baseunits);
+basey = HIWORD(baseunits);
+*/
Don't know the common wine-sense here, but without syntax-highlighting,
I think it is easy to miss that this code is disabled. I'd suggest
/*
*
*/
commenting or something similar which is immediately visible.
Those are my 2 cents, thanks for the work!
Regards,
Wolfram
Re: Patch feedback requested for OleCreatePropertyFrame()
Ok, I have reimplemented OleCreatePropertyFrame() from scratch. There
is no code from Hidenori's patch in mine, nor did I use it as a
reference (though of course I can't claim that I didn't learn anything
from his patch during my previous cleanup)
Would someone mind reviewing this for style/content? I still have no
idea how to write a test for a modal dialog, so if a test is required
for submission, I'd really appreciate someone giving me pointers on
how to go about it.
There are some features I didn't implement because I didn't really
understand MDSN sufficiently (specifcially the IPropertyPageSite
stuff), but it is mostly complete.
.Geoff
From 07ca9bda0bd66e53cfbdc441b59fa4bc2e988171 Mon Sep 17 00:00:00 2001
From: Geoffrey Hausheer
Date: Thu, 7 Jan 2010 22:45:28 -0800
Subject: Implement OleCreatePropertyFrame from scratch
---
dlls/oleaut32/Makefile.in|1 +
dlls/oleaut32/olepropframe.c | 343 ++
dlls/oleaut32/stubs.c| 22 ---
3 files changed, 344 insertions(+), 22 deletions(-)
create mode 100644 dlls/oleaut32/olepropframe.c
diff --git a/dlls/oleaut32/Makefile.in b/dlls/oleaut32/Makefile.in
index a8ab7b4..d304257 100644
--- a/dlls/oleaut32/Makefile.in
+++ b/dlls/oleaut32/Makefile.in
@@ -17,6 +17,7 @@ C_SRCS = \
oleaut.c \
olefont.c \
olepicture.c \
+ olepropframe.c \
recinfo.c \
regsvr.c \
safearray.c \
diff --git a/dlls/oleaut32/olepropframe.c b/dlls/oleaut32/olepropframe.c
new file mode 100644
index 000..cd1841c
--- /dev/null
+++ b/dlls/oleaut32/olepropframe.c
@@ -0,0 +1,343 @@
+/*
+ * OleCreatePropertyFrame
+ *
+ * Copyright 2010 Geoffrey Hausheer
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+ *
+ */
+
+#include "config.h"
+#include "wine/port.h"
+
+#include
+#include
+#include
+#include
+#include
+
+#define COBJMACROS
+#define NONAMELESSUNION
+#define NONAMELESSSTRUCT
+
+#include "winerror.h"
+#include "windef.h"
+#include "winbase.h"
+#include "winnls.h"
+#include "winreg.h"
+#include "winuser.h"
+#include "lzexpand.h"
+
+#include "wine/unicode.h"
+#include "objbase.h"
+#include "typelib.h"
+#include "wine/debug.h"
+#include "variant.h"
+#include "wine/list.h"
+#include "ocidl.h"
+#include "prsht.h"
+#include "olectl.h"
+
+WINE_DEFAULT_DEBUG_CHANNEL(olepropframe);
+
+typedef struct
+{
+ const IPropertyPageSiteVtbl* lpVtbl;
+ LCID lcid;
+ LONG ref;
+} OLEPropertyPageSite;
+
+typedef struct {
+struct {
+ DLGTEMPLATE dlg;
+ WORD menu;
+ WORD class;
+ WORD caption;
+ WORD font;
+} dialog;
+OLEPropertyPageSite pps;
+IPropertyPage *propPage;
+} OLEPropertyFrame;
+
+static INT_PTR CALLBACK prop_sheet_proc(HWND hwnd, UINT msg, WPARAM wparam,
+ LPARAM lparam)
+{
+
+switch(msg)
+{
+case WM_INITDIALOG:
+ {
+RECT rect;
+HRESULT result;
+PROPSHEETPAGEW *psp = (PROPSHEETPAGEW *)lparam;
+OLEPropertyFrame *opf = (OLEPropertyFrame *)psp->lParam;
+
+TRACE("(%p, %s, %ld, %p)\n", hwnd, "WM_INITDIALOG",
+ wparam, opf->propPage);
+ZeroMemory( &rect, sizeof(rect) );
+GetClientRect( hwnd, &rect );
+result = IPropertyPage_Activate(opf->propPage, hwnd, &rect, TRUE);
+if(result == S_OK) {
+result = IPropertyPage_Show(opf->propPage, SW_SHOW );
+if(result == S_OK) {
+SetWindowLongPtrW( hwnd, DWLP_USER, (LONG)opf);
+}
+}
+return FALSE;
+ }
+case WM_DESTROY:
+ {
+OLEPropertyFrame *opf = (OLEPropertyFrame *)GetWindowLongPtrW( hwnd,
DWLP_USER );
+if (opf) {
+IPropertyPage_Show(opf->propPage, SW_HIDE);
+IPropertyPage_Deactivate(opf->propPage);
+SetWindowLongPtrW( hwnd, DWLP_USER, (LONG)NULL);
+}
+ }
+case WM_NOTIFY:
+ {
+OLEPropertyFrame *opf = (OLEPropertyFrame *)GetWindowLongPtrW( hwnd,
DWLP_USER );
+NMHDR *nmhdr = (NMHDR *)lparam;
+if(! opf)
+ break;
+
+switch(nmhdr->code)
+{
+case PSN_APPLY:
+FIXME("(%p, %s, %s)\n", hwnd, "WM_NOTIFY", "PSN_APPLY");
+IPropertyPage_Apply(opf->pro
Re: Patch feedback requested for OleCreatePropertyFrame()
On Mon, Jan 4, 2010 at 7:18 PM, Geoffrey Hausheer wrote: > On Mon, Jan 4, 2010 at 9:06 AM, Wolfram Sang wrote: >>> task. There doesn't appear to be anything in this code that would be >>> patent encumbered, but the code itself certainly falls under >>> Hidenori's copyright which he has effectively revoked permission to >>> use. >> >> Strictly speaking, this is not possible. Although IANAL, this code is LGPL, >> so he can't revoke permission for this version of the code. He can ask to >> not use it, and this wish can be granted. You still should be allowed to >> pick it up. Next question is then, if a modified version will be picked up >> (still respecting the author's wish)? Or how much modifications are needed? >> Geez, such things are never trivial :( >> > > Well, I put the LGPL header on the code. I thought I had that right > since the code was posted to wine-patches Anyhow, the legal side > isn't really relevant. The question is what Alexandre will accept as > far as it goes. The current code is certainly copyright by Hienori, > and since I couldn't contact him, I must assume he doesn't want any of > it in Wine. Thus my assumption is that I need to rewrite the entire > thing, if we want to obey his wishes (which I do). However, my hope > was that I could use the knowledge I gained from his implementation to > do it myself. As I'm already biased by that knowledge, if that won't > be allowed, then I can't do the work at all. > As far as I remember Hidenori asked for removal of his Quartz work because he was affright of patents, Microsoft. He didn't ask for removal of his other code (perhaps he did in a private mail) but I think Alexandre also removed his other code because it is not worth to take any risks. Recently we had a case which is a little bit similar. In that case someone wrote some dirty tests to figure out what Windows was doing and used that knowledge to write Wine code. The code in question was rejected but the guy also wasn't allowed to use his knowledge to submit a wine test. I'm quite certain that you can't resubmit Hidenori's code because it was removed for a good reason. I'm not sure about tests though but I guess we don't want to take any risks with that either :( Wait for Alexandre his answer though. Roderick
Re: Patch feedback requested for OleCreatePropertyFrame()
On Mon, Jan 4, 2010 at 9:06 AM, Wolfram Sang wrote: >> task. There doesn't appear to be anything in this code that would be >> patent encumbered, but the code itself certainly falls under >> Hidenori's copyright which he has effectively revoked permission to >> use. > > Strictly speaking, this is not possible. Although IANAL, this code is LGPL, > so he can't revoke permission for this version of the code. He can ask to > not use it, and this wish can be granted. You still should be allowed to > pick it up. Next question is then, if a modified version will be picked up > (still respecting the author's wish)? Or how much modifications are needed? > Geez, such things are never trivial :( > Well, I put the LGPL header on the code. I thought I had that right since the code was posted to wine-patches Anyhow, the legal side isn't really relevant. The question is what Alexandre will accept as far as it goes. The current code is certainly copyright by Hienori, and since I couldn't contact him, I must assume he doesn't want any of it in Wine. Thus my assumption is that I need to rewrite the entire thing, if we want to obey his wishes (which I do). However, my hope was that I could use the knowledge I gained from his implementation to do it myself. As I'm already biased by that knowledge, if that won't be allowed, then I can't do the work at all.
Re: Patch feedback requested for OleCreatePropertyFrame()
task. There doesn't appear to be anything in this code that would be patent encumbered, but the code itself certainly falls under Hidenori's copyright which he has effectively revoked permission to use. Strictly speaking, this is not possible. Although IANAL, this code is LGPL, so he can't revoke permission for this version of the code. He can ask to not use it, and this wish can be granted. You still should be allowed to pick it up. Next question is then, if a modified version will be picked up (still respecting the author's wish)? Or how much modifications are needed? Geez, such things are never trivial :( Regards, Wolfram
Re: Patch feedback requested for OleCreatePropertyFrame()
On Mon, Jan 4, 2010 at 4:39 AM, John Klehm wrote: > On Mon, Jan 4, 2010 at 4:46 AM, Huw Davies wrote: >> 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. >> > > In case anyone was just slightly less curious than me about it: > http://www.winehq.org/wwn/122#Quartz.dll%20Removal > Thanks for turning that up. I did look through the archives to see what hapened, but aparently not hard enough. So now the only solution appears to be to rewrite the code from scratch. The question is, can I do that now that I've looked over his code? It does turn this from a simple fix to a much more daunting task. There doesn't appear to be anything in this code that would be patent encumbered, but the code itself certainly falls under Hidenori's copyright which he has effectively revoked permission to use. .Geoff
Re: Patch feedback requested for OleCreatePropertyFrame()
On Mon, Jan 4, 2010 at 4:46 AM, Huw Davies wrote: > 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. > In case anyone was just slightly less curious than me about it: http://www.winehq.org/wwn/122#Quartz.dll%20Removal --John Klehm
Re: Patch feedback requested for OleCreatePropertyFrame()
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: Patch feedback requested for OleCreatePropertyFrame()
Hi Geoffrey, 2010/1/4 Geoffrey Hausheer : > Some software I'm using is configured via an OleCreatePropertyFrame > call which isn't implemented in Wine (ticket #16564). I can use > winetricks + dcom98 to get it working, but would prefer not to need > that. 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. I cleaned up the patch to compile against > current Git and am including it here for inspection. I can't find any > documentation on why it wasn't included at that time, and haven't done > any Wine development for a long time, so I'm not sure what it will > take to get it included. > > I have tested it, and it works pretty much fine for me (not quite as > pretty as the dcom menu, but it is functional). I'd be happy to write > a test for it if I had any idea how to do that, but I'm not sure where > to begin for a function like this. > > Anyhow, I'm open to feedback on what should be done before I submit > the patch for inclusion > > FYI: I was unable to find a currently valid email address for Takeshima Those defines bother me: +#define CPropertyPageContainerImpl_AddRef(pContainer) (++((pContainer)->ref)) +#define CPropertyPageContainerImpl_Release(pContainer) (--((pContainer)->ref)) Interlocked* functions? The other define looks ugly, testcases are missing, and A functions are used, rather than the W functions, which is probably what blocked this. Some of the FIXME messages should really be warns too. Cheers, Maarten.
Patch feedback requested for OleCreatePropertyFrame()
Some software I'm using is configured via an OleCreatePropertyFrame
call which isn't implemented in Wine (ticket #16564). I can use
winetricks + dcom98 to get it working, but would prefer not to need
that. 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. I cleaned up the patch to compile against
current Git and am including it here for inspection. I can't find any
documentation on why it wasn't included at that time, and haven't done
any Wine development for a long time, so I'm not sure what it will
take to get it included.
I have tested it, and it works pretty much fine for me (not quite as
pretty as the dcom menu, but it is functional). I'd be happy to write
a test for it if I had any idea how to do that, but I'm not sure where
to begin for a function like this.
Anyhow, I'm open to feedback on what should be done before I submit
the patch for inclusion
FYI: I was unable to find a currently valid email address for Takeshima
.Geoff
From 6fece5fb94617f00345a7116f6a40b783d1c1466 Mon Sep 17 00:00:00 2001
From: Geoffrey Hausheer
Date: Sun, 3 Jan 2010 14:22:25 -0800
Subject: Add support for OleCreatePropertyFrame
This patch was originally posted in 2001 by TAKESHIMA Hidenori
---
dlls/oleaut32/Makefile.in |1 +
dlls/oleaut32/propertyframe.c | 602 +
dlls/oleaut32/stubs.c | 22 --
3 files changed, 603 insertions(+), 22 deletions(-)
create mode 100644 dlls/oleaut32/propertyframe.c
diff --git a/dlls/oleaut32/Makefile.in b/dlls/oleaut32/Makefile.in
index a8ab7b4..493d4eb 100644
--- a/dlls/oleaut32/Makefile.in
+++ b/dlls/oleaut32/Makefile.in
@@ -17,6 +17,7 @@ C_SRCS = \
oleaut.c \
olefont.c \
olepicture.c \
+ propertyframe.c \
recinfo.c \
regsvr.c \
safearray.c \
diff --git a/dlls/oleaut32/propertyframe.c b/dlls/oleaut32/propertyframe.c
new file mode 100644
index 000..96b4e67
--- /dev/null
+++ b/dlls/oleaut32/propertyframe.c
@@ -0,0 +1,602 @@
+/*
+ * Copyright 2001 TAKESHIMA Hidenori
+ * Copyright 2010 Geoffrey Hausheer
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
+ *
+ *
--
+ * Known problems (2001, TAKESHIMA Hidenori)
+ * Should use PropertySheetW rather than PropertySheetA
+ */
+
+#include "config.h"
+#include "wine/port.h"
+
+#include
+
+#define COBJMACROS
+#define NONAMELESSUNION
+#define NONAMELESSSTRUCT
+
+#include "winerror.h"
+#include "windef.h"
+#include "winbase.h"
+#include "wingdi.h"
+#include "winuser.h"
+#include "ole2.h"
+#include "olectl.h"
+#include "oleauto.h"
+#include "commctrl.h"
+#include "wine/debug.h"
+
+WINE_DEFAULT_DEBUG_CHANNEL(ole);
+
+typedef struct CPropertyPageContainerImpl CPropertyPageContainerImpl;
+
+static const struct
+{
+DLGTEMPLATEtempl;
+WORDwMenuName;
+WORDwClassName;
+WCHARwDummyCaption;
+BYTEpadding[4];
+} propsite_dlg =
+{
+{
+WS_VISIBLE | WS_CHILD, /* style */
+0, /* dwExtendedStyle */
+0, /* cdit */
+0, /* x */
+0, /* y */
+0, /* cx */
+0, /* cy */
+},
+0, 0, 0,
+};
+
+typedef struct CPropertyPageSiteImpl
+{
+const IPropertyPageSiteVtbl *lpVtbl;/* Needs to be the first item
in the struct */
+/* IUnknown fields */
+LONGref;
+
+/* IPropertyPageSite fields */
+CPropertyPageContainerImpl*pContainer;
+IPropertyPage*pPage;
+HWNDhwnd;
+BYTEtempl[sizeof(propsite_dlg)];
+PROPPAGEINFOinfo;
+BOOLbActivate;
+} CPropertyPageSiteImpl;
+
+struct CPropertyPageContainerImpl
+{
+ULONGref; /* for IUnknown(not used now) */
+LCIDlcid;
+DWORDm_cSites;
+CPropertyPageSiteImpl**m_ppSites;
+PROPSHEETPAGEA*m_pPsp;
+HRESULT m_hr;
+};
+
+/* for future use. */
+#define CPropertyPageContainerImpl_AddRef(pContainer)
(++((pContainer)->ref))
+#define CPropertyPageContainerImpl_Release(pContainer)
(--((pContainer)->ref))
+
+
+/***/
+
+#define PropSiteDlg_Return
