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

[Klaus Weide]

On Sun, 14 Mar 1999, Vlad Harchev wrote:
> On Tue, 16 Mar 1999, Klaus Weide wrote:
> > 
> > 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. 

I haven't digested this yet (but don't worry about it :) ).

> > > +PRIVATE void redraw_part_of_line ARGS4( 
> > > +        HTLine *,        line,
> > > +        char*,          str,
> > > +        int,            len,
> > > + HText *,        text)
> > > +{
> > ...........
> > > + 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)

That's nice for the ON changes, but won't work for the OFF changes.

This whole LynxChangeStyle interface is a mess, I'm just trying to use
it as best as I can understand, but I think nobody here understands
how it was originally supposed to work - RP has never explained it
afaik.  CStyle.previous is completely unused (and never being set).
There are remnants of a slang implementation (which never worked
afaik) where LynxChangeStyle is defined differently (see LYCurses.h)
and that seems to have used the 'previous' parameter, but the function
slang_style is nowhere to be seen...  Maybe the 'previous' was meant
to allow ABS_OFF changes to know what to change to.  (And maybe not;
but we could use it for this purpose, I think at this point it is
illusory that somebody will come forward to resurrect the original
intended meaning.)

Currently LynxChangeStyle(..,ABS_OFF,..) needs the stack to restore
the previous style.  (ABS_OFF is the same as STACK_OFF, another loose
end.)  IF it is changed to use the third argument (only for ABS_OFF?),
AND _internal_HTC() is changed to fill the new styles[..].previous
with the right style to go back to for OFF changes AND the logic in
split_line() is changed to copy the previous element, THEN your idea
could be made to work.  But that's a lot of IFs, another approach may
be easier.

Something like: take care that the stack doesn't underflow (i.e. don't
pop more than we push in one anchor's extent, and restore pointer at end
of anchor's extent to what it was before.

>    BTW, I've posted my lss file in message with subject 
>   'lynx-dev suggestions' dated dated (according to the 'Date' field)
>   5 March 1999.

Yes, I've seen it; but not tried it.

> I've tested the patch I've sent on the file with  
>   hyperlinks with 2-line anchors with multiplie attributes in anchor text.

Have you also tested with a case like the following?

   <A ...>blah blah <I>more| blah</I> last blah</A>
                           ^line break occurs here

Even more interesting cases:

   <A ...>blah blah<I>| more blah</I> last blah</A>
                      ^line break occurs here
(note space *after* start tag)

   <A ...>blah <B>blah <I>more| blah</I> last</B> blah</A>
                              ^line break occurs here
(several nested emphasis tags - assuming each shows as a different style)

   <A ...>blah <B> blah<I>more| much more... blah| blah</I> last</B> blah</A>
                              ^line break        ^second line break
(a very long anchor - possible side effects for going to next page)

I haven't tried any of these (since my code doesn't try to deal with
the problem at all) but I hope you will. :)

Here is a page that shows another effect; does your patch deal with it?

  <http://www.ietf.org/>

(You need to have styles for A and IMG that are different in order to
see it.)

Btw. that page shows also an effect of the </TR> change that was in my
patches (becomes visible when toggling ^V) because it has a table with
invalid </tr> tags.

> > 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.
> >
.....
> > > +    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.

Or done just enough times until the stack pointer is restored to what
it was at the beginning...

> > > +PUBLIC void redraw_lines_of_link ARGS1( 
> > > +            int , cur )
......
> > > +    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?

I don't know - I just know that I don't trust that last_line stuff to behave
always as I expect without checking. :)

While we are appending to a HText structure, last_line points to the current
(incomplete) one.  Then when we are done, there are some weird manipulations
(removing empty lines), with the end result that last_line points to the last
line (surprise :) ) which is now not incomplete (by definition - since we
have finished appending).  If that last line is not empty, it could be the
first line on the next screen (which would have just that one line), in that
case it seems (HTMainText->next_line == HTMainText->last_line) would be true
but we are not an the last screen.

Actually there's more questionable stuff in this area - it seems that the
total number of lines (as shown on the '=' screen and used for 'P'rint)
is off by one for HTML documents.  There are most likely code sections
that have come to rely on this kind of mis-counting, so correcting it
could introduce some other subtle problems.  (I haven't tried it.)
Also this mis-counting doesn't happen for plain text.  Add for plain
text, an incomplete (not terminated) line isn't counted (lynx counts
like wc), so a text/plain file with just one incomplete lien will
show up with 'Number of lines: 0' on the 'P'rint screen, but if
you 'Save to a local file' you end up with a file with 1 line...

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

True.
>                        (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. 

And not even always right (see www.ietf.org example).

>  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. [ example snipped ]

Right.

By the way, in some sense color style handling of anchor+emphasis
is the reverso of non-color-style handling: with non-color-style
lynx, empasis highlighting that *surrounds* an anchor is visibly
shown as different style for the anchor text, but not emphasis
*within* the anchor.

>      About source highlighting:
>  IMO it looks fancy, but confusing.

The changes were "cheap" - I just put together things that were already
there, and combined them with a bit of glue.  (The same way '-preparsed'
came into existence initially.  [ IIRC, FM commented that he wouldn't
have put such a developer-only feature in the code, but... well I differ,
taking into account the cost/benefit ratio.])   The source color stuff
> 
>  I'm ready to futher correct patch that I've sent.

Ok, please do so. :)

    Klaus