Alex Henrie alexhenrie24@gmail.com writes:
Technically, all the stuff about WM_SYSKEYDOWN is unnecessary. All we really need to do is fix the error handling, or remove this questionable optimization altogether.
I'm not sure what you mean by that. Also you are essentially reverting d75b0cdc986f464f3067767cd53aca6b85feb366, I'd like to hear some justification for that.
2016-11-09 12:26 GMT-07:00 Alexandre Julliard julliard@winehq.org:
Alex Henrie alexhenrie24@gmail.com writes:
Technically, all the stuff about WM_SYSKEYDOWN is unnecessary. All we really need to do is fix the error handling, or remove this questionable optimization altogether.
I'm not sure what you mean by that. Also you are essentially reverting d75b0cdc986f464f3067767cd53aca6b85feb366, I'd like to hear some justification for that.
The goal is to fix https://bugs.winehq.org/show_bug.cgi?id=41508
Currently, when MENU_TrackMenu gets WM_SYSKEYDOWN with wParam=VK_LEFT, it calls MENU_KeyLeft, which calls MENU_SuspendPopup. The problem is that MENU_SuspendPopup does not remove the message, and it sets TF_SKIPREMOVE so that MENU_TrackMenu does not remove it either. So, the message stays in the queue and MENU_TrackMenu is called all over again.
Instead of adding WM_SYSKEYDOWN to the switch statement, we could fix the bug in one line by moving "pmt->trackFlags |= TF_SKIPREMOVE;" into the WM_KEYDOWN case. Or, we could delete MENU_SuspendPopup entirely and the if statements that call it. However, we wouldn't be setting TF_SKIPREMOVE anymore for unrecognized messages, likely causing a return of the bug that d75b0cdc tried to fix.
By the way, I'm not exactly reverting d75b0cdc. I changed PM_REMOVE to PM_NOREMOVE to make sure that messages are removed only if they are recognized.
I hope that makes sense; I admit that my previous explanation was not as well thought out as it could have been. Thanks for the feedback.
-Alex
Alex Henrie alexhenrie24@gmail.com writes:
Instead of adding WM_SYSKEYDOWN to the switch statement, we could fix the bug in one line by moving "pmt->trackFlags |= TF_SKIPREMOVE;" into the WM_KEYDOWN case. Or, we could delete MENU_SuspendPopup entirely and the if statements that call it. However, we wouldn't be setting TF_SKIPREMOVE anymore for unrecognized messages, likely causing a return of the bug that d75b0cdc tried to fix.
By the way, I'm not exactly reverting d75b0cdc. I changed PM_REMOVE to PM_NOREMOVE to make sure that messages are removed only if they are recognized.
I hope that makes sense; I admit that my previous explanation was not as well thought out as it could have been. Thanks for the feedback.
Sorry for the delay in reviewing this. It still doesn't quite make sense to me. Changing to PM_NOREMOVE now means that the keyboard message won't get removed if some other message happened in between. I'm not sure that's a good idea.
2016-12-02 11:41 GMT-07:00 Alexandre Julliard julliard@winehq.org:
Alex Henrie alexhenrie24@gmail.com writes:
Instead of adding WM_SYSKEYDOWN to the switch statement, we could fix the bug in one line by moving "pmt->trackFlags |= TF_SKIPREMOVE;" into the WM_KEYDOWN case. Or, we could delete MENU_SuspendPopup entirely and the if statements that call it. However, we wouldn't be setting TF_SKIPREMOVE anymore for unrecognized messages, likely causing a return of the bug that d75b0cdc tried to fix.
By the way, I'm not exactly reverting d75b0cdc. I changed PM_REMOVE to PM_NOREMOVE to make sure that messages are removed only if they are recognized.
I hope that makes sense; I admit that my previous explanation was not as well thought out as it could have been. Thanks for the feedback.
Sorry for the delay in reviewing this. It still doesn't quite make sense to me. Changing to PM_NOREMOVE now means that the keyboard message won't get removed if some other message happened in between. I'm not sure that's a good idea.
We definitely do not want to remove the keyboard message if some other message is in line before it. In fact, the PeekMessageW( &msg, 0, 0, 0, PM_NOYIELD | PM_NOREMOVE ) statements later in the function depend on the assumption that they are reading a message that follows the WM_KEYDOWN. They are not supposed to be looking at a message that precedes the WM_KEYDOWN like is possible in the current code.
I made the function behave the way that it used to before d75b0cdc, while being careful to not reintroduce the bug where non-keyboard messages were accidentally removed.
-Alex
Alex Henrie alexhenrie24@gmail.com writes:
2016-12-02 11:41 GMT-07:00 Alexandre Julliard julliard@winehq.org:
Alex Henrie alexhenrie24@gmail.com writes:
Instead of adding WM_SYSKEYDOWN to the switch statement, we could fix the bug in one line by moving "pmt->trackFlags |= TF_SKIPREMOVE;" into the WM_KEYDOWN case. Or, we could delete MENU_SuspendPopup entirely and the if statements that call it. However, we wouldn't be setting TF_SKIPREMOVE anymore for unrecognized messages, likely causing a return of the bug that d75b0cdc tried to fix.
By the way, I'm not exactly reverting d75b0cdc. I changed PM_REMOVE to PM_NOREMOVE to make sure that messages are removed only if they are recognized.
I hope that makes sense; I admit that my previous explanation was not as well thought out as it could have been. Thanks for the feedback.
Sorry for the delay in reviewing this. It still doesn't quite make sense to me. Changing to PM_NOREMOVE now means that the keyboard message won't get removed if some other message happened in between. I'm not sure that's a good idea.
We definitely do not want to remove the keyboard message if some other message is in line before it. In fact, the PeekMessageW( &msg, 0, 0, 0, PM_NOYIELD | PM_NOREMOVE ) statements later in the function depend on the assumption that they are reading a message that follows the WM_KEYDOWN. They are not supposed to be looking at a message that precedes the WM_KEYDOWN like is possible in the current code.
At that point the keyboard message has already been handled, for instance by moving selection to the next item. So if you don't remove it, it will be handled twice.
2016-12-05 3:42 GMT-07:00 Alexandre Julliard julliard@winehq.org:
Alex Henrie alexhenrie24@gmail.com writes:
2016-12-02 11:41 GMT-07:00 Alexandre Julliard julliard@winehq.org:
Alex Henrie alexhenrie24@gmail.com writes:
Instead of adding WM_SYSKEYDOWN to the switch statement, we could fix the bug in one line by moving "pmt->trackFlags |= TF_SKIPREMOVE;" into the WM_KEYDOWN case. Or, we could delete MENU_SuspendPopup entirely and the if statements that call it. However, we wouldn't be setting TF_SKIPREMOVE anymore for unrecognized messages, likely causing a return of the bug that d75b0cdc tried to fix.
By the way, I'm not exactly reverting d75b0cdc. I changed PM_REMOVE to PM_NOREMOVE to make sure that messages are removed only if they are recognized.
I hope that makes sense; I admit that my previous explanation was not as well thought out as it could have been. Thanks for the feedback.
Sorry for the delay in reviewing this. It still doesn't quite make sense to me. Changing to PM_NOREMOVE now means that the keyboard message won't get removed if some other message happened in between. I'm not sure that's a good idea.
We definitely do not want to remove the keyboard message if some other message is in line before it. In fact, the PeekMessageW( &msg, 0, 0, 0, PM_NOYIELD | PM_NOREMOVE ) statements later in the function depend on the assumption that they are reading a message that follows the WM_KEYDOWN. They are not supposed to be looking at a message that precedes the WM_KEYDOWN like is possible in the current code.
At that point the keyboard message has already been handled, for instance by moving selection to the next item. So if you don't remove it, it will be handled twice.
Good point. I'll try again.
-Alex
-
Alex Henrie -
Alexandre Julliard