Re: [v3 04/12] comctl32: Extend TaskDialog, add simple callback and tests
On 10.03.2017 21:21, Fabian Maurer wrote:
v3: Rewrite to implement Nikolay Sivov's suggestions
Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/comctl32/taskdialog.c | 29 +++++++++++++++++++++++-- dlls/comctl32/tests/taskdialog.c | 46 +++++++++++++++++++++++++++++++++------- 2 files changed, 65 insertions(+), 10 deletions(-)
diff --git a/dlls/comctl32/taskdialog.c b/dlls/comctl32/taskdialog.c index 2166ce1898..c5f410716e 100644 --- a/dlls/comctl32/taskdialog.c +++ b/dlls/comctl32/taskdialog.c @@ -147,17 +147,41 @@ static void controls_add(struct list *controls, WORD id, const WCHAR *class, con list_add_tail(controls, &data->entry); }
+/* FIXME: Make thread safe */ +static const TASKDIALOGCONFIG *task_config = 0; +static HRESULT callback(HWND hwnd, UINT uNotification, WPARAM wParam, LPARAM lParam) +{ + if(task_config->pfCallback) + return task_config->pfCallback(hwnd, uNotification, wParam, lParam, task_config->lpCallbackData); + return S_OK; +} +
There's no reason to introduce global variable for that, just to remove it two patches after.
static INT_PTR CALLBACK DialogProc(HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lParam) { + HRESULT ret_callback; + switch (uMsg) { + case WM_INITDIALOG: + callback(hwndDlg, TDN_DIALOG_CONSTRUCTED, 0, 0); + callback(hwndDlg, TDN_CREATED, 0, 0); + return TRUE; case WM_COMMAND: - if(HIWORD(wParam) == BN_CLICKED && LOWORD(wParam) == IDOK) + if(HIWORD(wParam) == BN_CLICKED) { - EndDialog(hwndDlg, 0); + WORD command_id = LOWORD(wParam); + + ret_callback = callback(hwndDlg, TDN_BUTTON_CLICKED, command_id, 0); + if(ret_callback == S_OK) /* FIXME */ + { + EndDialog(hwndDlg, command_id); + } return TRUE; } break; + case WM_DESTROY: + callback(hwndDlg, TDN_DESTROYED, 0, 0); + break; } return FALSE;
For minimal working replacement of MessageBox callback support is not needed, right?
There's no reason to introduce global variable for that, just to remove it two patches after. It's not split the functionality into two patches. I could merge them if you want, but I thought it would be easier to understand if I separated those two logical changes. First getting it to work with a global variable, then change it into something thread-safe.
For minimal working replacement of MessageBox callback support is not needed, right? No it's not needed. But, we certainly want the tests, don't we? Without callback, there'll be no tests.
Regards, Fabian Maurer
On 11.03.2017 1:14, Fabian Maurer wrote:
There's no reason to introduce global variable for that, just to remove it two patches after. It's not split the functionality into two patches. I could merge them if you want, but I thought it would be easier to understand if I separated those two logical changes. First getting it to work with a global variable, then change it into something thread-safe.
Yes, I think it's better to do it in a clean way from the start, it doesn't add additional complexity anyway, only SetProp/GetProp.
For minimal working replacement of MessageBox callback support is not needed, right? No it's not needed. But, we certainly want the tests, don't we? Without callback, there'll be no tests.
We can have tests that have todo_wine's all over, until callbacks are supported, it's not a problem.
Regards, Fabian Maurer
On 11.03.2017 1:14, Fabian Maurer wrote:
There's no reason to introduce global variable for that, just to remove it two patches after.
It's not split the functionality into two patches. I could merge them if you want, but I thought it would be easier to understand if I separated those two logical changes. First getting it to work with a global variable, then change it into something thread-safe.
Yes, I think it's better to do it in a clean way from the start, it doesn't add additional complexity anyway, only SetProp/GetProp. I understand. I just thought it would be useful that way, but I'll change it.
For minimal working replacement of MessageBox callback support is not needed, right?
No it's not needed. But, we certainly want the tests, don't we? Without callback, there'll be no tests.
We can have tests that have todo_wine's all over, until callbacks are supported, it's not a problem. Well if we're going that route, why not leave the tests out alltogether and add then once the callbacks are implemented? There isn't a real benefit to add them now if they can't test wine yet.
Regards, Fabian Maurer
On 12.03.2017 22:39, Fabian Maurer wrote:
On 11.03.2017 1:14, Fabian Maurer wrote:
There's no reason to introduce global variable for that, just to remove it two patches after.
It's not split the functionality into two patches. I could merge them if you want, but I thought it would be easier to understand if I separated those two logical changes. First getting it to work with a global variable, then change it into something thread-safe.
Yes, I think it's better to do it in a clean way from the start, it doesn't add additional complexity anyway, only SetProp/GetProp. I understand. I just thought it would be useful that way, but I'll change it.
For minimal working replacement of MessageBox callback support is not needed, right?
No it's not needed. But, we certainly want the tests, don't we? Without callback, there'll be no tests.
We can have tests that have todo_wine's all over, until callbacks are supported, it's not a problem. Well if we're going that route, why not leave the tests out alltogether and add then once the callbacks are implemented? There isn't a real benefit to add them now if they can't test wine yet.
That works too. Just make sure to run them locally, so you don't have to fix things there were just added.
Regards, Fabian Maurer
participants (2)
-
Fabian Maurer -
Nikolay Sivov