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

[Klaus Weide]

On Thu, 18 Mar 1999, Vlad Harchev wrote:

> On Wed, 17 Mar 1999, Klaus Weide wrote:
> > [...]
> > Also all bets are off when the fixed limit of MAX_STYLES_ON_LINE is
> > exceeded (not tested).
>   
>    IMO, we can increase the value of MAX_STYLES_ON_LINE.

It's a tradeoff - the styles for each line are already wasting a lot of
memory (think of a 1 MB plain text files with most short lines, since it's
plain it doesn't even use styles).  I wouldn't want to increase the default
value - haven't seen a problem that I could attribute to MAX_STYLES_ON_LINE
being too small (although I haven't looked hard, and I'm sure one can
construct test cases where this is a problem).

> > However, since those are unusual circumstances, we may just use your
> > method anyway, and later fix the glitches (or learn to live with them).
> 
>   Yes, IMO better anything than nothing. 
> 
> >    ....
> >    <A ...>anchor text 1</A><A ...>anchor text 2</A> <B>more stuff</B>
> >    ...                                               ^ last ON change on 
> > line
> > 
> > (The B STACK_ON change would be saved at the position where the second
> > anchor starts when the first anchor is unselected - untested)
> 
>   There is a simple and nasty solution - we can create a new function from
>  a curses_w_style, that won't do caching, and use it in
>  draw_part_of_line.

Or add another parameter use_cached_styles to the existing function.
LynxChangeStyle can be re-defined to always set the new parameter to YES,
not many changes needed.

> > > I've composed an html of your examples [ ... ]   - it's OK.
> > 
> > Good. :)   There are still unusual situation where it wouldn't
> > be the case, but we could ignore them for now if you wish.
> 
>   IMO, problems in those unusual situations will arise not due to my
> patch, but due to bugs in other functions that deal with lss. IMO it's
> better to rely on some buggy/untested code of other functions (problems with
> which are so seldom visually detected) than don't fix apparent
> problems. 

I don't really understand the alternatives you are posing here. :)

> Anyway, lss support is still considered experimental so we are 
> free to experiment with it :)

Right.

> >   <A HREF="."><IMG ALT="" SRC=".">Some text</A>
> > and
> >   <A HREF="."><IMG SRC="."> Some text</A>
> > 
> > (The first kind may have to be nested in some other element that ends on the
> > same line, in order to see the effect.)
> > 
> > Without your patch[*], unhighlighting uses the IMG style for the whole link
> > text (or for the part that is on the first line, if the anchor is split
> > across lines).
>  
>  IMO (as I understand) my patch is not guilty :) It sets the same
> attributes as those ones will be set on ^L - the problem may be in the
> parser(s) IMO.

Not really, in my view - it's in the simplistic cached_styles mechanism which
saves ON changes but not OFF changes.

If cached_styles[] stored all the ON and OFF changes, none of the duplication
of parts of display_line that is in your patch would be needed.

OTOH, with your patch's method, we may not need cached_styles[] any more
at all.

> > [*] I still have not tested it, hoping that Tom will put my patch in dev.20
> >     and that you will then send an updated patch. :)
> 
>   When you stop criticizing it :) Do you think it's correct?

Better don't wait for that.  :)
I suggest you send what you have now (as patch against dev.20) so I have
something more concrete to criticize...


>  BTW: in function highlight:LYUtils.c your patch adds the following code:
>  
>             if (LYP >= 0 && LYP < CACHEH && LXP >= 0 && LXP < CACHEW) {
>                 s = cached_styles[LYP][LXP];
     .....
>             } else {
>                 s = s_a;
>             }
>             LynxChangeStyle(s, STACK_ON, 0);
>         }
>  Why LynxChangeStyle(s, STACK_ON, 0) /* (0) */? May be it will be better
>     LynxChangeStyle(s, ABS_ON, 0);  /* (1) */
>  Currently it doesn't hurts since "ABS_OFF is the same as STACK_OFF for
> the moment" in curses_w_style - and there is a call in the 'highlight'
>    lynx_stop_link_color (flag == ON, links[cur].inUnderline);
> later, and lynx_stop_link_color consists of the call
>    LynxChangeStyle(flag == ON ? s_alink : s_a, ABS_OFF, 0);
> At my (current) level of lss understanding  /* (1) */ is more correct than 
> /* (0) */ - there can happen attribute stack overflow when ABS_OFF will 
> treated
> different from STACK_OFF (I assume that my patch will be applied later) 

Yes, this is certainly not clean and relies on ABS_OFF == STACK_OFF.
Something has to change here (and most likely in other places) if
ABS_OFF becomes different from STACK_OFF.  And it may also have to
change for your patch - try to come up with something better.

There was a reason though why I changed it from ABS_ON (as it was
previously).  This gave the right balance between pushes ond pops,
which wasnt there before.  (Actually, the ON for hightext2 would
also have to be changed to STACK_ON, I didn't test with split anchors.)
This was needed (as well as changing ABS to STACK in statusline() )
to preserve the stack pointer, for keeping the right attributes
at least for the advance-by-one-page case (which is now the only
case where last_colorattr_ptr isn't reset to 0 explicitly in
display_page() ).  All this was done before making the change in
split_line, that is when lines were unbalanced more often.

If possible, I'd like your changes to preserve the balanced behavior
of highlight(), if possible even for lines where styles[] have
become unbalanced.   But if you don't want to bother about that,
send you patches anyway - it can be dealt with later.

   Klaus