- Dillo-dev - lists.mailman3.com

patch: Re: [Dillo-dev] an experiment in widget coloring (with code)

by jcid＠dillo.org

On Sun, Feb 03, 2008 at 08:32:46PM +0100, Johannes Hofmann wrote: > Hi Jorge, > > On Sun, Feb 03, 2008 at 02:53:59PM -0300, Jorge Arellano Cid wrote: > > Hi Johannes, > > > > On Sun, Feb 03, 2008 at 06:06:39PM +0100, Johannes Hofmann wrote: > > > Hi, > > > > > > the following patch fixes the problem here: > > > > > > diff -r 4a9e0384d5ca dw/fltkviewbase.cc > > > --- a/dw/fltkviewbase.cc Sat Feb 02 18:35:17 2008 +0100 > > > +++ b/dw/fltkviewbase.cc Sun Feb 03 17:59:04 2008 +0100 > > > @@ -64,7 +64,12 @@ void FltkViewBase::draw () > > > translateCanvasYToViewY (rect->y), > > > rect->width, rect->height); > > > > > > - viewRect.intersect(view); > > > + viewRect.intersect (view); > > > + > > > + rect->x = translateViewXToCanvasX (viewRect.x ()); > > > + rect->y = translateViewYToCanvasY (viewRect.y ()); > > > + rect->width = viewRect.w (); > > > + rect->height = viewRect.h (); > > > > > > if (!viewRect.empty ()) { > > > push_clip (viewRect); > > > > > > > > > > > > It uses the rectangle that has been intersected with the current > > > view area for expose(). > > > > Great! > > > > BTW, I have also found places where an intersection may be missing... > > > > > But the real bug seems to be hidden somewhere in the table draw code. > > > Without the patch, expose was called with a very high area (height > > > was 106497!). > > > > Please try to get to the root of the problem. I'll keep on the > > too-many-redraws problem. Later we can merge the ideas. > > The root cause seems to be an overflow of X11 coordinates. > If you try: > > #include <stdio.h> > #include <fltk/Window.h> > #include <fltk/Widget.h> > #include <fltk/Button.h> > #include <fltk/run.h> > #include <fltk/draw.h> > #include <fltk/damage.h> > using namespace fltk; > > Widget *test; > > class Test : public Widget { > public: > Test(int _x, int _y, int _w, int _h) : Widget(_x, _y, _w, > _h) {}; > > void draw() { > for (int y=1000;y < 70000; y +=10) { > fillrect(0, y, 10, 10); > } > } > }; > > int main(int argc, char **argv) { > Window *window = new Window(300, 180); > window->begin(); > test = new Test(20, 40, 260, 100); > window->end(); > window->show(argc, argv); > return run(); > } > > > > you will see a black bar. If you only count to 60000 instead of > 70000, the window stays gray. > > X11 uses shorts somewhere that can overflow. > So the fix in fltkviewbase.cc is sufficient, I think. Excellent, I'll leave it as is. -- Cheers Jorge.-

Feb. 4, 2008

[experimental patch] excessive redraws (again)

by jcid＠dillo.org

Hi Johannes, On Sun, Feb 03, 2008 at 06:58:23PM +0100, Johannes Hofmann wrote: > Hi Jorge, > > On Sun, Feb 03, 2008 at 02:44:56PM -0300, Jorge Arellano Cid wrote: > > Hi Johannes, > > > > On Fri, Feb 01, 2008 at 07:11:31PM +0100, Johannes Hofmann wrote: > > > Hello, > > > > > > let's get more agressive regarding these excessive redraws :-) > > > > > > Below are two patches. The first one adds a own run method to > > > FltkPlatform. It is pretty similar to what fltk delivers, but this > > > way we can tweak it as we like. Especially when to call our idle > > > functions. > > > > I'm experimenting with another approach because AFAIU throttling > > FLTK idle functions not only affects drawing code. > > My patch does not throttle FLTK idle functions. It just does not use FLTK > idle function mechanism for dillo. It uses an own mechanism so we > can tweak it for our needs independent from what FLTK does. > > > > > So far I have very good results. Stay tuned... > > Great. Well an experimental prototype was committed. The original idea here was to use this new API: void Textblock::flush (bool asap) void FltkViewBase::queueDrawTotal (bool asap) (asap = "As soon as possible") and to call the function inmediately when "asap" is true and to otherwise delay to a tunable number of seconds. After some tests it showed extraordinarily effective. For instance testing with the "All" directory from: http://chlamydia.fs.ei.tum.de/pub/DragonFly/packages/stable/DragonFly-1.10.… it was roughly using 2 to 3 times less CPU (in my notebook) than using the FltkPlatform::generalIdle() throttling patch. Some glitches remained, and while I was working on it, the test server above went down... :( I got a big surprise when after some more tests, it worked quite well when ignoring everything but calls with asap=true ! :-) For instance, now the code reads: void FltkViewBase::queueDrawTotal (bool asap) { static time_t lastRedraw = 0; //if (asap || time(0) > lastRedraw + 2) { if (asap) { redraw (DAMAGE_EXPOSE); lastRedraw = time(0); } } (lastRedraw is not used). The patch is much simpler than it looks. Many lines are just for adding the extra parameter. Images on long pages still queue a resize that produces a total redraw. i.e when an image outside the viewport resizes, the viewport is redrawn causing flickering. This is not completely trivial because the image resize may change the size of the parent widget, and the parent widget may affect the viewable area. If this can somehow be avoided, for instance testing whether the parent widget changes its size, it'd be great. This part of the code clearly needs a review. I wanted to commit early so you guys have the chance to play with it. -- Cheers Jorge.-

Feb. 4, 2008

[experimental patch] excessive redraws (again)

by jcid＠dillo.org

On Sun, Feb 03, 2008 at 06:58:23PM +0100, Johannes Hofmann wrote: > Hi Jorge, > > On Sun, Feb 03, 2008 at 02:44:56PM -0300, Jorge Arellano Cid wrote: > > Hi Johannes, > > > > On Fri, Feb 01, 2008 at 07:11:31PM +0100, Johannes Hofmann wrote: > > > Hello, > > > > > > let's get more agressive regarding these excessive redraws :-) > > > > > > Below are two patches. The first one adds a own run method to > > > FltkPlatform. It is pretty similar to what fltk delivers, but this > > > way we can tweak it as we like. Especially when to call our idle > > > functions. > > > > I'm experimenting with another approach because AFAIU throttling > > FLTK idle functions not only affects drawing code. > > My patch does not throttle FLTK idle functions. It just does not use FLTK > idle function mechanism for dillo. It uses an own mechanism so we > can tweak it for our needs independent from what FLTK does. Oh, after a more in-depth review I see you're only throttling when to call FltkPlatform's generalIdle. That's OK. I'm experimentin with a queueResize(bool asap)/flush(bool asap) approach. A couple of glitches remain... -- Cheers Jorge.-

Feb. 4, 2008

patch: conditionally sending Cookie2: Re: [Dillo-dev] Re: patch: cookies string leak

by place＠gobigwest.com

Jorge wrote: > On Fri, Feb 01, 2008 at 06:26:56PM +0000, place wrote: > > Also, what would you think of me either > > > > - stuffing the "Cookie2: $Version=\"1\"\r\n" header into a_Cookies_get(url) > > OK. attached. I renamed it a_Cookies_get_query() in an effort to suggest that it now returned more than just the cookies. Everything looks fine when I watch the headers, but I don't use dillo for anything that _relies_ on cookies working properly. So...anyone who habitually uses cookies might want to give this a bit of a test.

Feb. 4, 2008

[patch] fix size of select widgets

by Johannes.Hofmann＠gmx.de

Hi Jorge, On Sat, Feb 02, 2008 at 07:23:17PM -0300, Jorge Arellano Cid wrote: > On Sat, Feb 02, 2008 at 07:27:20PM +0100, Johannes Hofmann wrote: > > Here is a small patch to compute the size of select widgets similar > > to the way it is computed for text input widgets. > > The width is hardcoded to 10 * size of 'M' in the current font. > > I committed a different version that measures the string length > (as dillo1 and FF). You can test with: > > http://www.dillo.org/bugtrack/Dinsert.html > > (adding a long string to a local copy is useful). > > I doubt that's the right way to do it, but at least it works. > Please consider it experimental and give it a look. I don't think it is valid to compute the size in addItem(). The style may change later, especially in the future with CSS. I would propose to do the size computation in sizeRequest(), even if that looks inefficient (patch below). That way it is consistent with e.g. FltkLabelButtonResource. While at it, I replaced the hardcoded 20 for the small rectangle on the right with what FLTK uses for the width of the rectangle. You can see a difference with e.g: <html> <body> <form> <select> <option>foo</option> <option>bar</option> </select> <h1> <select> <option>foo</option> <option>bar</option> </select> </h1> </form> </body> </html> diff -r 1c41f9a73971 dw/fltkui.cc --- a/dw/fltkui.cc Sun Feb 03 21:13:11 2008 +0100 +++ b/dw/fltkui.cc Mon Feb 04 16:13:00 2008 +0100 @@ -1021,7 +1021,6 @@ FltkOptionMenuResource::FltkOptionMenuRe { init (platform); selection = NULL; - maxStringWidth = 0; } FltkOptionMenuResource::~FltkOptionMenuResource () @@ -1052,9 +1051,20 @@ void FltkOptionMenuResource::sizeRequest if (style) { FltkFont *font = (FltkFont*)style->font; ::fltk::setfont(font->font,font->size); - requisition->width = maxStringWidth + 20 + 2 * RELIEF_X_THICKNESS; + + int maxStringWidth = 0; + for (int i = 0; i < getNumberOfItems (); i++) { + int len = (int)::fltk::getwidth (getItem(i)); + if (len > maxStringWidth) { + maxStringWidth = len; + } + } + requisition->ascent = font->ascent + RELIEF_Y_THICKNESS; requisition->descent = font->descent + RELIEF_Y_THICKNESS; + requisition->width = maxStringWidth + + (requisition->ascent + requisition->descent) * 4 / 5 + + 2 * RELIEF_X_THICKNESS; } else { requisition->width = 1; requisition->ascent = 1; @@ -1065,20 +1075,12 @@ void FltkOptionMenuResource::addItem (co void FltkOptionMenuResource::addItem (const char *str, bool enabled, bool selected) { - if (str) { - FltkFont *font = (FltkFont*)style->font; - ::fltk::setfont(font->font,font->size); - int len = (int)::fltk::getwidth (str); - if (len > maxStringWidth) { - maxStringWidth = len; - queueResize (true); - } - } - FltkSelectionResource<dw::core::ui::OptionMenuResource>::addItem (str,enabled,selected); if (selected) selection = items->get (items->size()-1); + + queueResize (true); } bool FltkOptionMenuResource::isSelected (int index) diff -r 1c41f9a73971 dw/fltkui.hh --- a/dw/fltkui.hh Sun Feb 03 21:13:11 2008 +0100 +++ b/dw/fltkui.hh Mon Feb 04 16:13:00 2008 +0100 @@ -507,7 +507,6 @@ private: private: static void widgetCallback (::fltk::Widget *widget, void *data); void *selection; - int maxStringWidth; public: FltkOptionMenuResource (FltkPlatform *platform); > > -- > Cheers > Jorge.- > > _______________________________________________ > Dillo-dev mailing list > Dillo-dev(a)dillo.org > http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

Feb. 4, 2008

patch: Re: [Dillo-dev] an experiment in widget coloring (with code)

by jcid＠dillo.org

On Sun, Feb 03, 2008 at 07:31:17PM +0100, Johannes Hofmann wrote: > Hi Jorge, > > On Sun, Feb 03, 2008 at 02:53:59PM -0300, Jorge Arellano Cid wrote: > > Hi Johannes, > > > > On Sun, Feb 03, 2008 at 06:06:39PM +0100, Johannes Hofmann wrote: > > > Hi, > > > > > > the following patch fixes the problem here: > > > > > > diff -r 4a9e0384d5ca dw/fltkviewbase.cc > > > --- a/dw/fltkviewbase.cc Sat Feb 02 18:35:17 2008 +0100 > > > +++ b/dw/fltkviewbase.cc Sun Feb 03 17:59:04 2008 +0100 > > > @@ -64,7 +64,12 @@ void FltkViewBase::draw () > > > translateCanvasYToViewY (rect->y), > > > rect->width, rect->height); > > > > > > - viewRect.intersect(view); > > > + viewRect.intersect (view); > > > + > > > + rect->x = translateViewXToCanvasX (viewRect.x ()); > > > + rect->y = translateViewYToCanvasY (viewRect.y ()); > > > + rect->width = viewRect.w (); > > > + rect->height = viewRect.h (); > > > > > > if (!viewRect.empty ()) { > > > push_clip (viewRect); > > > > > > > > > > > > It uses the rectangle that has been intersected with the current > > > view area for expose(). > > > > Great! > > > > BTW, I have also found places where an intersection may be missing... > > > > > But the real bug seems to be hidden somewhere in the table draw code. > > > Without the patch, expose was called with a very high area (height > > > was 106497!). > > > > Please try to get to the root of the problem. I'll keep on the > > too-many-redraws problem. Later we can merge the ideas. > > > > > This seems to cause problems. Perhaps some overflowing values? > > > With the patch (which is a good optimization anyway), the > > > height of the area that is passed to expose is limited by the > > > current view size. > > > > OK. > > > > It looks like there's no problem to commit this patch. > > I mean it doesn't hide the other bug, right? > > Yes it hides the other bug, but you can commit it anyway. Committed. -- Cheers Jorge.-

Feb. 3, 2008

patch: Re: [Dillo-dev] an experiment in widget coloring (with code)

by Johannes.Hofmann＠gmx.de

Hi Jorge, On Sun, Feb 03, 2008 at 02:53:59PM -0300, Jorge Arellano Cid wrote: > Hi Johannes, > > On Sun, Feb 03, 2008 at 06:06:39PM +0100, Johannes Hofmann wrote: > > Hi, > > > > the following patch fixes the problem here: > > > > diff -r 4a9e0384d5ca dw/fltkviewbase.cc > > --- a/dw/fltkviewbase.cc Sat Feb 02 18:35:17 2008 +0100 > > +++ b/dw/fltkviewbase.cc Sun Feb 03 17:59:04 2008 +0100 > > @@ -64,7 +64,12 @@ void FltkViewBase::draw () > > translateCanvasYToViewY (rect->y), > > rect->width, rect->height); > > > > - viewRect.intersect(view); > > + viewRect.intersect (view); > > + > > + rect->x = translateViewXToCanvasX (viewRect.x ()); > > + rect->y = translateViewYToCanvasY (viewRect.y ()); > > + rect->width = viewRect.w (); > > + rect->height = viewRect.h (); > > > > if (!viewRect.empty ()) { > > push_clip (viewRect); > > > > > > > > It uses the rectangle that has been intersected with the current > > view area for expose(). > > Great! > > BTW, I have also found places where an intersection may be missing... > > > But the real bug seems to be hidden somewhere in the table draw code. > > Without the patch, expose was called with a very high area (height > > was 106497!). > > Please try to get to the root of the problem. I'll keep on the > too-many-redraws problem. Later we can merge the ideas. The root cause seems to be an overflow of X11 coordinates. If you try: #include <stdio.h> #include <fltk/Window.h> #include <fltk/Widget.h> #include <fltk/Button.h> #include <fltk/run.h> #include <fltk/draw.h> #include <fltk/damage.h> using namespace fltk; Widget *test; class Test : public Widget { public: Test(int _x, int _y, int _w, int _h) : Widget(_x, _y, _w, _h) {}; void draw() { for (int y=1000;y < 70000; y +=10) { fillrect(0, y, 10, 10); } } }; int main(int argc, char **argv) { Window *window = new Window(300, 180); window->begin(); test = new Test(20, 40, 260, 100); window->end(); window->show(argc, argv); return run(); } you will see a black bar. If you only count to 60000 instead of 70000, the window stays gray. X11 uses shorts somewhere that can overflow. So the fix in fltkviewbase.cc is sufficient, I think. Cheers, Johannes > > > This seems to cause problems. Perhaps some overflowing values? > > With the patch (which is a good optimization anyway), the > > height of the area that is passed to expose is limited by the > > current view size. > > OK. > > It looks like there's no problem to commit this patch. > I mean it doesn't hide the other bug, right? > > -- > Cheers > Jorge.- > > _______________________________________________ > Dillo-dev mailing list > Dillo-dev(a)dillo.org > http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

Feb. 3, 2008

patch: Re: [Dillo-dev] an experiment in widget coloring (with code)

by Johannes.Hofmann＠gmx.de

Hi Jorge, On Sun, Feb 03, 2008 at 02:53:59PM -0300, Jorge Arellano Cid wrote: > Hi Johannes, > > On Sun, Feb 03, 2008 at 06:06:39PM +0100, Johannes Hofmann wrote: > > Hi, > > > > the following patch fixes the problem here: > > > > diff -r 4a9e0384d5ca dw/fltkviewbase.cc > > --- a/dw/fltkviewbase.cc Sat Feb 02 18:35:17 2008 +0100 > > +++ b/dw/fltkviewbase.cc Sun Feb 03 17:59:04 2008 +0100 > > @@ -64,7 +64,12 @@ void FltkViewBase::draw () > > translateCanvasYToViewY (rect->y), > > rect->width, rect->height); > > > > - viewRect.intersect(view); > > + viewRect.intersect (view); > > + > > + rect->x = translateViewXToCanvasX (viewRect.x ()); > > + rect->y = translateViewYToCanvasY (viewRect.y ()); > > + rect->width = viewRect.w (); > > + rect->height = viewRect.h (); > > > > if (!viewRect.empty ()) { > > push_clip (viewRect); > > > > > > > > It uses the rectangle that has been intersected with the current > > view area for expose(). > > Great! > > BTW, I have also found places where an intersection may be missing... > > > But the real bug seems to be hidden somewhere in the table draw code. > > Without the patch, expose was called with a very high area (height > > was 106497!). > > Please try to get to the root of the problem. I'll keep on the > too-many-redraws problem. Later we can merge the ideas. > > > This seems to cause problems. Perhaps some overflowing values? > > With the patch (which is a good optimization anyway), the > > height of the area that is passed to expose is limited by the > > current view size. > > OK. > > It looks like there's no problem to commit this patch. > I mean it doesn't hide the other bug, right? Yes it hides the other bug, but you can commit it anyway. Cheers, Johannes > > -- > Cheers > Jorge.- > > _______________________________________________ > Dillo-dev mailing list > Dillo-dev(a)dillo.org > http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

Feb. 3, 2008

[experimental patch] excessive redraws (again)

by Johannes.Hofmann＠gmx.de

Hi Jorge, On Sun, Feb 03, 2008 at 02:44:56PM -0300, Jorge Arellano Cid wrote: > Hi Johannes, > > On Fri, Feb 01, 2008 at 07:11:31PM +0100, Johannes Hofmann wrote: > > Hello, > > > > let's get more agressive regarding these excessive redraws :-) > > > > Below are two patches. The first one adds a own run method to > > FltkPlatform. It is pretty similar to what fltk delivers, but this > > way we can tweak it as we like. Especially when to call our idle > > functions. > > I'm experimenting with another approach because AFAIU throttling > FLTK idle functions not only affects drawing code. My patch does not throttle FLTK idle functions. It just does not use FLTK idle function mechanism for dillo. It uses an own mechanism so we can tweak it for our needs independent from what FLTK does. > > So far I have very good results. Stay tuned... Great. > > > > The other patch changes resizeIdle() a bit. As stated in the > > comments, resizeIdle() can queue a new resizeIdle(). So instead of > > drawing the preliminary layout, then change it and redraw again, we > > now wait until the layout is settled and then draw it. > > Committed. OK. Cheers, Johannes > > -- > Cheers > Jorge.- > > _______________________________________________ > Dillo-dev mailing list > Dillo-dev(a)dillo.org > http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

Feb. 3, 2008

patch: Re: [Dillo-dev] an experiment in widget coloring (with code)

by jcid＠dillo.org

Hi Johannes, On Sun, Feb 03, 2008 at 06:06:39PM +0100, Johannes Hofmann wrote: > Hi, > > the following patch fixes the problem here: > > diff -r 4a9e0384d5ca dw/fltkviewbase.cc > --- a/dw/fltkviewbase.cc Sat Feb 02 18:35:17 2008 +0100 > +++ b/dw/fltkviewbase.cc Sun Feb 03 17:59:04 2008 +0100 > @@ -64,7 +64,12 @@ void FltkViewBase::draw () > translateCanvasYToViewY (rect->y), > rect->width, rect->height); > > - viewRect.intersect(view); > + viewRect.intersect (view); > + > + rect->x = translateViewXToCanvasX (viewRect.x ()); > + rect->y = translateViewYToCanvasY (viewRect.y ()); > + rect->width = viewRect.w (); > + rect->height = viewRect.h (); > > if (!viewRect.empty ()) { > push_clip (viewRect); > > > > It uses the rectangle that has been intersected with the current > view area for expose(). Great! BTW, I have also found places where an intersection may be missing... > But the real bug seems to be hidden somewhere in the table draw code. > Without the patch, expose was called with a very high area (height > was 106497!). Please try to get to the root of the problem. I'll keep on the too-many-redraws problem. Later we can merge the ideas. > This seems to cause problems. Perhaps some overflowing values? > With the patch (which is a good optimization anyway), the > height of the area that is passed to expose is limited by the > current view size. OK. It looks like there's no problem to commit this patch. I mean it doesn't hide the other bug, right? -- Cheers Jorge.-

Feb. 3, 2008