Re: Patch for line draw characters problem & screen scrolling

[Christopher Faylor <cgf at redhat dot com>]

On Wed, May 21, 2003 at 05:32:33PM +0200, Micha Nelissen wrote:
>Hi,
>
>Several problems encountered and tried to fix:
>
>1) line draw characters not showing up in combination Command Prompt with
>bash.
>2) screen scrolling fixed for termcap entry 'cs' -> screen split is very
>fast and cool.
>3) end-of-buffer cursor out of range; see changelog for more details.
>
>This is my first patch, so please don't flame ;). I am open to suggestions.
>
>Regards,

>2003-05-21  Micha Nelissen  <mdvpost@hotmail.com>
>
>* fhandler.h (dev_console): add title state variables.
>* fhandler_console.cc (get_tty_stuff): save old console title.

Why?  This wasn't mentioned in your description of the patch.

>* fhandler_console.cc (macro: srTop, srBottom; char_command): scroll_region is
>not relative to window top.

I'm not 100% sure what you're saying here, but if you set a scroll
region and then send a "goto line 1" escape sequence, I believe that, by
default, you go to actual line one on the screen, not the first line of
the scroll region.  This behavior is controlled by some escape sequence.
rustle, rustle.  <ESC>[?6h and <ESC>[?6l

>* fhandler_console.cc (scroll_screen): renamed variables sr1, sr2 to srScroll,
>srClip respectively which is clearer.

That's a gratuitous change, the bane of patch receivers everywhere.

>* fhandler_console.cc (scroll_screen, char_command, write_normal): more debug
>info.

Your debug output is in a different format than the rest of the debug
output in that file.  Granted, this is flexible, but your patch seems to
be extremely wordy.

>* fhandler_console.cc (write_normal): end of buffer check enables cursor to be
>out of range; it better emulates *nix terminal behaviour; ie. it is now
>possible to write a single character at right bottom of console buffer without
>the console scrolling the buffer.

How is this similar to UNIX?  If I do a:

sleep 5; echo hello

and then scroll my xterm up, xterm scrolls down when hello is printed.  It
sounds like your patch would not cause this to happen.

>* fhandler_console.cc (write_normal): cancelled premature optimization, do
>always use scroll_screen instead of '\n' sometimes

No.  This section of code has been hacked back and forth for years.  This
one is not going to happen.

>* fhandler_write.cc (write): if `user' clears title, then don't add `|'

I must be missing something.  Why is "|" being added to the title at all?

General comments about the rest of the patch:

1) Please break it up into smaller sets for approval.  Large patches like
this which attempt multiple things are difficult to review.

2) There is occasional deviation from GNU coding standards in your patch.
Specifically, there are missing spaces after commas and coercions.

3) You comment out some code rather than deleting it outright.  There's
no reason to leave old code around.

4) As Corinna mentioned, you need to send in an assignment.  The contributing
page has details.

Thanks for all of your hard work on this patch.

cgf