Hi,
Sorry, but there's a bunch of issues I need to point out. Of course we could address them later, but it'll mean rewriting a bunch of code, and I'd like to have it correct from the beginning. In general, I think this implementation limits us too much.
For example, I don't think it's a good idea to have the TASKDIALOGCONFIG structure being filled in run_test_, we need to be able to control it easily.
Second, we also need to be able to return different values in the callback, the current approach doesn't account for that.
And the most important part, the separation from messages to send and received messages to test against. It's better to send the next message only when the last message arrived, to make sure we process them in the correct order. I attached a patch to show the problem (apply on top of my in-review patch). When sending all messages together, the first message is skipped, and we don't want that to happen.
Please reconsider my approach, I can try to simplify it if you want, but I think the complexity is justified to solve a complicated problem.
Regards, Fabian Maurer
On 30.08.2017 2:06, Fabian Maurer wrote:
Hi,
Sorry, but there's a bunch of issues I need to point out. Of course we could address them later, but it'll mean rewriting a bunch of code, and I'd like to have it correct from the beginning. In general, I think this implementation limits us too much.
No, you're right, if it means large changes later it should be done differently.
For example, I don't think it's a good idea to have the TASKDIALOGCONFIG structure being filled in run_test_, we need to be able to control it easily.
I have no problem with that, except for maybe initializing size/callback/user data in run_test still.
Second, we also need to be able to return different values in the callback, the current approach doesn't account for that.
Ok, this is easy to add.
And the most important part, the separation from messages to send and received messages to test against. It's better to send the next message only when the last message arrived, to make sure we process them in the correct order. I attached a patch to show the problem (apply on top of my in-review patch). When sending all messages together, the first message is skipped, and we don't want that to happen.
What do you mean by last message arrival? Currently all messages to control are posted, so we don't wait for anything but rely on callback to be called eventually. Do you mean that you want to specify message to be sent to control for every callback individually, like for example in mes_cancel_esc_callback1 - WM_KEYDOWN is sent in response to TDN_CREATED and TDM_CLICK_BUTTON in response to TDN_BUTTON_CLICKED?
I'll look into that.
Please reconsider my approach, I can try to simplify it if you want, but I think the complexity is justified to solve a complicated problem.
Yes, I will, thanks for your comments.
Regards, Fabian Maurer
Hello,
thanks for the quick response.
I have no problem with that, except for maybe initializing size/callback/user data in run_test still.
Yes, since that's always the same that makes sense. It's a minor issue really, we could pull it out later, too, but we too could just define it outside from the beginning.
Ok, this is easy to add.
If this is so, don't mind this. I just didn't know whether we could fit that into the message structure, too.
What do you mean by last message arrival? Currently all messages to control are posted, so we don't wait for anything but rely on callback to be called eventually. Do you mean that you want to specify message to be sent to control for every callback individually, like for example in mes_cancel_esc_callback1 - WM_KEYDOWN is sent in response to TDN_CREATED and TDM_CLICK_BUTTON in response to TDN_BUTTON_CLICKED?
Basically yes. Right now all messages are posted at once, and that's where we run into problems. When running my patch on a win7 machine, mes_cancel_esc_callback1 succeeds while mes_cancel_esc_callback2 fails. For some reason the "M_KEYDOWN, VK_ESCAPE" message is completely ignored, and we get IDOK. E.g., modify it like ------ static const struct message_info mes_cancel_esc_callback2[] = { { TDN_CREATED, 0, 0, S_OK, { { WM_KEYDOWN, VK_ESCAPE, 0 }, { TDM_CLICK_BUTTON, IDOK, 0 }, { 0 }}}, { TDN_BUTTON_CLICKED, IDOK, 0, S_OK }, { 0 } }; ------ and it works fine again. As you see, this is something that shouldn't happen. Honestly, I have no idea why that happens, maybe you know more. Could be that I'm just doing something really wrong, but this is the reason I switched to the new strategy - sending the new messages after the old were processed. I just found it to be way more reliable. I'm fine with a different strategy of course, it's just that this was the only way I managed to solve this particular issue.
On a site note, the second test fail with ------ taskdialog.c:213: Failed sequence Cancellation: stopped by callback 2: taskdialog.c:213: 0: expected: nothing - actual: 8000 wp 00000000 lp 00000000 taskdialog.c:213: 1: expected: nothing - actual: 8000 wp 00000000 lp 00000000 taskdialog.c:213: 2: expected: nothing - actual: 8000 wp 00000001 lp 00000000 taskdialog.c:213: 3: expected: nothing - actual: 8000 wp 00000000 lp 00000000 ------ and that's obviously wrong. Attached a quick hack, I assume this is a bug in the msg.h implementation.
Regards, Fabian Maurer
Any update on this?
Regards, Fabian Maurer
-
Fabian Maurer -
Nikolay Sivov