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) >