Re: [AD] 2 glitches in the GUI code

["Elias Pschernig" <eliaspschernig@xxxxxxxxxx>]

> > Seems like the last GUI cleanup left 2 small glitches in the
> > MENU drawing code:
> 
> >From what I understand reading the code of 3.9.32 WIP, the glitch with the 8
> GUI objects was the classical off-by-one: x_start+w == x_end+1. But it's a
> little bit different for d_menu_proc(): it implicitly assumes that the
> border is not part of the dialog.

Neither d_menu_proc nor do_menu nor gui_menu_draw_menu say so though, so it's
natural to assume it handles the border the same way as other GUI objects (and
I don't see why a menu border would be different to a button border anyway).

> > The first is in d_menu_proc, and causes MENUs to be drawn 1
> > pixel off the position given in the DIALOG struct, as well
> > as using wrong values for the maximum width and height.
> 
> Note that the 'width' and 'height' fields of the dialog object are meant to
> be minimum values for d_menu_proc(). But I agree that the actual positions
> are wrong and the dimensions should include the whole object.

That's how I actually found out about this at first (I sent a patch for
allegro._tx telling about these minimum values, but my GUI layout still would
overdraw menus with buttons). After my patch I can mix all DIALOG objects now,
including d_menu_proc, and the ->w and ->h are always valid (provided I pass
dimensions at least as big as the minimum dimension).

> > The second causes all menus to be drawn one pixel too wide,
> > and one pixel too high.
> 
> Because of the 'underlining' bottom and right lines ?

Yes, they were overdrawing neighbouring GUI elements.
The only problematic element left is d_check_proc, which sometimes uses a
too big radius for its circle, overdrawing neighbour buttons. (I'm hoping to
fix this with a new ellipse function which will be able to draw circles with
even diameter.)

> > The attached patch fixes both. Interestingly, grabber already
> > uses the right values, so the patch actually fixes it instead
> > of breaking it. I guess the same applies to all other
> > programs using the GUI but who were not aware of these 2
> > glitches.
> 
> Yes, I think all programs (implicitly) assume the 'include the whole object'
> semantics.
> 
> Your patch looks good to me, except one little thing: try to follow the
> original formatting rules (no spaces in expressions in this case). The rules
> may not be the most sensible ones, but I think we should try to enforce
> consistency in this matter too (private message: Peter, see how fast I
> learnt ;-)

Yes, Peter told me in IRC to properly format my patches.. I even went through
all the modified lines in the .diff file, and fixed the formatting, which
included putting spaces around operators. Oh well, I guess I again have to
reread the formatting rules :)

> > I also made a patch for allegro._tx now to clarify that paragraph in the
> > docs about my changes.
> 
> Ok too, except two minor things:
> - I would say that 'Menus had been' is more appropriate here, but English is
> not my mother tongue so...

Yes, I agree, it should be 'had'.

> - 'until version 4.1.0': the plan is to make no API change in the 4.0.x
> series.

I saw the new api._tx - I didn't consider this patch as an API change though..

> It came to my attention that you have CVS write access, so would you mind to
> commit both patches yourself (mainline only) ?

I can try.. will simply executing 'cvs commit <file>' do the right thing, or
do I need some additional parameters?

> It would also be nice to add a line about these changes in the newly created
> docs/src/api._tx file.

Ok, I will do.

--
Elias Pschernig