Hi Jorge, On Thu, Apr 03, 2008 at 06:16:03PM -0400, Jorge Arellano Cid wrote:
Hi,
On Fri, Mar 28, 2008 at 05:08:01PM +0000, Jeremy Henty wrote:
On Fri, Mar 28, 2008 at 09:57:10AM -0400, Jorge Arellano Cid wrote:
On Wed, Mar 26, 2008 at 10:23:48PM +0000, Jeremy Henty wrote:
If the page contains a meta tag that changes the Content-Type then Dillo re-opens the URL. This calls Layout::removeWidget(), which in turn calls Layout::setAnchor(NULL). Ouch! If the URL contained a fragment then the anchor is lost. Worse, the anchor doesn't get restored because the URL is flagged as "just reloading" before it is reopened. The result is that the anchor mysteriously stops working.
Commenting out the call to Layout::setAnchor(NULL) in Layout::removeWidget() fixes the bug and doesn't seem to break anything else, but I'm not convinced it's the right thing to do.
In fact, repush is an after thought, and some parts of nav also need some re-work. The API, IIRC pushing #fragment URLs to the stack has some anomalies too. It'd be good to find/design a single path to handle it (experience shows multiple paths is asking for trouble :). I've looked at it and it seems like deep analysis is required.
Thanks for explaining this. I'll keep running with Layout::setAnchor(NULL) commented out for the moment and see how it goes. (Still no breakage yet.) I hope to be able to spend time looking at this further. If I have any thoughts about a good way to do all this I will post them. Unfortunately I have had to devote more time to earning a living recently (sigh) so I've spent less time looking at Dillo code than I would have liked.
After a long review of the code, I committed a patch that not only fixes things in nav.c but in other places which were also involved in the SEGFAULTs we were getting. Please test it Jeremy.
Johannes, there's a patch for dw2 too. It involves minor initialization of scrollX and scrollY, and a printf. If I open dillo with /tmp, a wild value is got for scrollY:
The added initialization is good I think.
jcid@d620:~/C/Dillo/d2/dillo2-cur/src$ ./dillo-fltk /tmp/ dillo_dns_init: Here we go! (threaded) Enabling cookies as from cookiesrc... Nav_open_url: new url='file:/tmp/' Nav_open_url: old_url='' idx=-1 [file dpi]: dpip_tag={<cmd='open_url' url='file:/tmp/' '>} adjustScrollPos: scrollX=0 scrollY=-155 ^^^^--- here!
Note: my /tmp is shorter than the viewport.
Please give it a look.
Funny, I had a similar effect in fltkviewport.cc some time ago. The problem becomes more obvious, when adding another printf at the beginning of adjustScrollPos (). Then we get: Nav_open_url: new url='file:/tmp/hu' Nav_open_url: old_url='' idx=-1 [file dpi]: dpip_tag={<cmd='open_url' url='file:/tmp/hu' '>} adjustScrollPos: scrollX=0 scrollY=0 adjustScrollPos: scrollX=0 scrollY=-376 So its adjustScrollPos() itself that makes scrollY negative! The fix is to do the checks the other way round and not to use "else if" (see attached patch). I noticed one regression with the new anchor code: The "Continue Reading Older Posts" link on www.boingboing.net (e.g. http://www.boingboing.net/2008/04/03/#entry-44240) no longer scrolls to the right position. Cheers, Johannes PS: I hope the patch format is ok like this. I'm using mercurial and the builtin diff may behave a bit differently than standard diff. Do you prefer inline or attached patches?