[PATCH 0/3] MR5756: comdlg32: Fix a few issues with color dialog

[PATCH 1/3] comdlg32: Update luminosity bar when changing hue/sat/lum manually

[Fabian Maurer]

From: Fabian Maurer

---
 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;

[PATCH 2/3] comdlg32: Prevent recursion inside CC_CheckDigitsInEdit

[Fabian Maurer]

From: Fabian Maurer

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))
    		   {

[PATCH 3/3] comdlg32: Properly handle out of bounds values

[Fabian Maurer]

From: Fabian Maurer

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;
  }

[Nikolay Sivov (＠nsivov)]

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?

[Fabian Maurer (＠DarkShadow44)]

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.

[Nikolay Sivov (＠nsivov)]

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.

[Fabian Maurer (＠DarkShadow44)]

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.

[Fabian Maurer (＠DarkShadow44)]

Is there anything I should do here?

[Fabian Maurer (＠DarkShadow44)]

@nsivov, anything I should change here?

[Nikolay Sivov (＠nsivov)]

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.

[Fabian Maurer (＠DarkShadow44)]

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

[Nikolay Sivov (＠nsivov)]

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.

[Nikolay Sivov (＠nsivov)]

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.

[PATCH 0/3] MR5756: comdlg32: Fix a few issues with color dialog - approved

[Nikolay Sivov (＠nsivov)]

This merge request was approved by Nikolay Sivov.