March 2012 - Wine-Devel - list.winehq.org

Re: [PATCH] strmbase: Display dll in which we are searching a class to instanciate.

by Christian Costa

2012/3/28 Alexandre Julliard <julliard(a)winehq.org> > Christian Costa <titan.costa(a)gmail.com> writes: > > > 2012/3/28 Alexandre Julliard <julliard(a)winehq.org> > > > >> Christian Costa <titan.costa(a)gmail.com> writes: > >> > >> > @@ -360,14 +361,17 @@ HRESULT WINAPI > STRMBASE_DllGetClassObject(REFCLSID > >> rclsid, REFIID riid, LPVOID * > >> > break; > >> > } > >> > > >> > + if (!GetModuleFileNameA(g_hInst, dllname, sizeof(dllname))) > >> > + strcpy(dllname, "???"); > >> > >> You don't want to do that in the success case. > >> > >> > > GetModuleFileNameA returns the length of the string copied to the buffer. > > The length is 0 in case of error. > > What I mean is that you only need the module name in case the class is > not found (and really, only in the null constructor case). > > It's convenient to see from a log where the class is supposed to be implemented but since this can be retreive in the registry, it can be remove.

March 28, 2012

Re: [PATCH] strmbase: Display dll in which we are searching a class to instanciate.

by Alexandre Julliard

Christian Costa <titan.costa(a)gmail.com> writes: > 2012/3/28 Alexandre Julliard <julliard(a)winehq.org> > >> Christian Costa <titan.costa(a)gmail.com> writes: >> >> > @@ -360,14 +361,17 @@ HRESULT WINAPI STRMBASE_DllGetClassObject(REFCLSID >> rclsid, REFIID riid, LPVOID * >> > break; >> > } >> > >> > + if (!GetModuleFileNameA(g_hInst, dllname, sizeof(dllname))) >> > + strcpy(dllname, "???"); >> >> You don't want to do that in the success case. >> >> > GetModuleFileNameA returns the length of the string copied to the buffer. > The length is 0 in case of error. What I mean is that you only need the module name in case the class is not found (and really, only in the null constructor case). -- Alexandre Julliard julliard(a)winehq.org

March 28, 2012

Re: [PATCH] strmbase: Display dll in which we are searching a class to instanciate.

by Christian Costa

2012/3/28 Alexandre Julliard <julliard(a)winehq.org> > Christian Costa <titan.costa(a)gmail.com> writes: > > > @@ -360,14 +361,17 @@ HRESULT WINAPI STRMBASE_DllGetClassObject(REFCLSID > rclsid, REFIID riid, LPVOID * > > break; > > } > > > > + if (!GetModuleFileNameA(g_hInst, dllname, sizeof(dllname))) > > + strcpy(dllname, "???"); > > You don't want to do that in the success case. > > GetModuleFileNameA returns the length of the string copied to the buffer. The length is 0 in case of error.

March 28, 2012

Re: 'Pending' patches state

by Jerome Leclanche

On Wed, Mar 28, 2012 at 12:01 PM, Alexandre Julliard <julliard(a)winehq.org>wrote: > Michael Stefaniuc <mstefani(a)redhat.com> writes: > > >> The pending state is feedback. It means that the patch is not clearly > > yes, but the worst possible feedback. > > > > New people assume you or the area maintainer need to still make up their > > mind on the patch but that's not the case, it is a done deal. > > Not necessarily. Patches are automatically marked pending as soon as I > look at them, until they get a more permanent resolution. There can be > many different reasons why they don't get one. > > > Iff one knows you and knows to read in between the lines then the above > > can be reworded as: > > "Current implementation rejected; idea might have some merit." > > > > That's what "Pending" means most of the times. > > Sometimes, yes. > > > Now for the other meanings of "Pending": > > - "Waiting for feedback from the main developer in that area." > > Don't remember to have ever seen that. Those patches stay normally in > > "New" as you don't look at them before the area developer ACKs. > > No, I usually do look at them. Sometimes I go back and mark them as New > again, but not always. > > > - "preferably in the form of a test case" > > That should be "Needs tests". > > It would be, in cases where tests are clearly the right way. There are > cases where it's not clear whether a test is feasible, and using "Needs > tests" may lead the submitter down the wrong path. > > > - With the two (minor) other meanings of "Pending" handled by other > > patch states we can rename "Pending" to "Convince me" or "Needs work". > > That would make it more obvious what is meant. > > I can certainly rename "pending" to "convince me" if it helps, but I > think it's important to have some sort of open-ended resolution to leave > room for developer's judgement. There are many cases where I'm genuinely > undecided about the right resolution, and I feel it's preferable to show > the undecision and leave it to the developer to try to address one way > or the other, even if that's of course harder than being told exactly > what to do. > > -- > Alexandre Julliard > julliard(a)winehq.org > I think the general feeling is that Pending should be renamed to "Decision pending" and that more feedback is needed at least in the form of "this is the wrong approach" or "this may be the right approach, explain yourself better". But the general feeling is that "Pending" currently means "Not good enough" and never gets looked at again. I agree it's very confusing. J. Leclanche

