Subject: Re: Description and patches for proposed new feature

From: Dominik Vogt <[email-masked]>
Date: Fri, 19 Mar 2004 11:06:16 +0100

On Wed, Jan 14, 2004 at 12:44:00PM -0500, Chris Ross wrote:
> [email-masked] wrote; in a message with the
> subject "Re: How do I redraw menuitems in an active menu?":
> >Why don't you post your patch so that I can take a look (include
> >instructions to show the problem).
> 
>   Well, as it happens, I figured out the problem with
> modifying the menu item.  It was just an issue of
> the code not clearing the menuitem window area before
> drawing a substring.  I got that fixed now, so it clears
> the whole area before drawing in the case of the new
> feature I added.
> 
>   Okay, included here are a description and patches for
> the new keyword I added.  This keyword will allow me
> to define menuitems that change based on modifier
> key-presses.  The patch is based on the HEAD of the
> CVS tree as of today (14-Jan-2004).
> 
>   Much feedback welcome from all!  Please, let me know
> any thoughts you have!  Thanks!

I have just started looking at the patch (it will take some time).
A few comments on the way the code is written:

[snip]

> +				/* XXX - do we need to make sure to account */
                                   ^^^

If you mark the places where work has to be done with !!! I always
find them (not important).

> +    fprintf(stderr, "Modifiers active: ");

This is okay for debugging, but if you want to print an fvwm
message, use fvwm_msg(INFO, ...).  Also, it would be a good thing
to mark debug code (Personally, I do not indent debug statements
and prepend them with /*!!!*/).

> +	    //fprintf(stderr, "action is '%s'\n", action);
            ^^

No C++ comments please.  We have to cope with th 89 ANSI C
standard too.

> +	    if (modifiers_string_to_modmask(modspec, &modmask)) {
> +		fvwm_msg(WARN, "__mloop_handle_event",
> +			 "Illegal modifier string '%s'", modspec);
                          ^^^^^^^

> +		fvwm_msg(WARN, "__mloop_handle_event",
> +			 "Illegal ModSwitch value '%s'", switchcmd);
                          ^^^^^^^

Better: Invalid

Ciao

Dominik ^_^  ^_^
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]