On Thu, Aug 30, 2007 at 09:32:15PM +0100, Thomas Adam wrote: > I've trivially written a patch that starts a window Shaded when it's mapped, > rather than relying on FvwmEvent to do this. Whilst there's no problems in > using FvwmEvent for this, the list of window to shade grew and grew. Hence > having it in the core made sense. > > You can find the patch here: > > http://edulinux.homeunix.org/fvwm/patches.html#toc5
(It is easier to comment on patches if you post them directly.) Comments: 1) Please limit lines to 79 characters. 2) There is no NEWS entry for the new featur. 3) > if (direction >= 0 && direction <= DIR_MAJOR_MASK) I see no reason to limit initial shading to n/s/w/e. If you use DIR_MINOR_MASK you get diagonal shading for free. 4) About data types: > + unsigned do_start_shaded : 1; ... > + int start_shaded_dir; This should be + unsigned do_start_shaded : 1; + unsigned start_shaded_dir :3; This way, memory is used more efficiently. Bit fields have to be kept in one place! 5) Please use spaces in if conditions the other way round: > if( SGET_STARTS_SHADED_DIR(style) != DIR_NONE ) Should be if (SGET_STARTS_SHADED_DIR(style) != DIR_NONE) ^^^^ ^^ 6) In the documentation you write that "Last" is a valid value for the initial shade direction. I don't think that's true when a window is created. 7) In the documentation you write > +<emphasis remap='I'>StartsLowered</emphasis> / <emphasis > remap='I'>StartShaded</emphasis> / <emphasis > remap='I'>StartsRaised</emphasis>, all in one line. This is incorrect. Styles appearing in a /-separated list are alternatives settings for the same feature. You have to begin a new line: <emphasis remap='I'>StartShaded</emphasis> / <emphasis remap='I'>!StartShaded</emphasis>, Ciao Dominik ^_^ ^_^ -- Dominik Vogt
signature.asc
Description: Digital signature
