- Dillo-dev - lists.mailman3.com

patch: clean up a_Html_add_image

by corvid＠lavabit.com

Jeremy wrote: > On Sat, Feb 07, 2009 at 08:20:18PM +0000, corvid wrote: > > Jeremy wrote: > > > > There is IMHO a nasty code smell around a_Html_add_new_image(): it > > > takes a boolean parameter that determines whether or not it > > > actually adds the image. It's only called from two other places, > > > once with the parameter set to false and once with it set to true. > > > [...] > > In principle it might be dangerous in some way for the callers to > > wait to add widgets until after Html_load_image() is called, > > If we want the a_Html_* API to guard against that then that is another > reason for changing it, since the current API allows you to create the > image and not add it. In that case I'd suggest applying my patch and > then changing a_Html_image_new() to static Html_image_new() and adding > a_Html_add_new_image() and a_Html_add_new_image_button(). That would > fix the code smell *and* enforce the "add any image that you create" > condition. Does that make sense? I'm not sure the "danger" is really worth worrying about; it's just something that crossed my mind...

Feb. 8, 2009

Close-tab Button

by jcid＠dillo.org

On Sat, Feb 07, 2009 at 09:57:52PM +0000, Jo?o Ricardo Louren?o wrote: > S?b, 2009-02-07 ?s 18:33 -0300, Jorge Arellano Cid escreveu: > > On Fri, Feb 06, 2009 at 05:56:28PM -0300, Jorge Arellano Cid wrote: > > > On Mon, Feb 02, 2009 at 12:37:18AM +0000, Jo?o Ricardo Louren?o wrote: > > > > Dom, 2009-02-01 ?s 14:51 -0300, Jorge Arellano Cid escreveu: > > > > > On Sun, Feb 01, 2009 at 12:06:49AM +0000, Jo?o Ricardo Louren?o wrote: > > > > > > Greetings to all. > > > > > > > > > > > > As a simple introduction may I say I thank you all for your work on > > > > > > Dillo. > > > > > > > > > > > > Currently, I am trying to implement the Close-Tab button. I've been > > > > > > discussing it with Jorge Arellano Cid and he suggested I 'brought-it' to > > > > > > dillo-dev. > > > > > > > > > > > > FLTK Tabs don't support a close-tab button, as many of you have seen, > > > > > > and the idea was to have a button at the end of the tabs list. > > > > > > > > > > > > For that, I created an fltk::Button widget in CustTabGroup. I position > > > > > > this widget at the end of the tabs list and, then, I add() it to the > > > > > > CustTabGroup's parent() (UI). > > > > > > > > > > > > After that, I set the UI's resizer() to be an InvisibleBox positioned in > > > > > > such a way that it doesn't resize the button itself, but only moves it. > > > > > > > > > > > > Refreshing the button (when changing/adding/closing tabs) is done in the > > > > > > handle() method of the CustTabGroup. > > > > > > > > > > > > Just a second ago I solved one of the problems, which was the fact that > > > > > > tab_height() returned 0 in the constructor and resize() didn't work. I > > > > > > worked around that by constructing the button only in event() if > > > > > > tab_height()>0 and it hasn't been constructed yet. This way we can use > > > > > > the correct height for the button. > > > > > > > > > > > > It may not seem reliable to do so many things in handle(). Any > > > > > > suggestions? > > > > > > > > > > We need to get something working, review it, and then we may > > > > > ask in fltk.general whether it could have side effects. > > > > > > > > > > > It is functional, only you can say if it fully suits Dillo's needs > > > > > > though. > > > > > > > > > > Please post the patch here so we can look at it. > > > > > > > > > > > Unfortunately, there are some problems: > > > > > > > > > > > > The first one is the most-relevant one. When the list of tabs grows it > > > > > > can go over the button. I have been trying to fix this, but I can't seem > > > > > > to do it. > > > > > > A possible way could (maybe) be to 'trick' the pager into thinking it > > > > > > has less space left to draw the tabs, and that could possibly be > > > > > > achieved using update_positions(), although I am having some problems > > > > > > with that. > > > > > > > > > > Or maybe just tweaking the value. After all it's a workaround. > > > > > > > > > > > The second problem is a minor one, but annoying. I believe when CSS > > > > > > rendering is triggered, the render area flickers and that is normal. > > > > > > However, the way I implemented this (probably because of it being in > > > > > > handle()) makes the Button also flash. > > > > > > > > > > A side affect of being a child of UI. > > > > > > > > > > > > > So, as requested, here is what I have got until now. There is a > > > > particular line I think is not necessary, I commented that specifically. > > > > > > I'm reviewing it now. > > > > Well, the good part is that it works and can be polished a bit. > > The problem is side effects (e.g. redraw storms while moving the mouse > > over the panel buttons, and who knows what else?). > > > > I tried a different approach and it almost worked OK, but after > > resizing the window, the redraw storms are on the whole window. > > > > Frustrating... > > > > It looks like drawing the button in the pager could work. I'll > > try to write some code this way. > > > > Thanks. Redraw storms are visible when running it under valgrind. Also, > parts of the code there were/are unnecessary. For instance, we could > send the redraw() message only when adding a new tab and not > continuously, but the redraw storms continue either way... > > Adding it to the pager seems the way to go there, but will you add it as > a button, or a custom drawn widget? Custom drawn widget. > If you need any help, do say, I'll try to do my best, Good! Currently I have a good start. It draws the "button" from draw_tabs() and the events can be got from CustTabGroup's handle(). I'll try to advance it a bit more and then send you the code for finishing. -- Cheers Jorge.-

