Reply inline.
On Sun, 8 Jan 2023 at 21:38, Crystal Kolipe <kolip...@exoticsilicon.com>
wrote:
> On Sun, Jan 08, 2023 at 08:55:46PM +0000, Nicholas Marriott wrote:
> > This is still too big for one go so I think it needs split further. I
> > suggest that first we get three diffs with the very easy stuff:
> >
> > 1) The two bug fixes
> >
> > 2) HPA/VPA/INDN/RIN
>
> You didn't mention '3' :-)
>
Yes I saw that after I sent it, I changed my mind :-).
> But anyway, that's basically the first diff I sent and the two one-liners.
>
> I've attached that to the end of this mail.
>
> However, I think it's counter-productive to put just this in without the
> changes to the function keys, because as soon as people start testing the
> patch and setting TERM=xterm, then the first thing they are going to
> complain
> about is that F1 - F4 don't work.
>
Your full diff is 400 lines and makes 5 or 6 different changes in four
files, it cannot go in in one go.
>
> > Some other comments on the larger diff:
> >
> > - Why do you change RS_SCROLLBACK_SCREENS?
>
> To more closely mirror xterm behaviour. The current scrollback buffer is a
> tiny fraction of what xterm offers by default. It's not that important,
> but
> if we're telling people that the console is now like xterm, it's better
> that
> it really is.
>
>
This should be a separate change also.
> > - Wouldn't it be better to make underline match rather than moving bits
> > about at runtime? Is that not possible?
>
> I'm not sure what you mean here.
>
> The original underlining code is unchanged by my diff. I only added double
> underline and strikethough.
>
> Are you talking about the rasops code that actually plots the extra
> underline
> pixels? That's done in the same way as the existing code, so I'm not sure
> what you would prefer done differently?
>
> Or is it the part in wscons where the underline flag is moved to bit 0?
> That
> is a nasty hack that is in the original code. I could take it out, but
> that
> would mean changing all of the bpp specific rasops code and removing the
> magic
> number 1 and replacing it with WSATTR_UNDERLINE.
>
Yes. WSATTR_UNDERLINE is 8 and you move it to bit 0 in rasops_pack_cattr,
so can it not be swapped with WSATTR_REVERSE so it is not necessary to move
it? You are changing rasops to use WSATTR flags in several places, so it
would be nice if it could use WSATTR_UNDERLINE directly instead of 1.
Obviously this should be done as a separate change also and it can wait
until later, there is enough here already.
>
> That sounds like a lot of churn that could potentially introduce new bugs
> and
> would require a lot of testing. I prefer to leave the original hack in,
> because the rasops code has seen little change for many years, and with the
> features that I'm adding I don't think it will need to see much more any
> time
> soon.
>
> > - It seems odd that invisible will still show some elements like the
> > underline. I think unless we can support it fully it would be better not
> to
> > support it at all.
>
> This is the correct behaviour. Xterm does the same thing. Invisible
> characters retain underline, double underline, and strikethrough.
>
Hah, you are right. The terminals I tried do not do this but xterm does.
Invisible is useless in any case but yes if it is supported it should be
the same as xterm.
> It would actually be easier to implement invisible without the extra
> elements,
> but I put in the extra effort to mimick xterm behaviour :).
>
> Oh, by the way, I tested my idea of creating a bold font by over-printing
> the
> glyph one pixel to the right. It looked good, and if we used this approach
> instead of setting a brighter colour for bold, then we could support bold
> in
> conjunction with all 256 colours instead of just the first eight.
>
> --- dev/wscons/wsemul_vt100_subr.c.orig Mon May 25 06:55:49 2020
> +++ dev/wscons/wsemul_vt100_subr.c Sun Jan 8 18:28:04 2023
> @@ -231,7 +231,7 @@
> switch (A3(edp->modif1, edp->modif2, c)) {
> case A3('>', '\0', 'c'): /* DA secondary */
> wsdisplay_emulinput(edp->cbcookie, WSEMUL_VT_ID2,
> - sizeof(WSEMUL_VT_ID2));
> + sizeof(WSEMUL_VT_ID2)-1);
> break;
>
> case A3('\0', '\0', 'J'): /* ED selective erase in display */
> @@ -461,6 +461,9 @@
> edp->ccol -= min(DEF1_ARG(0), edp->ccol);
> edp->flags &= ~VTFL_LASTCHAR;
> break;
> + case 'G': /* HPA */
> + edp->ccol = min(DEF1_ARG(0)-1, edp->ncols-1);
> + break;
> case 'H': /* CUP */
> case 'f': /* HVP */
> if (edp->flags & VTFL_DECOM)
> @@ -502,15 +505,36 @@
> WSEMULOP(rc, edp, &edp->abortstate, erasecols,
> ERASECOLS(NCOLS - n, n, edp->bkgdattr));
> break;
> + case 'S': /* INDN */
> + wsemul_vt100_scrollup (edp,DEF1_ARG(0));
> + break;
> + case 'T': /* RIN */
> + wsemul_vt100_scrolldown (edp,DEF1_ARG(0));
> + break;
> case 'X': /* ECH erase character */
> n = min(DEF1_ARG(0), COLS_LEFT + 1);
> WSEMULOP(rc, edp, &edp->abortstate, erasecols,
> ERASECOLS(edp->ccol, n, edp->bkgdattr));
> break;
> + case 'Z': /* KCBT */
> + if (!edp->ccol)
> + break;
> + if (edp->tabs) {
> + for (n=edp->ccol-1 ; n>0; n--)
> + if (edp->tabs[n])
> + break;
> + } else {
> + n=((edp->ccol - 1) & 8);
> + }
> + edp->ccol=n;
> + break;
> case 'c': /* DA primary */
> if (ARG(0) == 0)
> wsdisplay_emulinput(edp->cbcookie, WSEMUL_VT_ID1,
> - sizeof(WSEMUL_VT_ID1));
> + sizeof(WSEMUL_VT_ID1)-1);
> + break;
> + case 'd': /* VPA */
> + edp->crow = min(DEF1_ARG(0)-1, edp->nrows-1);
> break;
> case 'g': /* TBC */
> if (edp->tabs != NULL)
>
