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

[Vlad Harchev]

On Tue, 16 Mar 1999, Klaus Weide wrote:

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

  I was wrong - the solution I've suggested was incorrect. It shouldn't be
 LynxChangeStyle (CStyle.style,ABS_ON,CStyle.previous);
 It should be as it was - I found a nice feature of LSS - see below.

> 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.
   
        Here what I've found:
  In each HTLine, all styles are balanced, ie the # of styles with
(dir==STACK_ON) == # of styles with dir==(STACK_OFF) in each line. So, if
you apply all styles in the line in a sequence, the last_ptr (stack position)
will remain unchanged (if there are no others bugs in lss code). So, the
patch I've sent originally does correct thing - it applies all styles of
HTline, but draws only the required part of a line. After drawing that
part it sets remaining styles of a line, no loops should be altered in
patch I've originally sent. I've composed an html of your examples with
line breaks you've mentioned , and debugged lynx - last_ptr is always =0
after each link is unhighlitten - especially I've stepped through
'redraw_patr_of_line' for the last sample with 3-line anchor - it's OK. 
And of course :), links unhighlighting is done correctly. I've tested lss
file that is shipped with dev19 - ie before applying your patch.
  
> 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.

      It seems that all assertions are already implemented in 
 'curses_w_style'.

> [...]  
> 
> 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.)
 
  Sorry I didn't tested - browsing Inet is a pain for me. Can you describe 
  the effect?

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

   I've specially tested such case after reading your note - I wrote an
 html file in which there was only one line on last page, and I tested
 links on the page one before last - it works OK.

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

  May be the value should be simply corrected when displayed on the
  '=' screen ?  This will require small modification. 

> [...]
> 
> >      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
        
    It seems that this paragraph is truncted :( 
> > 
> >  I'm ready to futher correct patch that I've sent.
> 
> Ok, please do so. :)
>     Klaus
> 
  IMO it's nothing to correct in my patch after my words. But I'm ready to
write something that will improve source colorization.

 Best regards,
 -Vlad