Feb. 7, 2009

patch: clean up a_Html_add_image

by onepoint＠starurchin.org

On Sat, Feb 07, 2009 at 08:20:18PM +0000, corvid wrote: > Jeremy wrote: > > There is IMHO a nasty code smell around a_Html_add_new_image(): it > > takes a boolean parameter that determines whether or not it > > actually adds the image. It's only called from two other places, > > once with the parameter set to false and once with it set to true. > > [...] > In principle it might be dangerous in some way for the callers to > wait to add widgets until after Html_load_image() is called, If we want the a_Html_* API to guard against that then that is another reason for changing it, since the current API allows you to create the image and not add it. In that case I'd suggest applying my patch and then changing a_Html_image_new() to static Html_image_new() and adding a_Html_add_new_image() and a_Html_add_new_image_button(). That would fix the code smell *and* enforce the "add any image that you create" condition. Does that make sense? Regards, Jeremy Henty

Feb. 7, 2009

Close-tab Button

by jorl17.8＠gmail.com

S?b, 2009-02-07 ?s 18:33 -0300, Jorge Arellano Cid escreveu: > On Fri, Feb 06, 2009 at 05:56:28PM -0300, Jorge Arellano Cid wrote: > > On Mon, Feb 02, 2009 at 12:37:18AM +0000, Jo?o Ricardo Louren?o wrote: > > > Dom, 2009-02-01 ?s 14:51 -0300, Jorge Arellano Cid escreveu: > > > > On Sun, Feb 01, 2009 at 12:06:49AM +0000, Jo?o Ricardo Louren?o wrote: > > > > > Greetings to all. > > > > > > > > > > As a simple introduction may I say I thank you all for your work on > > > > > Dillo. > > > > > > > > > > Currently, I am trying to implement the Close-Tab button. I've been > > > > > discussing it with Jorge Arellano Cid and he suggested I 'brought-it' to > > > > > dillo-dev. > > > > > > > > > > FLTK Tabs don't support a close-tab button, as many of you have seen, > > > > > and the idea was to have a button at the end of the tabs list. > > > > > > > > > > For that, I created an fltk::Button widget in CustTabGroup. I position > > > > > this widget at the end of the tabs list and, then, I add() it to the > > > > > CustTabGroup's parent() (UI). > > > > > > > > > > After that, I set the UI's resizer() to be an InvisibleBox positioned in > > > > > such a way that it doesn't resize the button itself, but only moves it. > > > > > > > > > > Refreshing the button (when changing/adding/closing tabs) is done in the > > > > > handle() method of the CustTabGroup. > > > > > > > > > > Just a second ago I solved one of the problems, which was the fact that > > > > > tab_height() returned 0 in the constructor and resize() didn't work. I > > > > > worked around that by constructing the button only in event() if > > > > > tab_height()>0 and it hasn't been constructed yet. This way we can use > > > > > the correct height for the button. > > > > > > > > > > It may not seem reliable to do so many things in handle(). Any > > > > > suggestions? > > > > > > > > We need to get something working, review it, and then we may > > > > ask in fltk.general whether it could have side effects. > > > > > > > > > It is functional, only you can say if it fully suits Dillo's needs > > > > > though. > > > > > > > > Please post the patch here so we can look at it. > > > > > > > > > Unfortunately, there are some problems: > > > > > > > > > > The first one is the most-relevant one. When the list of tabs grows it > > > > > can go over the button. I have been trying to fix this, but I can't seem > > > > > to do it. > > > > > A possible way could (maybe) be to 'trick' the pager into thinking it > > > > > has less space left to draw the tabs, and that could possibly be > > > > > achieved using update_positions(), although I am having some problems > > > > > with that. > > > > > > > > Or maybe just tweaking the value. After all it's a workaround. > > > > > > > > > The second problem is a minor one, but annoying. I believe when CSS > > > > > rendering is triggered, the render area flickers and that is normal. > > > > > However, the way I implemented this (probably because of it being in > > > > > handle()) makes the Button also flash. > > > > > > > > A side affect of being a child of UI. > > > > > > > > > > So, as requested, here is what I have got until now. There is a > > > particular line I think is not necessary, I commented that specifically. > > > > I'm reviewing it now. > > Well, the good part is that it works and can be polished a bit. > The problem is side effects (e.g. redraw storms while moving the mouse > over the panel buttons, and who knows what else?). > > I tried a different approach and it almost worked OK, but after > resizing the window, the redraw storms are on the whole window. > > Frustrating... > > It looks like drawing the button in the pager could work. I'll > try to write some code this way. > Thanks. Redraw storms are visible when running it under valgrind. Also, parts of the code there were/are unnecessary. For instance, we could send the redraw() message only when adding a new tab and not continuously, but the redraw storms continue either way... Adding it to the pager seems the way to go there, but will you add it as a button, or a custom drawn widget? If you need any help, do say, I'll try to do my best, Jo?o

