A fatal bug in separator drawing!

[alpyre]

The latest changes to remove glitches at the bottom of the gadget revealed a very fatal implicit bug about separators.

DrawSeparator() function uses RectFill() calls. There are no problems when the separator is within gadget boundaries. But when smooth scrolling (vertically) there are times they had to be drawn partially at the edges. This is done either by drawing the separator with MUIClipping or drawing into the doublebuffer and blitting the required portion of it to the rastport.

The current implementation (interestingly) uses the clipping method in smooth scrolling (by passing FALSE for doublebuffer parameter of DumpText()).

This has a danger! If the gadget does not have any spacing (maybe some other GUI objects) between the window edges from top or bottom, the coordinates passed to the RectFill() calls can fall out of the window borders and crashing the system.

It seems that the previous coder(s) avoided these cases by coding this condition into DumpText():

MixedFunctions.c (640)
    if(lines-line_nr < 3 || doublebuffer == TRUE)
    {
      doublebuffer = TRUE;
    }   

What this does is; "if we are printing the first line at the top of the gadget, always use doublebuffering".
(and for the bottom, never print anything - which was the couse of the pattern glitches now removed).

This is very bad coding. Because in the current implementation, if the allocation of the doublebuffering rastport fails for a reason, it does NOT prevent the gadet from being created! Instead it fallbacks to non-doublebuffered (MUIClipping) mode (which would eventually crash when a separator was smooth scrolled out of gadget).

This currently causes crashes in YAM's mail preview gadget in the main window, if the e-mail has a separator and the user scrolls it using the scroller because the gadget is adjacent to the window bottom.

[alpyre]

My ideas to resolve this issue are:
1) Smooth scroll always in doublebuffer mode and prevent gadget creation without a doublebuffering rastport.
...or...
2) Put coordinate checks into DrawSeparator(). Never let it draw out of window (or gadget) boundaries.
...OR...
3) We can compromise to this code while it would be a very rare occasion for a such small doublebuffer bitmap not to allocate and always use doublebuffer when smooth scrolling (or add another condition to DumpText() to switch to doublebuffer mode if it is printing the last line at the bottom - as it does for the top).

What would you suggest on this issue please?
@jens-maus @tboeckel

[alpyre]

Thinking through the weekend I now think the safest way to go is ideas 2 plus 3.
Doublebuffering bitmap is allocated in mShow() and deallocated in mHide() which are the best places to do them because we will have _mwidth() there.

[tboeckel]

Better do this kind of stuff in MUIM_Setup/Cleanup instead of MUIM_Show/Hide. The latter will be called upon every window resize action, while the former is called when the window is opened/closed only.

[alpyre]

Yes, you're right. But actually it resizes the doublebuffering bitmap to the new size of the window. So I think it is smarter to keep it in Show/Hide.
Btw. I've made the changes to fix the bug already. We are safe now.

Now I want to ask something implementationally related to this.

What do you think about the space at the very bottom of the gadget? Should we make it print a partially visible line there?

For reference, in Windows' NotePad, it does not, but in Mousepad or Leafpad in Linux, it does.

What are your ideas on this?

[jens-maus]

What exactly do you mean with "the space at the very bottom of the gadget". Can you please provide a detailed screenshot to outline what you exactly mean?

[alpyre]

The space at the bottom when the gadget is sized to a height not proportional to the fontheight.

[jens-maus]

Thanks for the screenshot. Of course I would vote for showing partial lines there as well as that would enable more smooth scrolling and use as much space as possible. And it would give people a hint that there is more to come.

[alpyre]

I totally agree.

If we all agree on this behaviour, then I'm implementing the bug removal according to this and a pullrequest can happen within an hour.

@tboeckel

[alpyre]

I'm sorry guys. It turned out that I needed to change more than I expected in PrintLine(). But I'm pretty sure at most tomorrow I'll get it working. :)