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
Re: Patch feedback (GetProductInfo)
Fredag 03 juli 2009 11:10:01 skrev Nikolay Sivov: > Alexander Nicolaysen Sørnes wrote: > > Hello, > > > > Could someone give me a hint as to what is wrong with the following > > patches? > > > > http://www.winehq.org/pipermail/wine-patches/2009-June/074908.html > > http://www.winehq.org/pipermail/wine-patches/2009-June/074909.html > > > > They only adds some defines plus a stub, so hopefully there isn't that > > much I can have done wrong :) > > > > > > > > Alexander N. Sørnes > > Hi, Alexander. > > First of all constant defines should go to winnt.h to match PSDK. > Next I found RtlGetProductInfo() call in winnt.h. That's make me think > that kernel32 call should be a thin wrapper > over this ntdll call. I don't have Vista installed (I suppose it's Vista > related call), so I can't check. > > After that it might be nice to have a combo in winecfg to choose edition > - as I know upcoming Win7 also has > this product categories. Default value could be Ultimate of course. And > combo should only appear on >= Vista. > > Nikolay S. Thanks!
Re: Patch feedback (GetProductInfo)
Alexander Nicolaysen Sørnes wrote: Hello, Could someone give me a hint as to what is wrong with the following patches? http://www.winehq.org/pipermail/wine-patches/2009-June/074908.html http://www.winehq.org/pipermail/wine-patches/2009-June/074909.html They only adds some defines plus a stub, so hopefully there isn't that much I can have done wrong :) Alexander N. Sørnes Hi, Alexander. First of all constant defines should go to winnt.h to match PSDK. Next I found RtlGetProductInfo() call in winnt.h. That's make me think that kernel32 call should be a thin wrapper over this ntdll call. I don't have Vista installed (I suppose it's Vista related call), so I can't check. After that it might be nice to have a combo in winecfg to choose edition - as I know upcoming Win7 also has this product categories. Default value could be Ultimate of course. And combo should only appear on >= Vista. Nikolay S.
Patch feedback (GetProductInfo)
Hello, Could someone give me a hint as to what is wrong with the following patches? http://www.winehq.org/pipermail/wine-patches/2009-June/074908.html http://www.winehq.org/pipermail/wine-patches/2009-June/074909.html They only adds some defines plus a stub, so hopefully there isn't that much I can have done wrong :) Alexander N. Sørnes
Re: MSI Patch Feedback
On Fri, Jan 9, 2009 at 9:52 AM, ricardo filipe wrote: > > >>>"How should I attempt to test the changes I have made" >> I don't see testcases for any of these changes. Adding conformance >> tests would be the best first course of action: >> >> http://wiki.winehq.org/ConformanceTests >> http://wiki.winehq.org/WritingConformanceTests >> >> -- >> -Austin >> >> > > he is asking how he should test... i think he knows he has to do them :) > i had the same problem a while back and got no response too so it's on > standby... > There is no way to test the setting of environment variables by msi. You can't query the changed environment variable until the process exits. Using a child process doesn't help either. -- James Hawkins
RE: MSI Patch Feedback
>>"How should I attempt to test the changes I have made" > I don't see testcases for any of these changes. Adding conformance > tests would be the best first course of action: > > http://wiki.winehq.org/ConformanceTests > http://wiki.winehq.org/WritingConformanceTests > > -- > -Austin > > he is asking how he should test... i think he knows he has to do them :) i had the same problem a while back and got no response too so it's on standby... _ Show them the way! Add maps and directions to your party invites. http://www.microsoft.com/windows/windowslive/events.aspx
Re: MSI Patch Feedback
On Fri, Jan 9, 2009 at 3:18 AM, Andrew Nguyen wrote: > Hello, > > I've written a few patches against dlls/msi/action.c in an effort to > rectify a difference in behavior with an MSI-based installer between > Windows and Wine with regard to environment variable processing. > Behavior not explicitly enumerated by MSDN is noted when a combination > of the environment table "+" name prefix and "[~]" value expression > are combined. How should I attempt to test the changes I have made, > and is there anything I can do to improve the quality of my changes? > Feedback is appreciated! > > -Andrew > > > > I don't see testcases for any of these changes. Adding conformance tests would be the best first course of action: http://wiki.winehq.org/ConformanceTests http://wiki.winehq.org/WritingConformanceTests -- -Austin
MSI Patch Feedback
Hello, I've written a few patches against dlls/msi/action.c in an effort to rectify a difference in behavior with an MSI-based installer between Windows and Wine with regard to environment variable processing. Behavior not explicitly enumerated by MSDN is noted when a combination of the environment table "+" name prefix and "[~]" value expression are combined. How should I attempt to test the changes I have made, and is there anything I can do to improve the quality of my changes? Feedback is appreciated! -Andrew msipatches.tar.bz2 Description: BZip2 compressed data
Re: Patch Feedback
Hi Alistair, Alistair Leslie-Hughes wrote: > Hi, > > Is there anything wrong with patches? > > [1/2] mshtml: Implement IHTMLScriptElement get/put event > > [2/2] mshtml: Implement IHTMLScriptElement get/put htmlFor > > First of all, are you trying to fix a real bug? We're starting to handle scripts ourselves, so your implementation is not complete. It would be fine to add such semi-stubs if it fixes a real bug. Otherwise I'd prefer to leave them as stubs so we don't hide bugs. Also, we have NS_FAILED macro that you should use instead of !NS_SUCCEEDED and printing a message on failure would be nice. Jacek
Patch Feedback
Hi, Is there anything wrong with patches? [1/2] mshtml: Implement IHTMLScriptElement get/put event [2/2] mshtml: Implement IHTMLScriptElement get/put htmlFor Best Regards Alistair Leslie-Hughes
Re: Patch feedback
Lei Zhang a écrit : > On Sun, Jul 6, 2008 at 6:55 AM, Alexander Nicolaysen Sørnes > <[EMAIL PROTECTED]> wrote: > >> Hello, >> >> I would really appreciate any feedback as to how these patches can be >> improved >> so that they will be accepted into Wine. >> >> regedit: Add support for importing Unicode files >> http://www.winehq.org/pipermail/wine-patches/2008-June/056856.html >> >> I know this patch is a little big, but I'm not sure how it could be split up >> without possible data loss on import. Is that acceptable? I could for >> instance make a first patch that merely reads the file as Uncode then >> converts each line to multibyte characters. >> > > Sure. How about: > > 1 patch for the first two utility functions. > 1 patch for each W version of an existing function. > 1 or 2 patches to glue it all together. > > duplicating a lot of functions for A/W compatibility just shows that the internals of regedit should be moved to unicode first then your patch will be relatively straightforward A+ -- Eric Pouech "The problem with designing something completely foolproof is to underestimate the ingenuity of a complete idiot." (Douglas Adams)
Re: Patch feedback
On Sun, Jul 6, 2008 at 6:55 AM, Alexander Nicolaysen Sørnes <[EMAIL PROTECTED]> wrote: > Hello, > > I would really appreciate any feedback as to how these patches can be improved > so that they will be accepted into Wine. > > regedit: Add support for importing Unicode files > http://www.winehq.org/pipermail/wine-patches/2008-June/056856.html > > I know this patch is a little big, but I'm not sure how it could be split up > without possible data loss on import. Is that acceptable? I could for > instance make a first patch that merely reads the file as Uncode then > converts each line to multibyte characters. Sure. How about: 1 patch for the first two utility functions. 1 patch for each W version of an existing function. 1 or 2 patches to glue it all together.
Patch feedback
Hello, I would really appreciate any feedback as to how these patches can be improved so that they will be accepted into Wine. regedit: Add support for importing Unicode files http://www.winehq.org/pipermail/wine-patches/2008-June/056856.html I know this patch is a little big, but I'm not sure how it could be split up without possible data loss on import. Is that acceptable? I could for instance make a first patch that merely reads the file as Uncode then converts each line to multibyte characters. user32: Always use old-style colours for Windows 3.x http://www.winehq.org/pipermail/wine-patches/2008-June/056898.html Alexander N. Sørnes
Re: patch feedback?
Damjan Jovanovic <[EMAIL PROTECTED]> writes: > +classKey = SetupDiOpenClassRegKeyExW(&guid, KEY_ALL_ACCESS, > + DIOCR_INSTALLER, NULL, NULL); > +/* wine does incorrectly return FALSE, so ... */ > +ok(classKey != FALSE && classKey != INVALID_HANDLE_VALUE, > +"failed opening class key, error %ld\n", GetLastError()); Tests are not supposed to cope with Wine bugs, that defeats the whole purpose of the test. If returning FALSE is wrong then the test should fail in that case. -- Alexandre Julliard [EMAIL PROTECTED]
patch feedback?
Hi I've tried submitting a patch to wine-patches 3 times now in the past 3 weeks, and not only has it not been applied, but I never even got any feedback. Please tell me what I am doing wrong. The latest patch is below. Thank you Damjan --- Damjan Jovanovic <[EMAIL PROTECTED]> wrote: > Date: Thu, 3 Aug 2006 23:49:11 -0700 (PDT) > From: Damjan Jovanovic <[EMAIL PROTECTED]> > To: [EMAIL PROTECTED] > Subject: setupapi: test+patch for > SetupDiOpenClassRegKeyExW (try 3) > > wine's SetupDiOpenClassRegKeyExW returns FALSE > instead > of INVALID_HANDLE_VALUE in several places, and > doesn't > brace the GUID string like it should. The test > reveals > these problems, and the patch fixes them. > > ChangeLog: > * Fixed returns values, braced GUID string, and > added > a test, for SetupDiOpenClassRegKeyExW. > > Damjan Jovanovic ([EMAIL PROTECTED]) > > > a/dlls/setupapi/tests/Makefile.in 2006-07-10 > 18:01:08.0 +0200 > +++ b/dlls/setupapi/tests/Makefile.in 2006-08-04 > 16:15:05.0 +0200 > @@ -3,7 +3,7 @@ > SRCDIR= @srcdir@ > VPATH = @srcdir@ > TESTDLL = setupapi.dll > -IMPORTS = setupapi kernel32 > +IMPORTS = setupapi kernel32 advapi32 rpcrt4 > > CTESTS = \ > devinst.c \ > --- a/dlls/setupapi/tests/devinst.c 2006-08-03 > 10:07:11.0 +0200 > +++ b/dlls/setupapi/tests/devinst.c 2006-08-04 > 17:57:57.0 +0200 > @@ -27,6 +27,7 @@ > #include "winuser.h" > #include "winreg.h" > #include "setupapi.h" > +#include "rpc.h" > > #include "wine/test.h" > > @@ -63,7 +64,85 @@ > ok(ret, "SetupDiDestroyDeviceInfoList failed : > %ld\n", error); > } > > +static void test_SetupDiOpenClassRegKeyExW() > +{ > +GUID guid; > +RPC_STATUS rpcStatus; > +HKEY hkey; > +WCHAR *unbracedGuid; > +WCHAR *bracedGuid; > + > +rpcStatus = UuidCreate(&guid); > +if (rpcStatus != RPC_S_OK) > +{ > +ok(FALSE, "failed to generate test guid, > error %ld", rpcStatus); > +return; > +} > + > +/* Check return value for non-existant key */ > +hkey = SetupDiOpenClassRegKeyExW(&guid, > KEY_ALL_ACCESS, > +DIOCR_INSTALLER, NULL, NULL); > +ok(hkey == INVALID_HANDLE_VALUE, > +"invalid return value %p from > SetupDiOpenClassRegKeyExW " > +"for non-existant key, expected %p\n", > hkey, INVALID_HANDLE_VALUE); > + > +rpcStatus = UuidToStringW(&guid, > &unbracedGuid); > +if (rpcStatus != RPC_S_OK) > +{ > +ok(FALSE, "failed to get string form of > guid, error %ld", rpcStatus); > +return; > +} > + > +bracedGuid = HeapAlloc(GetProcessHeap(), 0, > +(lstrlenW(unbracedGuid) + 3) * > sizeof(WCHAR)); > +if (bracedGuid != NULL) > +{ > +HKEY classesKey; > + > +bracedGuid[0] = (WCHAR) '{'; > +memcpy(&bracedGuid[1], unbracedGuid, > +lstrlenW(unbracedGuid) * > sizeof(WCHAR)); > +bracedGuid[1 + lstrlenW(unbracedGuid)] = > (WCHAR) '}'; > +bracedGuid[1 + lstrlenW(unbracedGuid) + 1] > = (WCHAR) 0; > + > +SetLastError(0xdeadbeef); > +classesKey = > SetupDiOpenClassRegKeyExW(NULL, KEY_ALL_ACCESS, > +DIOCR_INSTALLER, NULL, NULL); > +ok(classesKey != INVALID_HANDLE_VALUE, > +"failed to open the device classes > registry key: error %ld\n", > +GetLastError()); > +if (classesKey != INVALID_HANDLE_VALUE) > +{ > +DWORD ret; > +HKEY classKey; > +ret = RegCreateKeyW(classesKey, > bracedGuid, &classKey); > +if (ret == ERROR_SUCCESS) > +{ > +RegCloseKey(classKey); > +SetLastError(0xdeadbeef); > +classKey = > SetupDiOpenClassRegKeyExW(&guid, KEY_ALL_ACCESS, > + DIOCR_INSTALLER, NULL, NULL); > +/* wine does incorrectly return > FALSE, so ... */ > +ok(classKey != FALSE && classKey != > INVALID_HANDLE_VALUE, > +"failed opening class key, > error %ld\n", GetLastError()); > +RegCloseKey(classKey); > +RegDeleteKeyW(classesKey, > bracedGuid); > +} > +else > +ok(FALSE, "failed creating device > key: error %ld\n", ret); > +RegCloseKey(classesKey); > +} > + > +HeapFree(GetProcessHeap(), 0, bracedGuid); > +} > +else > +ok(FALSE, "failed to allocate memory for > braced guid\n"); > + > +RpcStringFreeW(&unbracedGuid); > +} > + > START_TEST(devinst) > { > test_SetupDiCreateDeviceInfoListEx(); > +test_SetupDiOpenClassRegKeyExW(); > } > --- a/dlls/setupapi/devinst.c 2006-08-03 > 10:07:11.0 +0200 > +++ b/dlls/setupapi/devinst.c 2006-08-04 > 18:10:06.0 +0200 > @@ -1393,6 +1393,7 @@ > PVOID Reserved) > { > LPWSTR lpGuidString; > +LPWSTR lpBracedGuidString; > HKEY hClassesKey; > HKEY hClassKey; > LPCWSTR lpKeyName; > @@ -
Re: wined3d: GLSL Patch feedback requested
On Tuesday 13 June 2006 18:39, Jason Green wrote: > On 6/13/06, H. Verbeet <[EMAIL PROTECTED]> wrote: > > On 13/06/06, Raphael <[EMAIL PROTECTED]> wrote: > > > I only have one question: > > > i see you have a cache to reuse prgid in set_glsl_shader_program but > > > what happens if user/app change the shaders content (for example using > > > SetFunction) ? your pre-compiled program will be correct ? > > > (because i don't see any list cleanup in shaders function reset) > > > > Good point :-) > > I wonder if respecifying the source and recompiling the program is > > worth it, or if we should just create a new shader object. > > http://msdn.microsoft.com/library/default.asp?url=/library/en-us/directx9_c >/IDirect3DPixelShader9.asp > > >From what I can tell (for DX9 at least), the app shouldn't be able to > > use SetFunction(). I thought it was an internal wined3d function. > Unless it's a DX8 call that I'm unaware of. I haven't seen any apps > try to do that yet, but maybe I'm just not looking closely enough. > > :-) The apps tend to CreatePixelShader(), then later [often during > > the render loop], they SetPixelShader(). I haven't seen any that > modify an existing shader yet. No seems correct to me (from DX8-9 APIs) but never say never :) Anyway cleaning glprograms on Delete can be a good idea :) And we must think, for futur, about the ressources manager (was last oliver works). As in some cases the CG will not be able to contains all precompiled shaders couples, and we must swap them on need ;) Best Regards and thx to you two, Raphael pgprt8voNzH1B.pgp Description: PGP signature
Re: wined3d: GLSL Patch feedback requested
On 13/06/06, Jason Green <[EMAIL PROTECTED]> wrote: From what I can tell (for DX9 at least), the app shouldn't be able to use SetFunction(). You're right.
Re: wined3d: GLSL Patch feedback requested
On 6/13/06, H. Verbeet <[EMAIL PROTECTED]> wrote: On 13/06/06, Raphael <[EMAIL PROTECTED]> wrote: > I only have one question: > i see you have a cache to reuse prgid in set_glsl_shader_program but what > happens if user/app change the shaders content (for example using > SetFunction) ? your pre-compiled program will be correct ? > (because i don't see any list cleanup in shaders function reset) Good point :-) I wonder if respecifying the source and recompiling the program is worth it, or if we should just create a new shader object. http://msdn.microsoft.com/library/default.asp?url=/library/en-us/directx9_c/IDirect3DPixelShader9.asp From what I can tell (for DX9 at least), the app shouldn't be able to use SetFunction(). I thought it was an internal wined3d function. Unless it's a DX8 call that I'm unaware of. I haven't seen any apps try to do that yet, but maybe I'm just not looking closely enough. :-) The apps tend to CreatePixelShader(), then later [often during the render loop], they SetPixelShader(). I haven't seen any that modify an existing shader yet.
Re: wined3d: GLSL Patch feedback requested
On 13/06/06, Raphael <[EMAIL PROTECTED]> wrote: I only have one question: i see you have a cache to reuse prgid in set_glsl_shader_program but what happens if user/app change the shaders content (for example using SetFunction) ? your pre-compiled program will be correct ? (because i don't see any list cleanup in shaders function reset) Good point :-) I wonder if respecifying the source and recompiling the program is worth it, or if we should just create a new shader object. - It is correct to have GLSL binding code out of "if (useVertexShaderFunction)" block ? It is, actually. If there's no shader in use, we need to tell GL to use fixed function again. The glUseProgramObjectARB call takes care of that. (programId is zero when no shaders are in use).
Re: wined3d: GLSL Patch feedback requested
On Thursday 08 June 2006 17:35, Jason Green wrote: > The current cumulative patch is located here: > > http://cmhousing.net/wine/glsl_cumul.diff > I hate you, reviewing all this patch made me crazy. Anyway is an impressive work I only have one question: i see you have a cache to reuse prgid in set_glsl_shader_program but what happens if user/app change the shaders content (for example using SetFunction) ? your pre-compiled program will be correct ? (because i don't see any list cleanup in shaders function reset) And many comments: void inline drawPrimitiveDrawStrided(IWineD3DDevice *iface, BOOL useVertexShaderFunction, BOOL usePixelShaderFunction, int useHW, WineDirect3DVertexStridedData *dataLocations, UINT numberOfvertices, UINT numberOfIndicies, GLenum glPrimType, const void *idxData, short idxSize, int minIndex, long StartIdx) { IWineD3DDeviceImpl *This = (IWineD3DDeviceImpl *)iface; @@ -1791,91 +1920,59 @@ #endif TRACE("Loaded arrays\n"); +/* Bind the correct GLSL shader program based on the currently set vertex & pixel shaders. */ +if (wined3d_settings.shader_mode == SHADER_GLSL) { +GLhandleARB programId; + +set_glsl_shader_program(iface); +programId = This->stateBlock->shaderPrgId; + +if (programId != 0) { +/* Start using this program ID */ +TRACE_(d3d_shader)("Using GLSL program %u\n", programId); +GL_EXTCALL(glUseProgramObjectARB(programId)); +checkGLcall("glUseProgramObjectARB"); +} +} + if (useVertexShaderFunction) { - - It is correct to have GLSL binding code out of "if (useVertexShaderFunction)" block ? - i think moving binding code (GLSL and ARB) on a new base shader method (ex IWineD3DBaseShaderImpl_Bind) will be cleaner (and permit to not have GLS/ARB code in drawprim) - same for shaders constant loading Best Regards, Raphael pgpu82Xo7y16L.pgp Description: PGP signature
Re: wined3d: GLSL Patch feedback requested
+ * If a program for the given combination does not exist, create one, and store + * that data in both shader objects so we can delete all of the programs later. + * If it creates a program, it will link the given objects, too. Is this comment still relevant? - Fix relative addressing (it was working correctly, then I lost some stuff when my hard drive got corrupted, and now it's using R0 instead of A0 as the address register for some reason) For 1.X shaders, the relative address token = 0 (a.k.a R0), and A0.x must be used instead. Another problem may be matrix instructions, which don't seem to set the reladdr token right now.
Re: wined3d: GLSL Patch feedback requested
That looks very impressive (good progress). I would have to look at the actual GLSL produced by this. The actual translation points look good. From: "Jason Green" <[EMAIL PROTECTED]> Date: Thu, 8 Jun 2006 11:54:04 -0400 By the way, here's a comparison screenshot of Civ4 from before my hard drive failed, and this is without having texturing on pixel shaders entirely working. Using GLSL (working completely for vertex shaders 2.0, at least the instructions that Civilization 4 used): http://cmhousing.net/wine/civ4_glsl.png compared to: http://cmhousing.net/wine/civ4_ingame2.png and http://cmhousing.net/wine/civ4_3.png with current git and using regular ARB shaders. So, we at least know that this is going to help. :-) On 6/8/06, Jason Green <[EMAIL PROTECTED]> wrote: The current cumulative patch is located here: http://cmhousing.net/wine/glsl_cumul.diff (This includes the recently submitted "Split constant loading out of drawPrim()" patch, which hasn't been applied to git yet) Before submitting, I plan to fix the following things: - Fix relative addressing (it was working correctly, then I lost some stuff when my hard drive got corrupted, and now it's using R0 instead of A0 as the address register for some reason) - Switch the shader_reg_maps.constantsF[] to a CHAR array instead of a BOOL array (maybe even a bitmap(?)) This whole patchset should be a no-op for normal users, unless they have the UseGLSL registry key enabled. Here's a list of the changelogs that this cumulative patch entails, somewhat in the order that I'll be sending them: [already sent] Move constant loading out of DrawPrimDrawStrided() - DrawPrim is just too big of a function. This separates the passing of constants to the shader into new functions. - Fixes an off-by-one error when loading vertex declaration constants (should be <, not <=) - Adds a function for GLSL loading of constants (aka Uniforms) - Adds a GLSL program variable to the stateblock and sets it to 0 (a future patch will actually create this program) Add GLSL helper functions to device.c - Adds functions to attach & detach shader objects, create and delete programs, and maintain the list of programs. - Adds a list of GLSL shader programs to the device which is initialized on Init3D(), and deleted on Release() Add the bulk of the GLSL string generation functions - Add a new file glsl_shader.c which contains almost every GLSL specific function we'll need - Move print_glsl_info() into glsl_shader.c - Move the shader_reg_maps struct info into the private header, and make it part of SHADER_OPCODE_ARG. - Create a new shared ps/vs register map for float constants (future patch will make ARB programs use this, too) - This is a big patch, but none of the new functions in glsl_shader.c are being called yet. This just sets them up. Unified float constant register mapping between ARB pixel and vertex shaders. - Got rid of the separate constant maps. - Side effect of this is that the map is a bit larger for pixel shaders than it needs to be. - Will make this dynamic in a future patch. Added more declarations to GLSL in generate_glsl_declarations() - Declare more variable names for GLSL programs. - Correct output name for pixel shaders (gl_FragColor instead of glFragColor) Map pixel shader instructions to GLSL generating functions. - Also, delete the GLSL program when the refcount hits 0 Map vertex shader instructions to GLSL generating functions. - Also, delete the GLSL program when the refcount hits 0 Allow drawPrim to create and use the GLSL program - Now that we can fully create a GLSL program, this patch lets us actually use it. I would have submitted these all separate for review, but I have some janitorial git cleanup to do first to get all of this in the right order. - Nick
Re: wined3d: GLSL Patch feedback requested
By the way, here's a comparison screenshot of Civ4 from before my hard drive failed, and this is without having texturing on pixel shaders entirely working. Using GLSL (working completely for vertex shaders 2.0, at least the instructions that Civilization 4 used): http://cmhousing.net/wine/civ4_glsl.png compared to: http://cmhousing.net/wine/civ4_ingame2.png and http://cmhousing.net/wine/civ4_3.png with current git and using regular ARB shaders. So, we at least know that this is going to help. :-) On 6/8/06, Jason Green <[EMAIL PROTECTED]> wrote: The current cumulative patch is located here: http://cmhousing.net/wine/glsl_cumul.diff (This includes the recently submitted "Split constant loading out of drawPrim()" patch, which hasn't been applied to git yet) Before submitting, I plan to fix the following things: - Fix relative addressing (it was working correctly, then I lost some stuff when my hard drive got corrupted, and now it's using R0 instead of A0 as the address register for some reason) - Switch the shader_reg_maps.constantsF[] to a CHAR array instead of a BOOL array (maybe even a bitmap(?)) This whole patchset should be a no-op for normal users, unless they have the UseGLSL registry key enabled. Here's a list of the changelogs that this cumulative patch entails, somewhat in the order that I'll be sending them: [already sent] Move constant loading out of DrawPrimDrawStrided() - DrawPrim is just too big of a function. This separates the passing of constants to the shader into new functions. - Fixes an off-by-one error when loading vertex declaration constants (should be <, not <=) - Adds a function for GLSL loading of constants (aka Uniforms) - Adds a GLSL program variable to the stateblock and sets it to 0 (a future patch will actually create this program) Add GLSL helper functions to device.c - Adds functions to attach & detach shader objects, create and delete programs, and maintain the list of programs. - Adds a list of GLSL shader programs to the device which is initialized on Init3D(), and deleted on Release() Add the bulk of the GLSL string generation functions - Add a new file glsl_shader.c which contains almost every GLSL specific function we'll need - Move print_glsl_info() into glsl_shader.c - Move the shader_reg_maps struct info into the private header, and make it part of SHADER_OPCODE_ARG. - Create a new shared ps/vs register map for float constants (future patch will make ARB programs use this, too) - This is a big patch, but none of the new functions in glsl_shader.c are being called yet. This just sets them up. Unified float constant register mapping between ARB pixel and vertex shaders. - Got rid of the separate constant maps. - Side effect of this is that the map is a bit larger for pixel shaders than it needs to be. - Will make this dynamic in a future patch. Added more declarations to GLSL in generate_glsl_declarations() - Declare more variable names for GLSL programs. - Correct output name for pixel shaders (gl_FragColor instead of glFragColor) Map pixel shader instructions to GLSL generating functions. - Also, delete the GLSL program when the refcount hits 0 Map vertex shader instructions to GLSL generating functions. - Also, delete the GLSL program when the refcount hits 0 Allow drawPrim to create and use the GLSL program - Now that we can fully create a GLSL program, this patch lets us actually use it. I would have submitted these all separate for review, but I have some janitorial git cleanup to do first to get all of this in the right order.
wined3d: GLSL Patch feedback requested
The current cumulative patch is located here: http://cmhousing.net/wine/glsl_cumul.diff (This includes the recently submitted "Split constant loading out of drawPrim()" patch, which hasn't been applied to git yet) Before submitting, I plan to fix the following things: - Fix relative addressing (it was working correctly, then I lost some stuff when my hard drive got corrupted, and now it's using R0 instead of A0 as the address register for some reason) - Switch the shader_reg_maps.constantsF[] to a CHAR array instead of a BOOL array (maybe even a bitmap(?)) This whole patchset should be a no-op for normal users, unless they have the UseGLSL registry key enabled. Here's a list of the changelogs that this cumulative patch entails, somewhat in the order that I'll be sending them: [already sent] Move constant loading out of DrawPrimDrawStrided() - DrawPrim is just too big of a function. This separates the passing of constants to the shader into new functions. - Fixes an off-by-one error when loading vertex declaration constants (should be <, not <=) - Adds a function for GLSL loading of constants (aka Uniforms) - Adds a GLSL program variable to the stateblock and sets it to 0 (a future patch will actually create this program) Add GLSL helper functions to device.c - Adds functions to attach & detach shader objects, create and delete programs, and maintain the list of programs. - Adds a list of GLSL shader programs to the device which is initialized on Init3D(), and deleted on Release() Add the bulk of the GLSL string generation functions - Add a new file glsl_shader.c which contains almost every GLSL specific function we'll need - Move print_glsl_info() into glsl_shader.c - Move the shader_reg_maps struct info into the private header, and make it part of SHADER_OPCODE_ARG. - Create a new shared ps/vs register map for float constants (future patch will make ARB programs use this, too) - This is a big patch, but none of the new functions in glsl_shader.c are being called yet. This just sets them up. Unified float constant register mapping between ARB pixel and vertex shaders. - Got rid of the separate constant maps. - Side effect of this is that the map is a bit larger for pixel shaders than it needs to be. - Will make this dynamic in a future patch. Added more declarations to GLSL in generate_glsl_declarations() - Declare more variable names for GLSL programs. - Correct output name for pixel shaders (gl_FragColor instead of glFragColor) Map pixel shader instructions to GLSL generating functions. - Also, delete the GLSL program when the refcount hits 0 Map vertex shader instructions to GLSL generating functions. - Also, delete the GLSL program when the refcount hits 0 Allow drawPrim to create and use the GLSL program - Now that we can fully create a GLSL program, this patch lets us actually use it. I would have submitted these all separate for review, but I have some janitorial git cleanup to do first to get all of this in the right order.