Feb. 7, 2009

Close-tab Button

by jcid＠dillo.org

On Fri, Feb 06, 2009 at 05:56:28PM -0300, Jorge Arellano Cid wrote: > On Mon, Feb 02, 2009 at 12:37:18AM +0000, Jo?o Ricardo Louren?o wrote: > > Dom, 2009-02-01 ?s 14:51 -0300, Jorge Arellano Cid escreveu: > > > On Sun, Feb 01, 2009 at 12:06:49AM +0000, Jo?o Ricardo Louren?o wrote: > > > > Greetings to all. > > > > > > > > As a simple introduction may I say I thank you all for your work on > > > > Dillo. > > > > > > > > Currently, I am trying to implement the Close-Tab button. I've been > > > > discussing it with Jorge Arellano Cid and he suggested I 'brought-it' to > > > > dillo-dev. > > > > > > > > FLTK Tabs don't support a close-tab button, as many of you have seen, > > > > and the idea was to have a button at the end of the tabs list. > > > > > > > > For that, I created an fltk::Button widget in CustTabGroup. I position > > > > this widget at the end of the tabs list and, then, I add() it to the > > > > CustTabGroup's parent() (UI). > > > > > > > > After that, I set the UI's resizer() to be an InvisibleBox positioned in > > > > such a way that it doesn't resize the button itself, but only moves it. > > > > > > > > Refreshing the button (when changing/adding/closing tabs) is done in the > > > > handle() method of the CustTabGroup. > > > > > > > > Just a second ago I solved one of the problems, which was the fact that > > > > tab_height() returned 0 in the constructor and resize() didn't work. I > > > > worked around that by constructing the button only in event() if > > > > tab_height()>0 and it hasn't been constructed yet. This way we can use > > > > the correct height for the button. > > > > > > > > It may not seem reliable to do so many things in handle(). Any > > > > suggestions? > > > > > > We need to get something working, review it, and then we may > > > ask in fltk.general whether it could have side effects. > > > > > > > It is functional, only you can say if it fully suits Dillo's needs > > > > though. > > > > > > Please post the patch here so we can look at it. > > > > > > > Unfortunately, there are some problems: > > > > > > > > The first one is the most-relevant one. When the list of tabs grows it > > > > can go over the button. I have been trying to fix this, but I can't seem > > > > to do it. > > > > A possible way could (maybe) be to 'trick' the pager into thinking it > > > > has less space left to draw the tabs, and that could possibly be > > > > achieved using update_positions(), although I am having some problems > > > > with that. > > > > > > Or maybe just tweaking the value. After all it's a workaround. > > > > > > > The second problem is a minor one, but annoying. I believe when CSS > > > > rendering is triggered, the render area flickers and that is normal. > > > > However, the way I implemented this (probably because of it being in > > > > handle()) makes the Button also flash. > > > > > > A side affect of being a child of UI. > > > > > > > So, as requested, here is what I have got until now. There is a > > particular line I think is not necessary, I commented that specifically. > > I'm reviewing it now. Well, the good part is that it works and can be polished a bit. The problem is side effects (e.g. redraw storms while moving the mouse over the panel buttons, and who knows what else?). I tried a different approach and it almost worked OK, but after resizing the window, the redraw storms are on the whole window. Frustrating... It looks like drawing the button in the pager could work. I'll try to write some code this way. -- Cheers Jorge.-

