Re: lynx-dev When unhighlighting some highlighted links, lynx draws them incorrectly

[Vlad Harchev]

On Tue, 16 Mar 1999, Klaus Weide wrote:

> On Sat, 13 Mar 1999, Vlad Harchev wrote:
> > On Fri, 5 Mar 1999, Vlad Harchev wrote:
> > 
> > > 
> > >   When unhighlighting highlighted links that contain some extra 
> > > formatting,
> > >   lynx-2.8.2dev12 and lynx-2.8 draw them incorrectly ( compared to 
> > >   the way the were  drawn before becoming highlighted first time)  when 
> > >   compiled with lss support [...]
> 
> >   Here is a patch that will fix this unhighlighting bug in lynx-dev19
> > compiled with lss support. Original behaviour can be restored by defining
> > NO_HILIT_FIX.
> 
> > diff -ruP 2.8.2dev19-orig/src/GridText.c 
> > lynx-2.8.2dev19-fixed/src/GridText.c
> > --- lynx-2.8.2dev19-orig/src/GridText.c     Tue Mar  9 22:45:03 1999
> > +++ lynx-2.8.2dev19-fixed/src/GridText.c    Sat Mar 13 05:17:34 1999
> > @@ -10278,3 +10278,234 @@
> > +/*
> > + this function draws the part of line 'line', pointed by 'str' (which can 
> > be 
> > + non terminated with null - ie be line->data+N ) drawing 
> > + 'len' bytes (not characters) of it. 
> 
> I thought data always *is* terminated with null - is that not true?
>
  line->data seems to be always terminated with null, but the function
  assumes that 
   strlen(line->data) >= len, and that str can be contained in the range
   [ line->data, line->data + strlen(line->data) ) ie if callee passes the 
  line->data+N as argument for parameter 'str', there needn't be '\0' at
  line->data+N+len - I've considered that this requires less tweaking the 
  contents of line->data. 
  
> > + It doesn't check whether the 'len' 
> > + bytes cross a character boundary (if multibyte chars are in string). 
> > + Assumes that the cursor is positioned in the place where the 1st char of 
> > + string should be drawn. [...]
> > + This code is based on display_line. [...]
> > +*/
> > +PRIVATE void redraw_part_of_line ARGS4( 
> > +        HTLine *,  line,
> > +        char*,          str,
> > +        int,            len,
> > +   HText *,        text)
> > +{
> ...........
> > +    /* this assumes that the part of line to be drawn fits in the screen*/
> > +    while (  data < end_of_data ) {
> > +        buffer[0] = *data;
> > +   data++;
> > +
> > +#if defined(USE_COLOR_STYLE) || defined(SLSC)
> > +#define CStyle line->styles[current_style]
> > +
> > +   while (current_style < line->numstyles &&
> > +          i >= (int) (CStyle.horizpos + line->offset + 1))
> > +   {
> > +           LynxChangeStyle (CStyle.style,CStyle.direction,CStyle.previous);

        Now I think that it should be:
  LynxChangeStyle (CStyle.style,ABS_ON,CStyle.previous);
        see few lines below,
> > +           current_style++;
> > +   }
> > +#endif
> 
> I suspect that this can create the kind of problems my patch is
> trying to resolve - LynxChangeStyle uses a "stack" of styles, which
> can get messed up if it's used for non-contiguous sections of text
> where style changes are not properly nested...  I haven't tested
> it though.  We can try to use your method and see whether it creates
> problems.

     Yes, now I've realized that it can. I see a clean solution:
  (it's placed 14 lines above :) - this won't stack the attributes, so no
  attribute stack overflow will happen. It also can be optimized -
  don't set the attrs before we have found that one we a needed - so
  LynxChangeStyle(..) can be moved out of the loop (NOTE: this idea wasn't
  tested by me, but I believe that it's correct)
    

> (These would be the kinds of problems you don't see if your lss file
> is simple or if you only look at "simple" documents; they may ony
> occur when the anchor is on more than one line, or with some more
> unusual conditions added...)

   BTW, I've posted my lss file in message with subject 
  'lynx-dev suggestions' dated dated (according to the 'Date' field)
  5 March 1999. I've tested the patch I've sent on the file with  
  hyperlinks with 2-line anchors with multiplie attributes in anchor text.

> Also, on the first iteration  LynxChangeStyle is called for all style
> changes on the current line that should come *before* the current
> position.  That doesn't look like a good thing, although maybe it
> happens to have no visible bad effect.
> 
> > +   switch (buffer[0]) {
> > +
> ...........
> > +   } /* end of switch */
> > +    } /* end of while */
> > +
> > +#ifndef USE_COLOR_STYLE
> > +    stop_underline();
> > +    stop_bold();
> > +#else
> > +    while (current_style < line->numstyles)
> > +    {
> > +   LynxChangeStyle (CStyle.style, CStyle.direction, CStyle.previous);
> > +   current_style++;
> > +    }
> 
> Here again, doing LynxChangeStyle for all style changes *after* the anchor
> may not be a good thing.

   Yes, this should be removed.

> > +#undef CStyle
> > +#endif
> > +    return;
> > +}
> ......
> 
> > +PUBLIC void redraw_lines_of_link ARGS1( 
> > +            int , cur )
> > +{
> > +#if defined(USE_COLOR_STYLE) && !defined(NO_HILIT_FIX)        
> > +#define pvtTITLE_HEIGHT 1
> > +    HTLine* todr1,*todr2;
> > +    int lines_back;                    
> > +    if (HTMainText->next_line == HTMainText->last_line) {
> 
> Somehow I never realized that there was a HTMainText->next_line
> that can be used for stuff like this...
> 
> I'm not sure it will always be set correctly; there may be a border
> case you missed when (HTMainText->next_line == HTMainText->last_line)
> is true but 
> 
> > +    /* we are at the last page - that is partially filled */
> 
> is not true.
  
 That comment should be read as /* we are at the last page */ - it doesn't
 matter whether it's fully or partially filled. What do you mean by
 'border case' ? Till now I belived that 
    HTMainText->next_line == HTMainText->last_line
 will always be true for last page (if member 'state' or 'stale' is false
 as you say). Am I wrong?

> > +       lines_back = HTMainText->Lines - ( links[cur].ly-pvtTITLE_HEIGHT+
> > +       HTMainText->top_of_screen);
> > +    } else  {               
> > +       lines_back=display_lines - (links[cur].ly-pvtTITLE_HEIGHT);
> > +    };
> > +    todr1=HTMainText->next_line;
> > +    while (lines_back--) todr1=todr1->prev;
> 
> A cleaner / clearer way for this (finding next or last line, then going
> backwards) would be to use HTMainText->top_of_screen_line.  It seems
> to be meant as a pointer to the first line on the screen (see comments
> in GridText.c before 'struct _HText {'), but is currently never used
> or set; but setting it the right way in display_page should be a
> trivial change.
  
  Yes, it will be cleaner/clearer.

> By the way, in the GridText.c comment:
> 
>     /*      Notes on struct _Htext:
>     **      next_line is valid if state is false.
> 
> I suspect that "state" is an ancient typo for "stale".
> 
> 
>     Klaus
> 

 
 My thoughts on the lss patch you've sent:
 
 Our patches are quite orthogonal - your patch doesn't fix problems my
 patch supposed to fix (but adds a lof of other nice modifications):
   The code of my patch dumbly draws the link anchor the way it should be
 drawn on ^L. The part of your patch that modifies 'highlight'
 fixes only setting attribute of the 1st character of the
 anchor. If the anchor was drawn using more than one attribute before
 highlighting, it won't be drawn using that number of colors during
 unhighlighting but only one attribute. Here is an example (I've tested it
 with sample.lss file modified by your patch):

 <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
 <HTML><HEAD><TITLE>e</TITLE></HEAD>
 <BODY LANG="EN">
 <UL> 
 <LI> <A NAME="tex2html37" HREF="node24.html">IPC Keys</A>
 <LI> <A NAME="tex2html38" HREF="node25.html">The <TT>ipcs</TT> Command</A>
 </UL></BODY></HTML>
 
 Try selecting/unselecting last link with either of patches applied.
     About source highlighting:
 IMO it looks fancy, but confusing. I think that it should be done
 as in Netscape (View->Page Source) - the angle brackets have one color,
 tag names and params are of 2nd color, values of tag params are in
 of 3rd color, and the text of 4th. This is really syntax highlighting. 
 I'm thinking on generating html file  that will represent the source of 
 the file being viewed, probably with all embedded hrefs to be activable 
 links, and HTML formatting that will decorate the source's lexems in the
 way similar to Netscape. I think that it can be relatively easy implemented
 in some scripting language or even  as 'sed' ruleset...

   Such approach has the following advantages:
 [main for those that don't use lss] Colorized page source can be viewed  
    not only when lss support is compiled in.    
   [*]   IMO it will be more visually attractive.
   [*]   Highlighting rules can be tuned by advanced users
   [*]   I'm ready to implement this as 'sed' script.
        
 Disadvantages:
   [*]   Not all OSes have scripting languages installed or even available. 
        

 No more thoughts yet :) 

 I'm ready to futher correct patch that I've sent.


 Best regards,
 -Vlad