29 Aug
2017
29 Aug
'17
11:06 p.m.
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
30 Aug
30 Aug
6:36 a.m.
New subject: [PATCH 1/2] comctl32/taskdialog: Initial support for callback procedure
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
11:05 a.m.
New subject: [PATCH 1/2] comctl32/taskdialog: Initial support for callback procedure
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
12 Sep
12 Sep
4:28 p.m.
New subject: [PATCH 1/2] comctl32/taskdialog: Initial support for callback procedure
Any update on this?
Regards,
Fabian Maurer
2442
days inactive
2456
days old
3 comments
2 participants
participants (2)
-
Fabian Maurer -
Nikolay Sivov