Feb. 7, 2009

patch: clean up a_Html_add_image

by corvid＠lavabit.com

Jeremy wrote: > There is IMHO a nasty code smell around a_Html_add_new_image(): it > takes a boolean parameter that determines whether or not it actually > adds the image. It's only called from two other places, once with the > parameter set to false and once with it set to true. > > This patch cleans things up by renaming a_Html_add_new_image() to > a_Html_image_new(), removing the boolean parameter and making the > caller add the image (or not) as appropriate. > > Perhaps it is a judgement call as to whether this is an improvement, > but I feel that it is. Please comment. In principle it might be dangerous in some way for the callers to wait to add widgets until after Html_load_image() is called, but as long as there aren't any timeouts in between, I don't think we'd be able to make it break.

Feb. 7, 2009

patch: clean up a_Html_add_image

by onepoint＠starurchin.org

D'oh! Patch attached. Jeremy Henty

Feb. 7, 2009

patch: clean up a_Html_add_image

by Johannes.Hofmann＠gmx.de

On Sat, Feb 07, 2009 at 05:57:21PM +0000, Jeremy Henty wrote: > > There is IMHO a nasty code smell around a_Html_add_new_image(): it > takes a boolean parameter that determines whether or not it actually > adds the image. It's only called from two other places, once with the > parameter set to false and once with it set to true. > > This patch cleans things up by renaming a_Html_add_new_image() to > a_Html_image_new(), removing the boolean parameter and making the > caller add the image (or not) as appropriate. > > Perhaps it is a judgement call as to whether this is an improvement, > but I feel that it is. Please comment. did you forget to attach the patch? Cheers, Johannes

Feb. 7, 2009

patch: clean up a_Html_add_image

by onepoint＠starurchin.org

There is IMHO a nasty code smell around a_Html_add_new_image(): it takes a boolean parameter that determines whether or not it actually adds the image. It's only called from two other places, once with the parameter set to false and once with it set to true. This patch cleans things up by renaming a_Html_add_new_image() to a_Html_image_new(), removing the boolean parameter and making the caller add the image (or not) as appropriate. Perhaps it is a judgement call as to whether this is an improvement, but I feel that it is. Please comment. Regards, Jeremy Henty

Feb. 7, 2009

patch: <s> line appearance

by corvid＠lavabit.com

Johannes wrote: > On Sat, Feb 07, 2009 at 05:01:12AM +0000, corvid wrote: > > I was thinking that large text with a thin line through it > > looks odd. > > Turns out that there was also a problem with <sup> causing the > > <s> line to be too high. > > > > This looks about right to me with the few fonts I have around. > > Maybe one could determine the line width based on the xHeight member > of the font? Just an idea. Ah, good. 'diff' was sort of compensating for what sup and sub do to ascent and descent, but imperfectly -- as you probably noticed. Here's a new patch.

Feb. 7, 2009