March 28, 2012

Re: 'Pending' patches state

by Alexandre Julliard

Michael Stefaniuc <mstefani(a)redhat.com> writes: >> The pending state is feedback. It means that the patch is not clearly > yes, but the worst possible feedback. > > New people assume you or the area maintainer need to still make up their > mind on the patch but that's not the case, it is a done deal. Not necessarily. Patches are automatically marked pending as soon as I look at them, until they get a more permanent resolution. There can be many different reasons why they don't get one. > Iff one knows you and knows to read in between the lines then the above > can be reworded as: > "Current implementation rejected; idea might have some merit." > > That's what "Pending" means most of the times. Sometimes, yes. > Now for the other meanings of "Pending": > - "Waiting for feedback from the main developer in that area." > Don't remember to have ever seen that. Those patches stay normally in > "New" as you don't look at them before the area developer ACKs. No, I usually do look at them. Sometimes I go back and mark them as New again, but not always. > - "preferably in the form of a test case" > That should be "Needs tests". It would be, in cases where tests are clearly the right way. There are cases where it's not clear whether a test is feasible, and using "Needs tests" may lead the submitter down the wrong path. > - With the two (minor) other meanings of "Pending" handled by other > patch states we can rename "Pending" to "Convince me" or "Needs work". > That would make it more obvious what is meant. I can certainly rename "pending" to "convince me" if it helps, but I think it's important to have some sort of open-ended resolution to leave room for developer's judgement. There are many cases where I'm genuinely undecided about the right resolution, and I feel it's preferable to show the undecision and leave it to the developer to try to address one way or the other, even if that's of course harder than being told exactly what to do. -- Alexandre Julliard julliard(a)winehq.org

March 28, 2012

Re: 'Pending' patches state

by Dmitry Timoshkov

Alexandre Julliard <julliard(a)winehq.org> wrote: > > WM_SHOWWINDOW at the start and at the end of every message sequence > > means that ShowWindow() should be used to hide and show the window > > during SetParent call processing. > > That's the sort of explanation you should have included in your > patch, instead of expecting me to dig through the source or the list > archive to find it. Plus of course some explanation as to why the test > can't be marked as succeeding despite the change. There are other things inside of SetParent that break message sequences, that's why the patches were prepended with a comment explaining what's wrong with Setparent and saying that it's the first step. > > Taking an opportunity to discuss other my patches :) I'd like to get > > a comment to 84685 as well. > > Try making it simpler. Thanks. -- Dmitry.

March 28, 2012

Re: [PATCH] strmbase: Display dll in which we are searching a class to instanciate.

by Alexandre Julliard

Christian Costa <titan.costa(a)gmail.com> writes: > @@ -360,14 +361,17 @@ HRESULT WINAPI STRMBASE_DllGetClassObject(REFCLSID rclsid, REFIID riid, LPVOID * > break; > } > > + if (!GetModuleFileNameA(g_hInst, dllname, sizeof(dllname))) > + strcpy(dllname, "???"); You don't want to do that in the success case. -- Alexandre Julliard julliard(a)winehq.org

