On 31/08/2007, Dominik Vogt <[EMAIL PROTECTED]> wrote: > 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.)
OK. I'll do that in future. > Comments: > > 1) Please limit lines to 79 characters. Vim didn't seem to like this, so I'll do it by hand. > 2) There is no NEWS entry for the new featur. I'll add one if it's something you think you'll add. > 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. Ah -- one of the reasons I did this was diagonal shading where borderwidths are small are *very* hard to see. Mind you, the onus is on the user when configuring the style I suppose. I'll change this then. > 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! Heh. > 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) > ^^^^ ^^ Ugh. OK. > 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. Correct. > 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>, OK. When I get back home I'll make all those tiny changes and resubmit to directly. (I can see it being really useful in conjunction with PositionPlacement.) Thanks, -- Thomas Adam
