----- Doug Torrance <[email protected]> a écrit :
> The RGB panel of the WINGs color panel lists the red, green, and blue values
> as
> base 10 numbers. However, hexadecimal numbers are very common when dealing
> with
> RGB colors. This patch adds two radio buttons at the bottom of the RGB panel
> to allow users to choose their preferred number system.
> ---
> WINGs/wcolorpanel.c | 103
> +++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 85 insertions(+), 18 deletions(-)
>
> diff --git a/WINGs/wcolorpanel.c b/WINGs/wcolorpanel.c
> index 2977702..64ce177 100644
> --- a/WINGs/wcolorpanel.c
> +++ b/WINGs/wcolorpanel.c
> @@ -161,6 +161,9 @@ typedef struct W_ColorPanel {
> WMButton *grayPresetBtn[7];
>
> /* RGB Panel */
> + int rgbState;
> [...]
> +enum {
> + RGBdec,
> + RGBhex
> +};
Hi,
May I suggest something different here? enums are much more that a replacement
for #defines, so you should use something like this in the structure:
+ enum {
+ ...
+ } rgbState;
So the variable will be more than just an int, the compiler can make a number
of consistency checks which are useful.
> [...]
> +void rgbIntToChar(W_ColorPanel *panel, int *value)
> +{
> + char tmp[4];
> + char *format;
the format being a const string, I would suggest 'const char *format'
> +
> + if (panel->rgbState == RGBdec)
> + format = "%d";
> + if (panel->rgbState == RGBhex)
> + format = "%0X";
a switch (panel->rgbState) would probably be better here, amongst other things
because by using an enum instead of an int the compiler will report a missing
case at compile time (if we suppose someday someone adds RGBoct for example)
instead of a strange crash at run time
> [...]
> +int *rgbCharToInt(W_ColorPanel *panel)
> +{
> + static int value[3];
> +
> + if (panel->rgbState == RGBdec) {
> + value[0] = atoi(WMGetTextFieldText(panel->rgbRedT));
> + value[1] = atoi(WMGetTextFieldText(panel->rgbGreenT));
> + value[2] = atoi(WMGetTextFieldText(panel->rgbBlueT));
> + }
> + if (panel->rgbState == RGBhex) {
> + value[0] = strtol(WMGetTextFieldText(panel->rgbRedT), NULL, 16);
> + value[1] = strtol(WMGetTextFieldText(panel->rgbGreenT), NULL,
> 16);
> + value[2] = strtol(WMGetTextFieldText(panel->rgbBlueT), NULL,
> 16);
> + }
Same here, a switch would be better. you could probably also use strtol for
both case, so actually just switching the value for the base which means less
duplicated code.
> [...]
> @@ -2420,6 +2465,28 @@ static void rgbTextFieldCallback(void *observerData,
> WMNotification * notificati
> panel->lastChanged = WMRGBModeColorPanel;
> }
>
> +static void rgbDecToHex(WMWidget *w, void *data)
> +{
> + W_ColorPanel *panel = (W_ColorPanel *) data;
> + (void) w;
> + int *value;
The "(void) w;" is actually a statement, not a declaration, so it should be
after the variable declarations (with a blank line before and after).
> [...]
Regards,
Christophe.
--
To unsubscribe, send mail to [email protected].