March 28, 2012

Re: 'Pending' patches state

by Michael Stefaniuc

Alexandre, On 03/28/2012 10:17 AM, Alexandre Julliard wrote: > Dmitry Timoshkov <dmitry(a)baikal.ru> writes: > >> It's very confusing, and absolutely not clear what is required from the >> patch submitter, especially since *there is no any feedback on the patch*. >> 'Rejected' at least requies some sort of feedback, while 'Pending' doesn't. >> To me 'Pending' looks like a silent case of 'Reject', but without any >> obligation to explain why. >> >> I find myself on somewhat shaky ground when I see a bunch of my patches >> in the pending state, especially if they already contain the tests and >> main developer in the area I'm changing is Alexandre :). >> >> Is it possible to get at least some feedback for pending patches? Pretty >> please? > > The pending state is feedback. It means that the patch is not clearly yes, but the worst possible feedback. New people assume you or the area maintainer need to still make up their mind on the patch but that's not the case, it is a done deal. > correct, but that it's complicated to articulate exactly why. Like it > says, you should try to make it more convincing, either by simplifying > the patch, or writing a test case. Iff one knows you and knows to read in between the lines then the above can be reworded as: "Current implementation rejected; idea might have some merit." That's what "Pending" means most of the times. Now for the other meanings of "Pending": - "Waiting for feedback from the main developer in that area." Don't remember to have ever seen that. Those patches stay normally in "New" as you don't look at them before the area developer ACKs. - "preferably in the form of a test case" That should be "Needs tests". - With the two (minor) other meanings of "Pending" handled by other patch states we can rename "Pending" to "Convince me" or "Needs work". That would make it more obvious what is meant. > For instance your patch 84692 says that "tests confirm that", but you > don't say which tests, and there are no new tests or fixed todos in the > patch, so it looks suspicious. Yes, I could dig out the tests myself and > investigate it in detail, but when it gets to that point I usually just > move on to the next patch, hence "pending". IMHO that should have been "Needs tests"; that would have forced Dmitry to point you to the tests. bye michael

March 28, 2012

Re: strmbase: add beginnings of BaseWindow

by Alexandre Julliard

Aric Stewart <aric(a)codeweavers.com> writes: > +HRESULT WINAPI BaseWindow_Destroy(BaseWindow *This) > +{ > + if (This->hWnd) > + BaseWindowImpl_DoneWithWindow(This); > + > + UnregisterClassW(This->pClassName, NULL); You most likely don't want to unregister the class every time. > + wnd_class_registered = GetClassInfoW(This->hInstance, This->pClassName, > + &winclass); > + > + if (!wnd_class_registered) > + { There shouldn't be any reason to do this. -- Alexandre Julliard julliard(a)winehq.org

March 28, 2012

Re: 'Pending' patches state

by Alexandre Julliard

Dmitry Timoshkov <dmitry(a)baikal.ru> writes: > I'm sorry, but that's not a feedback, and casual contributors may even > not be aware of that patch tracking page. And as I mentioned if the patch > already contains the tests it's not really obvious what should be added > in addition. In the light of recent discussions about friendliness to > users in bugzilla, I think that developers deserve at least small fraction > of friendliness as well (Alexandre, you are nice and friendly all the time, > but at least I sometimes feel like sending the patches to a blackwhole). If you want to write a script that sends a nice friendly mail every time a patch changes status I'd be happy to use it. > There is not much tests for SetParent, and 84692 suggests to look at > the tests added by > http://www.winehq.org/pipermail/wine-patches/2011-February/098711.html > > WM_SHOWWINDOW at the start and at the end of every message sequence > means that ShowWindow() should be used to hide and show the window > during SetParent call processing. That's the sort of explanation you should have included in your patch, instead of expecting me to dig through the source or the list archive to find it. Plus of course some explanation as to why the test can't be marked as succeeding despite the change. > Taking an opportunity to discuss other my patches :) I'd like to get > a comment to 84685 as well. Try making it simpler. -- Alexandre Julliard julliard(a)winehq.org

March 28, 2012