Hi Jacek,
Jacek Caban jacek@codeweavers.com wrote:
This is part of the fix for bug 40282.
Could you please add actual tests that show the handling of disabled window state? Neither current tests nor the new ones actually check that at all, having such tests would prevent new regressions in this area. Feel free to use my tests from the staging repository as a base if desired, or write your own ones that use the same approach.
On 04/05/16 13:59, Dmitry Timoshkov wrote:
Hi Jacek,
Jacek Caban jacek@codeweavers.com wrote:
This is part of the fix for bug 40282.
Could you please add actual tests that show the handling of disabled window state? Neither current tests nor the new ones actually check that at all, having such tests would prevent new regressions in this area. Feel free to use my tests from the staging repository as a base if desired, or write your own ones that use the same approach.
This patch is not directly related to enabled/disabled state of parent/owner. I plan to send such tests together with the other part of the fix. For the reference, I attached a patch that (on top on this one) passes all your tests.
Thanks, Jacek
Jacek Caban jacek@codeweavers.com wrote:
This is part of the fix for bug 40282.
Could you please add actual tests that show the handling of disabled window state? Neither current tests nor the new ones actually check that at all, having such tests would prevent new regressions in this area. Feel free to use my tests from the staging repository as a base if desired, or write your own ones that use the same approach.
This patch is not directly related to enabled/disabled state of parent/owner. I plan to send such tests together with the other part of the fix.
It still looks like shooting in the dark to me without writing the tests first and only then starting to fix the test failures one by one, so it becomes absolutely clear what exactly is fixed and where. Once again I'd like to point out that writing exhaustive tests before starting to add any fixes is very important for better understanding of the problem, especially in such fargile areas as user32 window management code.
For the reference, I attached a patch that (on top on this one) passes all your tests.
I have more tests that show that removing DF_OWNERENABLED handling altogether (like my own patches also do) is wrong. These tests are not part of the staged patch set.
On 04/05/16 14:34, Dmitry Timoshkov wrote:
Jacek Caban jacek@codeweavers.com wrote:
This is part of the fix for bug 40282.
Could you please add actual tests that show the handling of disabled window state? Neither current tests nor the new ones actually check that at all, having such tests would prevent new regressions in this area. Feel free to use my tests from the staging repository as a base if desired, or write your own ones that use the same approach.
This patch is not directly related to enabled/disabled state of parent/owner. I plan to send such tests together with the other part of the fix.
It still looks like shooting in the dark to me without writing the tests first and only then starting to fix the test failures one by one, so it becomes absolutely clear what exactly is fixed and where. Once again I'd like to point out that writing exhaustive tests before starting to add any fixes is very important for better understanding of the problem, especially in such fargile areas as user32 window management code.
Actually, my patch shows that we handle parent window in a wrong way. Period. That's what's obviously wrong and that's what I'm fixing at the moment. I don't touch enabling and disabling owner in this patch. In fact, it shouldn't change it at all. So please, forget about context of this bug, do you still have any comment about this particular patch?
For the reference, I attached a patch that (on top on this one) passes all your tests.
I have more tests that show that removing DF_OWNERENABLED handling altogether (like my own patches also do) is wrong. These tests are not part of the staged patch set.
Yep, that's why I only attached it for the reference and didn't send it to wine-patches. It needs more tests and I don't consider your test good enough.
Jacek
Jacek Caban jacek@codeweavers.com wrote:
This is part of the fix for bug 40282.
Could you please add actual tests that show the handling of disabled window state? Neither current tests nor the new ones actually check that at all, having such tests would prevent new regressions in this area. Feel free to use my tests from the staging repository as a base if desired, or write your own ones that use the same approach.
This patch is not directly related to enabled/disabled state of parent/owner. I plan to send such tests together with the other part of the fix.
It still looks like shooting in the dark to me without writing the tests first and only then starting to fix the test failures one by one, so it becomes absolutely clear what exactly is fixed and where. Once again I'd like to point out that writing exhaustive tests before starting to add any fixes is very important for better understanding of the problem, especially in such fargile areas as user32 window management code.
Actually, my patch shows that we handle parent window in a wrong way. Period. That's what's obviously wrong and that's what I'm fixing at the moment. I don't touch enabling and disabling owner in this patch. In fact, it shouldn't change it at all. So please, forget about context of this bug, do you still have any comment about this particular patch?
It seems that we heard something similar when you sent the patches last time that have caused this regression. And absense of good tests in both cases doesn't add a lot of confidence that there will be no new regressions.
For the reference, I attached a patch that (on top on this one) passes all your tests.
I have more tests that show that removing DF_OWNERENABLED handling altogether (like my own patches also do) is wrong. These tests are not part of the staged patch set.
Yep, that's why I only attached it for the reference and didn't send it to wine-patches. It needs more tests and I don't consider your test good enough.
Probably my tests are not good for you, but please show your tests first, so we could make a comparison. Btw, it's pretty clear to me that you used my tests as a reference for of your test, if my tests were that bad you wouldn't do that, right? :)
On 04/05/16 15:04, Dmitry Timoshkov wrote:
Jacek Caban jacek@codeweavers.com wrote:
This is part of the fix for bug 40282.
Could you please add actual tests that show the handling of disabled window state? Neither current tests nor the new ones actually check that at all, having such tests would prevent new regressions in this area. Feel free to use my tests from the staging repository as a base if desired, or write your own ones that use the same approach.
This patch is not directly related to enabled/disabled state of parent/owner. I plan to send such tests together with the other part of the fix.
It still looks like shooting in the dark to me without writing the tests first and only then starting to fix the test failures one by one, so it becomes absolutely clear what exactly is fixed and where. Once again I'd like to point out that writing exhaustive tests before starting to add any fixes is very important for better understanding of the problem, especially in such fargile areas as user32 window management code.
Actually, my patch shows that we handle parent window in a wrong way. Period. That's what's obviously wrong and that's what I'm fixing at the moment. I don't touch enabling and disabling owner in this patch. In fact, it shouldn't change it at all. So please, forget about context of this bug, do you still have any comment about this particular patch?
It seems that we heard something similar when you sent the patches last time that have caused this regression. And absense of good tests in both cases doesn't add a lot of confidence that there will be no new regressions.
Once again: how is my *current* patch related at all to what you request?
For the reference, I attached a patch that (on top on this one) passes all your tests.
I have more tests that show that removing DF_OWNERENABLED handling altogether (like my own patches also do) is wrong. These tests are not part of the staged patch set.
Yep, that's why I only attached it for the reference and didn't send it to wine-patches. It needs more tests and I don't consider your test good enough.
Probably my tests are not good for you, but please show your tests first, so we could make a comparison. Btw, it's pretty clear to me that you used my tests as a reference for of your test, if my tests were that bad you wouldn't do that, right? :)
I said not good *enough*, I never said they are not good, so stop accusing me about something I didn't say. And if I had tests ready, I'd already send this *unrelated* patch. I will learn to not share with you WIP diffs if you don't understand what they are.
And well, if it matters that much to you, your tests are nice for overview of the bug, but it's hard to point the exact problem from them, so more detailed tests written with implementation in mind are needed and that's how I discovered the problem I'm fixing. They also show that we're only interested in only WS_CHILD style (and usually just for wine_todos), so calling them for all style combinations feels a bit like out-placed shooting in the dark, not showing the exact problem. More detailed tests are required to show exact problems with WS_CHILD in dialog templates.
Jacek
Jacek Caban jacek@codeweavers.com wrote:
> This is part of the fix for bug 40282. Could you please add actual tests that show the handling of disabled window state? Neither current tests nor the new ones actually check that at all, having such tests would prevent new regressions in this area. Feel free to use my tests from the staging repository as a base if desired, or write your own ones that use the same approach.
This patch is not directly related to enabled/disabled state of parent/owner. I plan to send such tests together with the other part of the fix.
It still looks like shooting in the dark to me without writing the tests first and only then starting to fix the test failures one by one, so it becomes absolutely clear what exactly is fixed and where. Once again I'd like to point out that writing exhaustive tests before starting to add any fixes is very important for better understanding of the problem, especially in such fargile areas as user32 window management code.
Actually, my patch shows that we handle parent window in a wrong way. Period. That's what's obviously wrong and that's what I'm fixing at the moment. I don't touch enabling and disabling owner in this patch. In fact, it shouldn't change it at all. So please, forget about context of this bug, do you still have any comment about this particular patch?
It seems that we heard something similar when you sent the patches last time that have caused this regression. And absense of good tests in both cases doesn't add a lot of confidence that there will be no new regressions.
Once again: how is my *current* patch related at all to what you request?
All ask you about is writing a comprehensive set of test cases before you start trying to fix anything. Your fixes simply lack any resonable justification from the tests. And yes, the regression is caused by enabling wrong window at the end of dialog message loop, that's why I'm asking about the tests that check disabled window state.
I'm sure that if I would send such a patch on my own it would be immediately rejected with the "needs tests" motivation.
For the reference, I attached a patch that (on top on this one) passes all your tests.
I have more tests that show that removing DF_OWNERENABLED handling altogether (like my own patches also do) is wrong. These tests are not part of the staged patch set.
Yep, that's why I only attached it for the reference and didn't send it to wine-patches. It needs more tests and I don't consider your test good enough.
Probably my tests are not good for you, but please show your tests first, so we could make a comparison. Btw, it's pretty clear to me that you used my tests as a reference for of your test, if my tests were that bad you wouldn't do that, right? :)
I said not good *enough*, I never said they are not good, so stop accusing me about something I didn't say. And if I had tests ready, I'd already send this *unrelated* patch. I will learn to not share with you WIP diffs if you don't understand what they are.
Apparently my comment was not about the WIP diff, and I didn't accuse you in anything, just mentioned an obvious fact.
And well, if it matters that much to you, your tests are nice for overview of the bug, but it's hard to point the exact problem from them, so more detailed tests written with implementation in mind are needed and that's how I discovered the problem I'm fixing. They also show that we're only interested in only WS_CHILD style (and usually just for wine_todos), so calling them for all style combinations feels a bit like out-placed shooting in the dark, not showing the exact problem. More detailed tests are required to show exact problems with WS_CHILD in dialog templates.
Probaly testing all the style combinations is an overkill, but if there is an existing test that already loops around all of them and adding new tests there seems like an easy way of checking whether there are any other styles besides WS_CHILD, WS_POPUP and WS_EX_CONTROLPARENT that may change things why don't use that opportunity? And without testing them all I'd personally refrain from claims that only WS_CHILD makes difference. Or you prefer to wait for another regression report?
Hi Dmitry,
I will ask you one more time to go back to the code and being technical instead of personal. Take a look at the code and show me a code path, that makes enabling or disabling any different than before the patch. If you can't then you will understand that the change is unrelated. If you do, I'm happy to write a test for that case.
Jacek
On 05.04.2016 16:28, Jacek Caban wrote:
Hi Dmitry,
I will ask you one more time to go back to the code and being technical instead of personal. Take a look at the code and show me a code path, that makes enabling or disabling any different than before the patch. If you can't then you will understand that the change is unrelated. If you do, I'm happy to write a test for that case.
Jacek
To both of you, please calm down a bit. Whereas there is nothing obviously wrong with the patch, I can also understand Dmitrys request to merge more tests first, before we come to the conclusion that this solution must be right. In fact, when taking into account the feedback I received quite often to my own patches, the second approach (first ensure tests cover all areas, then changing the implementation) is indeed preferred for Wine, so I'm not really sure what the problem is here. How should Dmitry add his own tests when you want to send all in a single patch? Since Dmitry already did the effort to track down your regression, wouldn't it be useful to trust him a bit more and to cooperate?
Imho, what we especially need to ensure is that wineserver handling for various flag combinations is really correct, before we modify the owner on user32 side. Based on some tests I did a while ago (when handing for WS_POPUP | WS_CHILD was added), there are definitely remaining todos. Also please note that neither your old nor your new code correctly replicates wineserver behaviour because WIN_CreateWindowEx contains a check: if ((cs->style & (WS_CHILD|WS_POPUP)) != WS_CHILD) Under specific circumstances the owner argument is simply ignored.
Regards, Sebastian
Hi Sebastian,
On 04/05/16 17:25, Sebastian Lackner wrote:
On 05.04.2016 16:28, Jacek Caban wrote:
Hi Dmitry,
I will ask you one more time to go back to the code and being technical instead of personal. Take a look at the code and show me a code path, that makes enabling or disabling any different than before the patch. If you can't then you will understand that the change is unrelated. If you do, I'm happy to write a test for that case.
Jacek
To both of you, please calm down a bit. Whereas there is nothing obviously wrong with the patch, I can also understand Dmitrys request to merge more tests first, before we come to the conclusion that this solution must be right. In fact, when taking into account the feedback I received quite often to my own patches, the second approach (first ensure tests cover all areas, then changing the implementation) is indeed preferred for Wine, so I'm not really sure what the problem is here. How should Dmitry add his own tests when you want to send all in a single patch? Since Dmitry already did the effort to track down your regression, wouldn't it be useful to trust him a bit more and to cooperate?
To be honest, I don't. I said it multiple time: his requests are unrelated. I will send those tests when they will be related to the patch I send. In fact I'd be writing them now if I wouldn't have to deal with this nonsense. But still and again, this it's unrelated to this patch, so this stays as is. Let's keep it separated, I will try to explain it one last time in a different mail.
Imho, what we especially need to ensure is that wineserver handling for various flag combinations is really correct, before we modify the owner on user32 side. Based on some tests I did a while ago (when handing for WS_POPUP | WS_CHILD was added), there are definitely remaining todos. Also please note that neither your old nor your new code correctly replicates wineserver behaviour because WIN_CreateWindowEx contains a check: if ((cs->style & (WS_CHILD|WS_POPUP)) != WS_CHILD) Under specific circumstances the owner argument is simply ignored.
Yes, it's intentionally different, because my tests show it should be different. If we pass WS_CHILD, then the argument that we pass as owner is in fact a parent. A parent does not have the requirements of being top level window (or one with no WS_CHILD flag to be precise). Otherwise it's an owner, which has the restriction. That's how CreateWindow works, it's tested and fixed by [1]. Tests that I included in the patch show that dialogs created by DialogBoxIndirectParam are different. They have the same restriction applied to the case with WS_CHILD as if it was an owner. My tests clearly show that. So to conclude: - I did fix todos you talk about. At least ones I know about. - I did add test that proves it's different in this case and that it has to be done in user32. Does that sound right to you?
Jacek
[1] http://source.winehq.org/git/wine.git/commitdiff/d13a44e4aa12fa5ad3498c39934...
Jacek Caban jacek@codeweavers.com wrote:
I will ask you one more time to go back to the code and being technical instead of personal.
Hmm? My comments have pure technical background, all I ask is about adding more tests to show that your patch is correct and to prevent possible regressions in future.
Take a look at the code and show me a code path, that makes enabling or disabling any different than before the patch. If you can't then you will understand that the change is unrelated. If you do, I'm happy to write a test for that case.
Sorry, but that's your job to demonstrate with a bunch of test cases that your patch is correct, I'm just pointing out to obvious omissions in the provided tests. If that's too much of an additional effort for you for some reason then I have no idea why you decided to work on this at all, just drop this, user32 is too complex area for hasty craft, and requires a lot of time to write the tests and investigate things.
On 04/05/16 17:55, Dmitry Timoshkov wrote:
Jacek Caban jacek@codeweavers.com wrote:
I will ask you one more time to go back to the code and being technical instead of personal.
Hmm? My comments have pure technical background, all I ask is about adding more tests to show that your patch is correct and to prevent possible regressions in future.
And if you want me to take you seriously as a reviewer, then start reading the code. Let me read it for you. We call EnableWindow() in two functions:
1) DIALOG_CreateIndirect Before and after my patch, we use exactly the same window in DIALOG_CreateIndirect: I didn't change that logic at all. Do you see it?
2) DIALOG_DoDialogBox In DIALOG_DoDialogBox we currently use GetWindow(GW_OWNER) (which is a subject for future changes). My patch doesn't change it as well. Here we have two cases:
2a) dialog is WS_CHILD: For WS_CHILD, owner is NULL with and without my patch. 2b) For other cases, we have a real owner, but then again, the owner will be the same before and after my patch. We know that, I added a test and fix for that in [1] and I ensure that in tests included to the patch.
End of story.
As I told you, I will write a patch for that, when I will work on that. This patch is about other, unrelated, issue that I do test in the patch.
If that's too much of an additional effort for you for some reason then I have no idea why you decided to work on this at all, just drop this, user32 is too complex area for hasty craft, and requires a lot of time to write the tests and investigate things.
And that's personal again.
Jacek
[1] http://source.winehq.org/git/wine.git/commitdiff/d13a44e4aa12fa5ad3498c39934...
Jacek Caban jacek@codeweavers.com wrote:
I will ask you one more time to go back to the code and being technical instead of personal.
Hmm? My comments have pure technical background, all I ask is about adding more tests to show that your patch is correct and to prevent possible regressions in future.
And if you want me to take you seriously as a reviewer, then start reading the code. Let me read it for you. We call EnableWindow() in two functions:
- DIALOG_CreateIndirect
Before and after my patch, we use exactly the same window in DIALOG_CreateIndirect: I didn't change that logic at all. Do you see it?
- DIALOG_DoDialogBox
In DIALOG_DoDialogBox we currently use GetWindow(GW_OWNER) (which is a subject for future changes). My patch doesn't change it as well. Here we have two cases:
2a) dialog is WS_CHILD: For WS_CHILD, owner is NULL with and without my patch. 2b) For other cases, we have a real owner, but then again, the owner will be the same before and after my patch. We know that, I added a test and fix for that in [1] and I ensure that in tests included to the patch.
End of story.
As I told you, I will write a patch for that, when I will work on that. This patch is about other, unrelated, issue that I do test in the patch.
The source of the regression that your patch is supposedly tries to fix is that the dialog code enables wrong window at the end of dialog message loop. Neither old nor current tests don't even pretend that they are testing that correct window is being enabled/disabled. And that's what I ask for: add such tests. You had refused to add more tests previously and as the result we have a regression.
Regarding the claims that only WS_CHILD style makes a difference: apply my tests and in the tests replace the checks (WS_CHILD|WS_POPUP) == WS_CHILD by just testing for WS_CHILD and the test will fail. If your tests don't fail then your tests simply don't check all the possible style combinations that may lead to failures.
If that's too much of an additional effort for you for some reason then I have no idea why you decided to work on this at all, just drop this, user32 is too complex area for hasty craft, and requires a lot of time to write the tests and investigate things.
And that's personal again.
Then every comment requesting to add more tests that actually confirm that the proposed changes are correct while the sender doesn't want to write such tests are personal. Refusing to create more tests to prove the correctness of the patch doesn't add a lot of confidence to that patch at all. Writing more tests is always an additional work, and claiming that a request to add the tests is personal sounds at least strange.
On 4/6/16 5:32 AM, Dmitry Timoshkov wrote:
The source of the regression that your patch is supposedly tries to fix is that the dialog code enables wrong window at the end of dialog message loop.
No, it fixes issue I found while looking at this, but it's a separated and isolated issue. I don't touch the code that you ask me to write a test for. You fail to understand that.
BTW, your tests don't test what you ask me to test and they were enough for you to write a wrong fix. Otherwise you'd know that this code in the end of the loop should be removed. Then again, it's not related to this particular patch.
Jacek
Jacek Caban jacek@codeweavers.com wrote:
The source of the regression that your patch is supposedly tries to fix is that the dialog code enables wrong window at the end of dialog message loop.
No, it fixes issue I found while looking at this, but it's a separated and isolated issue. I don't touch the code that you ask me to write a test for. You fail to understand that.
I perfectly understand what you are trying to fix, and you actually mentioned it yourself already: "This is part of the fix for bug 40282". And that bug is a regression caused by your previous changes not backed up by the tests, and I already explained that the source of the regression is in wrong handling of enable/disable of the dialog owner, and once again I'd like to point out that fixing this without any tests simply can't work.
BTW, your tests don't test what you ask me to test and they were enough for you to write a wrong fix. Otherwise you'd know that this code in the end of the loop should be removed. Then again, it's not related to this particular patch.
Well, at lease I have written some tests before I tried to fix anything, and I'm not claiming that my fix is perfect/correct/useful at all. You are more than welcome to show your own exhaustive tests that check every detail of the dialog owner enable/disable logic.
P.S. I'm not the person who commits patches to Wine, why are you arguing with me if it's pretty clear that you are not going to write any tests at all in order to demonstrate that your patch is correct?
On 4/6/16 10:05 AM, Dmitry Timoshkov wrote:
Jacek Caban jacek@codeweavers.com wrote:
The source of the regression that your patch is supposedly tries to fix is that the dialog code enables wrong window at the end of dialog message loop.
No, it fixes issue I found while looking at this, but it's a separated and isolated issue. I don't touch the code that you ask me to write a test for. You fail to understand that.
I perfectly understand what you are trying to fix, and you actually mentioned it yourself already: "This is part of the fix for bug 40282". And that bug is a regression caused by your previous changes not backed up by the tests, and I already explained that the source of the regression is in wrong handling of enable/disable of the dialog owner, and once again I'd like to point out that fixing this without any tests simply can't work.
You said it yourself, you comment on my comment, not the code. I added tests for the code I change.
BTW, your tests don't test what you ask me to test and they were enough for you to write a wrong fix. Otherwise you'd know that this code in the end of the loop should be removed. Then again, it's not related to this particular patch.
Well, at lease I have written some tests before I tried to fix anything, and I'm not claiming that my fix is perfect/correct/useful at all. You are more than welcome to show your own exhaustive tests that check every detail of the dialog owner enable/disable logic. P.S. I'm not the person who commits patches to Wine, why are you arguing with me if it's pretty clear that you are not going to write any tests at all in order to demonstrate that your patch is correct?
I already told you a few time that I will send it when I will be relevant. In fact I already did, but it's not yet ready for sending.
Stop wasting my time.
Jacek
Jacek Caban jacek@codeweavers.com wrote:
The source of the regression that your patch is supposedly tries to fix is that the dialog code enables wrong window at the end of dialog message loop.
No, it fixes issue I found while looking at this, but it's a separated and isolated issue. I don't touch the code that you ask me to write a test for. You fail to understand that.
I perfectly understand what you are trying to fix, and you actually mentioned it yourself already: "This is part of the fix for bug 40282". And that bug is a regression caused by your previous changes not backed up by the tests, and I already explained that the source of the regression is in wrong handling of enable/disable of the dialog owner, and once again I'd like to point out that fixing this without any tests simply can't work.
You said it yourself, you comment on my comment, not the code. I added tests for the code I change.
Your assumption is incorrect, but I really feel discouraged by your attempts to pretend that I comment not on your patch or that your patch fixes something not related to my comments.
BTW, your tests don't test what you ask me to test and they were enough for you to write a wrong fix. Otherwise you'd know that this code in the end of the loop should be removed. Then again, it's not related to this particular patch.
Well, at lease I have written some tests before I tried to fix anything, and I'm not claiming that my fix is perfect/correct/useful at all. You are more than welcome to show your own exhaustive tests that check every detail of the dialog owner enable/disable logic. P.S. I'm not the person who commits patches to Wine, why are you arguing with me if it's pretty clear that you are not going to write any tests at all in order to demonstrate that your patch is correct?
I already told you a few time that I will send it when I will be relevant. In fact I already did, but it's not yet ready for sending.
Stop wasting my time.
Why are you replying at all? Once again: I'm not the person who commits the patches to Wine, and I'm pretty skeptical that this kind of replies is going to help to strengthen your arguments.
Dmitry Timoshkov dmitry@baikal.ru writes:
P.S. I'm not the person who commits patches to Wine, why are you arguing with me if it's pretty clear that you are not going to write any tests at all in order to demonstrate that your patch is correct?
I'm not committing the patches until I'm satisfied that the tests are sufficient, and if you feel that they are not, I want to understand why, and make sure that the necessary tests get written. Your technical feedback is welcome and appreciated.
But accusing Jacek of refusing to write tests at all is neither true nor helpful. It would be nice if you could refrain from such comments and concentrate on technical issues, where I believe that you do have useful knowledge to contribute. Can we please all try to get along?
Alexandre Julliard julliard@winehq.org wrote:
P.S. I'm not the person who commits patches to Wine, why are you arguing with me if it's pretty clear that you are not going to write any tests at all in order to demonstrate that your patch is correct?
I'm not committing the patches until I'm satisfied that the tests are sufficient, and if you feel that they are not, I want to understand why, and make sure that the necessary tests get written. Your technical feedback is welcome and appreciated.
I think that I've already described the technical details about both the source of the regression that Jacek is trying to fix, and the fact that the tests that could confirm that the fix is correct simply don't exist at all.
But accusing Jacek of refusing to write tests at all is neither true nor helpful. It would be nice if you could refrain from such comments and concentrate on technical issues, where I believe that you do have useful knowledge to contribute. Can we please all try to get along?
So, is Jacek actually going to write the tests that would help avoiding new regressions in the area of dialog owner enable/disable management logic instead of pretending that his patch fixes something else?
Dmitry Timoshkov dmitry@baikal.ru writes:
I think that I've already described the technical details about both the source of the regression that Jacek is trying to fix, and the fact that the tests that could confirm that the fix is correct simply don't exist at all.
I haven't seen Jacek claim that the patch fixes the regression. I think we all agree that more changes are needed, with corresponding tests of course.
As far as I can tell, your objection is not to this specific patch, but to the fact that we shouldn't commit anything until we have a full solution? If so, I certainly don't have a problem with waiting.
So, is Jacek actually going to write the tests that would help avoiding new regressions in the area of dialog owner enable/disable management logic instead of pretending that his patch fixes something else?
Yes, he is writing more tests, he said so already.
Alexandre Julliard julliard@winehq.org wrote:
I think that I've already described the technical details about both the source of the regression that Jacek is trying to fix, and the fact that the tests that could confirm that the fix is correct simply don't exist at all.
I haven't seen Jacek claim that the patch fixes the regression. I think we all agree that more changes are needed, with corresponding tests of course.
As far as I can tell, your objection is not to this specific patch, but to the fact that we shouldn't commit anything until we have a full solution? If so, I certainly don't have a problem with waiting.
My objection is that this patch pretends to be a partial fix for the regression while there is no any tests for the regression source at all, or the tests that actually demonstrate how all of this is supposed to work with correct implementation.
At the same time we see numerous claims that my tests are not good enough and the fix I've created for this regression is not correct, while there nothing has been shown as a viable alternative to them, just empty claims and hints (stay tuned, I'm working on something!) and accusations that I'm not understanding the proposed fix.
On 04/06/16 11:43, Dmitry Timoshkov wrote:
So, is Jacek actually going to write the tests that would help avoiding new regressions in the area of dialog owner enable/disable management logic
Once again: yes and I don't need you to tell me that, I'd do that anyway, it's more than obvious to me. I will send those tests when I will fix it. Still, this patch is unrelated.
instead of pretending that his patch fixes something else?
I don't pretend anything, go back to the code and see. I fix parent of created window. See it yourself, it's right there. That's it.
Jacek
Jacek Caban jacek@codeweavers.com wrote:
instead of pretending that his patch fixes something else?
I don't pretend anything, go back to the code and see. I fix parent of created window. See it yourself, it's right there. That's it.
You are clamimg that your patch is a partial fix for the regression, perhaps it's better to start with sending the tests that show what's wrong with current implementation and only then start sending patches one by one with fixes for every problem found by the tests targeted separately?
On 04/06/16 13:00, Dmitry Timoshkov wrote:
You are clamimg
And again, you comment the claim, not the code.
Jacek
-
Alexandre Julliard -
Dmitry Timoshkov -
Jacek Caban -
Sebastian Lackner