[PATCH 0/3] MR5756: comdlg32: Fix a few issues with color dialog
From: Fabian Maurer <dark.shadow4(a)web.de> --- dlls/comdlg32/colordlg.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/comdlg32/colordlg.c b/dlls/comdlg32/colordlg.c index e16014c8d19..84540a1348f 100644 --- a/dlls/comdlg32/colordlg.c +++ b/dlls/comdlg32/colordlg.c @@ -1011,6 +1011,7 @@ static LRESULT CC_WMCommand(CCPRIV *lpp, WPARAM wParam, LPARAM lParam, WORD noti CC_EditSetRGB(lpp); CC_PaintCross(lpp); CC_PaintTriangle(lpp); + CC_PaintLumBar(lpp); } } break; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5756
From: Fabian Maurer <dark.shadow4(a)web.de> Since we set the text, this would trigger the exact same logic again. --- dlls/comdlg32/colordlg.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/dlls/comdlg32/colordlg.c b/dlls/comdlg32/colordlg.c index 84540a1348f..4a5da107bd8 100644 --- a/dlls/comdlg32/colordlg.c +++ b/dlls/comdlg32/colordlg.c @@ -393,7 +393,7 @@ static BOOL CC_MouseCheckResultWindow( HWND hDlg, LPARAM lParam ) /*********************************************************************** * CC_CheckDigitsInEdit [internal] */ -static int CC_CheckDigitsInEdit( HWND hwnd, int maxval ) +static int CC_CheckDigitsInEdit( CCPRIV *infoPtr, HWND hwnd, int maxval ) { int i, k, m, result, value; char buffer[30]; @@ -423,8 +423,10 @@ static int CC_CheckDigitsInEdit( HWND hwnd, int maxval ) if (result) { LRESULT editpos = SendMessageA(hwnd, EM_GETSEL, 0, 0); + infoPtr->updating = TRUE; SetWindowTextA(hwnd, buffer ); SendMessageA(hwnd, EM_SETSEL, 0, editpos); + infoPtr->updating = FALSE; } return value; } @@ -966,10 +968,10 @@ static LRESULT CC_WMCommand(CCPRIV *lpp, WPARAM wParam, LPARAM lParam, WORD noti case COLOR_BLUE: if (notifyCode == EN_UPDATE && !lpp->updating) { - i = CC_CheckDigitsInEdit(hwndCtl, 255); + i = CC_CheckDigitsInEdit(lpp, hwndCtl, 255); r = GetRValue(lpp->lpcc->rgbResult); g = GetGValue(lpp->lpcc->rgbResult); - b= GetBValue(lpp->lpcc->rgbResult); + b = GetBValue(lpp->lpcc->rgbResult); xx = 0; switch (LOWORD(wParam)) { @@ -996,7 +998,7 @@ static LRESULT CC_WMCommand(CCPRIV *lpp, WPARAM wParam, LPARAM lParam, WORD noti case COLOR_LUM: if (notifyCode == EN_UPDATE && !lpp->updating) { - i = CC_CheckDigitsInEdit(hwndCtl , LOWORD(wParam) == COLOR_HUE ? 239 : 240); + i = CC_CheckDigitsInEdit(lpp, hwndCtl, LOWORD(wParam) == COLOR_HUE ? 239 : 240); xx = 0; switch (LOWORD(wParam)) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5756
From: Fabian Maurer <dark.shadow4(a)web.de> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56102 --- dlls/comdlg32/colordlg.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/comdlg32/colordlg.c b/dlls/comdlg32/colordlg.c index 4a5da107bd8..0093b455b1b 100644 --- a/dlls/comdlg32/colordlg.c +++ b/dlls/comdlg32/colordlg.c @@ -417,6 +417,7 @@ static int CC_CheckDigitsInEdit( CCPRIV *infoPtr, HWND hwnd, int maxval ) value = atoi(buffer); if (value > maxval) /* build a new string */ { + value = maxval; sprintf(buffer, "%d", maxval); result = 2; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5756
Nikolay Sivov (@nsivov) commented about dlls/comdlg32/colordlg.c:
if (result) { LRESULT editpos = SendMessageA(hwnd, EM_GETSEL, 0, 0); + infoPtr->updating = TRUE; SetWindowTextA(hwnd, buffer ); SendMessageA(hwnd, EM_SETSEL, 0, editpos); + infoPtr->updating = FALSE; } This does not look good. If we force content to something valid, why would it trigger same logic again?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5756#note_71913
On Fri May 31 11:51:52 2024 +0000, Nikolay Sivov wrote:
This does not look good. If we force content to something valid, why would it trigger same logic again? Because everytime the text in that edit control gets modified, it triggers that logic.
The same thing is done in `CC_EditSetRGB` and `CC_EditSetHSL`, but it was missing here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5756#note_71955
On Fri May 31 18:28:10 2024 +0000, Fabian Maurer wrote:
Because everytime the text in that edit control gets modified, it triggers that logic. The same thing is done in `CC_EditSetRGB` and `CC_EditSetHSL`, but it was missing here. We modify it when clamping. Why would that cause another text change? My understanding is that you enter something -> change notification -> check -> limit -> change notification -> check -> no need to limit.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5756#note_71958
On Fri May 31 19:16:59 2024 +0000, Nikolay Sivov wrote:
We modify it when clamping. Why would that cause another text change? My understanding is that you enter something -> change notification -> check -> limit -> change notification -> check -> no need to limit. We call `SetWindowTextA` which causes a text change. This text change triggers another change notification and leads to `CC_CheckDigitsInEdit` being called again. The entire logic runs twice, which it shouldn't. That's why we have `if (notifyCode == EN_UPDATE && !lpp->updating)` and other functions (like `CC_EditSetHSL`) set `updating` to true.
We should do the same here, so only the text is set and the change notification is ignored. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5756#note_71959
Is there anything I should do here? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5756#note_77833
@nsivov, anything I should change here? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5756#note_80638
On Sun Sep 1 22:31:02 2024 +0000, Fabian Maurer wrote:
@nsivov, anything I should change here? I see now that "updating" field is likely unavoidable, because EN_UPDATE/EN_CHANGE does not actually indicate an update or a change to contents. How do I test your changes? Maybe you have a small interactive test program.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5756#note_80640
On Sun Sep 1 22:31:02 2024 +0000, Nikolay Sivov wrote:
I see now that "updating" field is likely unavoidable, because EN_UPDATE/EN_CHANGE does not actually indicate an update or a change to contents. How do I test your changes? Maybe you have a small interactive test program.
/* i686-w64-mingw32-gcc main.c -l comdlg32 */
#include <windows.h>
#include <commdlg.h>
int main() {
CHOOSECOLOR cc = {0};
cc.lStructSize = sizeof(cc);
cc.Flags = CC_FULLOPEN;
ChooseColor(&cc);
return 0;
}
This is the shortest test program. - When changing the text for Hue, the right bar is not updated properly - When changing Hue/Sat to 999, the bar gets very weird -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5756#note_80641
On Sun Sep 1 22:59:42 2024 +0000, Fabian Maurer wrote:
``` /* i686-w64-mingw32-gcc main.c -l comdlg32 */ #include <windows.h> #include <commdlg.h> int main() { CHOOSECOLOR cc = {0}; cc.lStructSize = sizeof(cc); cc.Flags = CC_FULLOPEN; ChooseColor(&cc); return 0; } ``` This is the shortest test program. - When changing the text for Hue, the right bar is not updated properly - When changing Hue/Sat to 999, the bar gets very weird
Such test exe crashes for me on Windows. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5756#note_80644
On Mon Sep 2 01:21:54 2024 +0000, Nikolay Sivov wrote:
Such test exe crashes for me on Windows. On Windows it crashes on null lpCustColors field.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5756#note_80702
This merge request was approved by Nikolay Sivov. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5756
participants (3)
-
Fabian Maurer -
Fabian Maurer (＠DarkShadow44)
-
Nikolay Sivov (＠nsivov)