- Dillo-dev - lists.mailman3.com

[BUG]: lout::misc::StringBuffer::clear() does not invalidate the string

by Johannes.Hofmann＠gmx.de

On Fri, May 02, 2008 at 06:15:44PM +0100, Jeremy Henty wrote: > On Fri, May 02, 2008 at 12:08:10PM -0400, Jorge Arellano Cid wrote: > > On Fri, May 02, 2008 at 01:48:43PM +0100, Jeremy Henty wrote: > > > > > > lout::misc::StringBuffer::clear() omits to invalidate the internal > > > cached string, > > > > I know how hard it gets to fix this kind of bugs sometimes. > > It only appears if you don't add anything to the StringBuffer between > clearing it and reading it, so it is hidden in almost all normal use. > > > BTW, does this mean the last selection patch shows no bugs anymore? > > There is one remaining issue with selections but I think it's not to > do with my patch. If several resource items are marked selected then > only the last one is selected on the FLTK menu widget. I think the > problem is that FltkSelectionResource::addItem selects the item widget > by calling set_item() on the menu widget, and that's only the right > thing to do for a fltk::Choice . For a fltk::Browser you just want to > call Widget::set_selected() on the menu item. I guess the fix is to > have a virtual hook in FltkSelectionResource and have each of > FltkListResource and FltkOptionMenuResource override the hook to do > the right thing. *If* I'm right then my selection patch is good as it > stands. I'll have a go at a patch and see if it works. Oh, that's nasty. There seems to be no way to select multiply items in fltk::Menu. As far as I can see one needs a fltk::Browser object for that. Perhaps the easiest way is to comletely overwrite the addItem() method in FltkListResource? Does FltkListResource need to provide something like submenus? Regards, Johannes > > I'd be glad of any tips for doing this. The fltk::Menu internals are > confusing and I'm also not familiar with the WidgetStack machinery in > FltkSelectionResource . > > Regards, > > Jeremy Henty > > _______________________________________________ > Dillo-dev mailing list > Dillo-dev(a)dillo.org > http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

May 2, 2008

patch: Re: [Dillo-dev] dillo-fltk on x86_64

by jcid＠dillo.org

On Fri, May 02, 2008 at 03:52:44PM +0000, corvid wrote: > Jorge wrote: > > On Wed, Apr 30, 2008 at 08:52:06PM +0000, corvid wrote: > > > Jorge wrote: > > > > A patch for ~/.dillo creation was just committed. > > > > > > I think dillo should tell the user that it is creating the directory. > > > > Committed. > > Shouldn't it be something like the following? > > (BTW, I had to set prefs.show_msg to get MSGs out of this and a_Prefs_init(), > but setting it in dillo.cc doesn't feel like a good thing to do.) > Ooops, yes. Looks like I've had to little time lately. A fix was committed. Please check! :-) -- Cheers Jorge.-

May 2, 2008

[BUG]: lout::misc::StringBuffer::clear() does not invalidate the string

by onepoint＠starurchin.org

On Fri, May 02, 2008 at 12:08:10PM -0400, Jorge Arellano Cid wrote: > On Fri, May 02, 2008 at 01:48:43PM +0100, Jeremy Henty wrote: > > > > lout::misc::StringBuffer::clear() omits to invalidate the internal > > cached string, > > I know how hard it gets to fix this kind of bugs sometimes. It only appears if you don't add anything to the StringBuffer between clearing it and reading it, so it is hidden in almost all normal use. > BTW, does this mean the last selection patch shows no bugs anymore? There is one remaining issue with selections but I think it's not to do with my patch. If several resource items are marked selected then only the last one is selected on the FLTK menu widget. I think the problem is that FltkSelectionResource::addItem selects the item widget by calling set_item() on the menu widget, and that's only the right thing to do for a fltk::Choice . For a fltk::Browser you just want to call Widget::set_selected() on the menu item. I guess the fix is to have a virtual hook in FltkSelectionResource and have each of FltkListResource and FltkOptionMenuResource override the hook to do the right thing. *If* I'm right then my selection patch is good as it stands. I'll have a go at a patch and see if it works. I'd be glad of any tips for doing this. The fltk::Menu internals are confusing and I'm also not familiar with the WidgetStack machinery in FltkSelectionResource . Regards, Jeremy Henty

May 2, 2008

[BUG]: lout::misc::StringBuffer::clear() does not invalidate the string

by jcid＠dillo.org

On Fri, May 02, 2008 at 01:48:43PM +0100, Jeremy Henty wrote: > > Well, *that* was a bit of a wild goose chase! Remember I complained > of selection problems in ListResources? It was nothing of the kind. > Instead test/dw-ui-test was spuriously reporting that things were > still selected when everything was in fact unselected. And the cause > was a bug in lout::misc::Stringbuffer ! > > lout::misc::StringBuffer::clear() omits to invalidate the internal > cached string, so if you call lout::misc::StringBuffer::getChars() > (which updates the cached string and marks it as valid), then > lout::misc::StringBuffer::clear() and then *immediately* call > lout::misc::StringBuffer::getChars() again, you get the contents of > the StringBuffer before you cleared it (because the cached value has > not been invalidated), instead of the empty string. D'oh! > > One-liner patch attached. Good you found it! I know how hard it gets to fix this kind of bugs sometimes. Committed. BTW, does this mean the last selection patch shows no bugs anymore? -- Cheers Jorge.-

May 2, 2008

patch: Re: [Dillo-dev] dillo-fltk on x86_64

by corvid＠lavabit.com

Jorge wrote: > On Wed, Apr 30, 2008 at 08:52:06PM +0000, corvid wrote: > > Jorge wrote: > > > A patch for ~/.dillo creation was just committed. > > > > I think dillo should tell the user that it is creating the directory. > > Committed. Shouldn't it be something like the following? (BTW, I had to set prefs.show_msg to get MSGs out of this and a_Prefs_init(), but setting it in dillo.cc doesn't feel like a good thing to do.)

May 2, 2008

dillo-fltk on x86_64

by jcid＠dillo.org

On Wed, Apr 30, 2008 at 08:52:06PM +0000, corvid wrote: > Jorge wrote: > > A patch for ~/.dillo creation was just committed. > > I think dillo should tell the user that it is creating the directory. Committed. -- Cheers Jorge.-

May 2, 2008

[BUG]: lout::misc::StringBuffer::clear() does not invalidate the string

by onepoint＠starurchin.org

Well, *that* was a bit of a wild goose chase! Remember I complained of selection problems in ListResources? It was nothing of the kind. Instead test/dw-ui-test was spuriously reporting that things were still selected when everything was in fact unselected. And the cause was a bug in lout::misc::Stringbuffer ! lout::misc::StringBuffer::clear() omits to invalidate the internal cached string, so if you call lout::misc::StringBuffer::getChars() (which updates the cached string and marks it as valid), then lout::misc::StringBuffer::clear() and then *immediately* call lout::misc::StringBuffer::getChars() again, you get the contents of the StringBuffer before you cleared it (because the cached value has not been invalidated), instead of the empty string. D'oh! One-liner patch attached. Regards, Jeremy Henty

May 2, 2008

[patch]: take 2: FtlkListResource

by Johannes.Hofmann＠gmx.de

On Thu, May 01, 2008 at 03:09:41PM +0100, Jeremy Henty wrote: > > Since Johannes' criticisms were (IIRC) mostly about my patch's > complexity I hope he will look at a new patch that sticks to my > original approach but is a lot simpler. > > I realised that if the user_data() of the Menu Widget items stored the > index of the Item instead of the Item itself then FltkListResource > could store the selections by index in a misc::SimpleVector<bool>. > This means there is no need to wrap bools in object::Object pointers > and no need to add instance methods to Items to make them > Hashable. Most of the scaffolding of the previous patch just > disappears (the line count decreases by a third). > > I don't want to dismiss Johannes' alternative suggestion out of hand, > but I still think my approach has a lot going for it and I'd like > people to have a chance to criticise my best shot at it. Very nice. This is really much simpler. I also understand, your point about not to extend Item with members specific for this usecase. Cheers, Johannes > > Comments are welcome! > > Jeremy Henty > diff -pru -- 00_pre/dw/fltkui.cc 01_post/dw/fltkui.cc > --- 00_pre/dw/fltkui.cc 2008-04-26 21:42:58.000000000 +0100 > +++ 01_post/dw/fltkui.cc 2008-05-01 14:00:11.000000000 +0100 > @@ -858,10 +858,11 @@ template <class I> FltkSelectionResource > } > > template <class I> > -::fltk::Item *FltkSelectionResource<I>::Item::createNewWidget () > +::fltk::Item *FltkSelectionResource<I>::Item::createNewWidget (int index) > { > - ::fltk::Item *item = new ::fltk::Item (name); > - item->user_data (this); > + ::fltk::Item *item = > + new ::fltk::Item (name); > + item->user_data ((void *) index); > return item; > } > > @@ -869,7 +870,10 @@ template <class I> > ::fltk::ItemGroup * > FltkSelectionResource<I>::Item::createNewGroupWidget () > { > - return new ::fltk::ItemGroup (name); > + ::fltk::ItemGroup *itemGroup = > + new ::fltk::ItemGroup (name); > + itemGroup->user_data ((void *) -1); > + return itemGroup; > } > > > @@ -922,12 +926,14 @@ FltkSelectionResource<I>::createNewWidge > > ::fltk::Group *currGroup = widgetStack->stack->getTop()->getTypedValue(); > > + int index = 0; > for (Iterator <Item> it = allItems->iterator (); it.hasNext (); ) { > Item *item = it.getNext (); > switch (item->type) { > case Item::ITEM: > - itemWidget = item->createNewWidget (); > + itemWidget = item->createNewWidget (index); > currGroup->add (itemWidget); > + index++; > break; > > case Item::START: > @@ -960,6 +966,7 @@ template <class I> void FltkSelectionRes > bool enabled, > bool selected) > { > + int index = items->size (); > Item *item = createNewItem (Item::ITEM, str, enabled, selected); > items->put (item); > allItems->append (item); > @@ -967,7 +974,7 @@ template <class I> void FltkSelectionRes > for (Iterator <WidgetStack> it = widgetStacks->iterator (); > it.hasNext(); ) { > WidgetStack *widgetStack = it.getNext (); > - ::fltk::Item *itemWidget = item->createNewWidget (); > + ::fltk::Item *itemWidget = item->createNewWidget (index); > widgetStack->stack->getTop()->getTypedValue()->add (itemWidget); > > if (!enabled) > @@ -1031,10 +1038,10 @@ template <class I> const char *FltkSelec > // ---------------------------------------------------------------------- > > FltkOptionMenuResource::FltkOptionMenuResource (FltkPlatform *platform): > - FltkSelectionResource <dw::core::ui::OptionMenuResource> (platform) > + FltkSelectionResource <dw::core::ui::OptionMenuResource> (platform), > + selection(-1) > { > init (platform); > - selection = NULL; > } > > FltkOptionMenuResource::~FltkOptionMenuResource () > @@ -1057,7 +1064,7 @@ void FltkOptionMenuResource::widgetCallb > void *data) > { > ((FltkOptionMenuResource *) data)->selection = > - ((::fltk::Menu *) widget)->item()->user_data(); > + (int) (((::fltk::Menu *) widget)->item()->user_data()); > } > > void FltkOptionMenuResource::sizeRequest (core::Requisition *requisition) > @@ -1092,14 +1099,14 @@ void FltkOptionMenuResource::addItem (co > FltkSelectionResource<dw::core::ui::OptionMenuResource>::addItem > (str,enabled,selected); > if (selected) > - selection = items->get (items->size()-1); > + selection = (items->size ()) - 1; > > queueResize (true); > } > > bool FltkOptionMenuResource::isSelected (int index) > { > - return items->get(index) == selection; > + return index == selection; > } > > // ---------------------------------------------------------------------- > @@ -1107,7 +1114,8 @@ bool FltkOptionMenuResource::isSelected > FltkListResource::FltkListResource (FltkPlatform *platform, > core::ui::ListResource::SelectionMode > selectionMode): > - FltkSelectionResource <dw::core::ui::ListResource> (platform) > + FltkSelectionResource <dw::core::ui::ListResource> (platform), > + itemsSelected(8) > { > init (platform); > } > @@ -1119,10 +1127,34 @@ FltkListResource::~FltkListResource () > > ::fltk::Menu *FltkListResource::createNewMenu (core::Allocation *allocation) > { > - return > + ::fltk::Menu *menu = > new ::fltk::MultiBrowser (allocation->x, allocation->y, > allocation->width, > allocation->ascent + allocation->descent); > + menu->callback(widgetCallback,this); > + menu->when(::fltk::WHEN_CHANGED); > + return menu; > +} > + > +void FltkListResource::widgetCallback (::fltk::Widget *widget, void *data) > +{ > + ::fltk::Widget *fltkItem = ((::fltk::Menu *) widget)->item (); > + int index = (int) (fltkItem->user_data ()); > + if (index == -1) return; > + bool selected = fltkItem->selected (); > + ((FltkListResource *) data)->itemsSelected.set (index, selected); > +} > + > +void FltkListResource::addItem (const char *str, bool enabled, bool selected) > +{ > + FltkSelectionResource<dw::core::ui::ListResource>::addItem > + (str,enabled,selected); > + int index = itemsSelected.size (); > + itemsSelected.increase (); > + itemsSelected.set (index,selected); > + > + // blindly copied from FltkOptionMenuResource, is it necessary? > + queueResize (true); > } > > void FltkListResource::sizeRequest (core::Requisition *requisition) > @@ -1135,8 +1167,7 @@ void FltkListResource::sizeRequest (core > > bool FltkListResource::isSelected (int index) > { > - /** todo Not implemented. */ > - return true; > + return itemsSelected.get (index); > } > > } // namespace ui > diff -pru -- 00_pre/dw/fltkui.hh 01_post/dw/fltkui.hh > --- 00_pre/dw/fltkui.hh 2008-04-26 21:42:58.000000000 +0100 > +++ 01_post/dw/fltkui.hh 2008-05-01 14:00:11.000000000 +0100 > @@ -458,8 +458,8 @@ protected: > bool selected = false); > ~Item (); > > - ::fltk::Item *createNewWidget (); > - ::fltk::ItemGroup *createNewGroupWidget (); > + ::fltk::Item *createNewWidget (int index); > + ::fltk::ItemGroup *createNewGroupWidget (); > }; > > class WidgetStack: public object::Object > @@ -508,7 +508,7 @@ protected: > > private: > static void widgetCallback (::fltk::Widget *widget, void *data); > - void *selection; > + int selection; > > public: > FltkOptionMenuResource (FltkPlatform *platform); > @@ -526,11 +526,17 @@ class FltkListResource: > protected: > ::fltk::Menu *createNewMenu (core::Allocation *allocation); > > +private: > + static void widgetCallback (::fltk::Widget *widget, void *data); > + misc::SimpleVector <bool> itemsSelected; > + > public: > FltkListResource (FltkPlatform *platform, > core::ui::ListResource::SelectionMode selectionMode); > ~FltkListResource (); > > + void addItem (const char *str, bool enabled, bool selected); > + > void sizeRequest (core::Requisition *requisition); > bool isSelected (int index); > }; > diff -pru -- 00_pre/test/dw_ui_test.cc 01_post/test/dw_ui_test.cc > --- 00_pre/test/dw_ui_test.cc 2008-04-26 21:42:59.000000000 +0100 > +++ 01_post/test/dw_ui_test.cc 2008-05-01 14:00:11.000000000 +0100 > @@ -192,7 +192,7 @@ int main(int argc, char **argv) > checklabel->addText (strdup (" check"), cellStyle); > checklabel->flush (); > > - for(int i = 0; i < 1; i++) { > + for(int i = 0; i < 2; i++) { > table->addRow (cellStyle); > > Embed *sel = new Embed (selres[i]); > _______________________________________________ > Dillo-dev mailing list > Dillo-dev(a)dillo.org > http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

May 1, 2008

[patch]: take 2: FtlkListResource

by onepoint＠starurchin.org

Since Johannes' criticisms were (IIRC) mostly about my patch's complexity I hope he will look at a new patch that sticks to my original approach but is a lot simpler. I realised that if the user_data() of the Menu Widget items stored the index of the Item instead of the Item itself then FltkListResource could store the selections by index in a misc::SimpleVector<bool>. This means there is no need to wrap bools in object::Object pointers and no need to add instance methods to Items to make them Hashable. Most of the scaffolding of the previous patch just disappears (the line count decreases by a third). I don't want to dismiss Johannes' alternative suggestion out of hand, but I still think my approach has a lot going for it and I'd like people to have a chance to criticise my best shot at it. Comments are welcome! Jeremy Henty

May 1, 2008

[patch]: FtlkListResource select callbacks

by onepoint＠starurchin.org

On Wed, Apr 30, 2008 at 10:52:37PM +0200, Johannes Hofmann wrote: > > This is a first cut at making FltkListResource work properly. > > > > The resource maintains a hash of its items' selected status, the > > MultiBrowser widget gets a callback that updates the hash and > > isSelected() looks up the hash for the answer. It's the same > > technique I used for FltkOptionMenuResource , where the FLTK > > widgets are linked to their resources by user_data() and callback > > arguments. > > > > I added equals() and hashValue() methods to Items so they could be > > hash keys, and implemented an object::Object wrapper class for > > boolean values to be the hash values. The wrapper class is inside > > FltkListResource , though it could easily move to lout::object > > IMHO. > > It seems a bit complicated to me. Wouldn't it be easier to just add > a member "selected" to Item that stores whether the item is > currently selected or not? May well be that I'm missing something > though. It's not so much complicated as cumbersome: the logic is pretty straightforward, but using the Hash requires extra scaffolding. I was reluctant to add stuff to Item because that class serves double duty for both FltkListResource and FltkOptionMenuResource and so it's ugly for Item to contain stuff that is specific to one of the resource classes. Of course the resource classes could derive their own Item classes, but that adds scaffolding too. IIRC I experimented with that when I first did FltkOptionMenuResource (before going for a simpler suggestion of yours, I think) and the templating made it particularly messy. Let me think about it a little while I fix the bugs. I think the bug fixes will be orthogonal to the issue of how we store the selection information so it shouldn't impact implementing your suggestions if that's the way to go. Regards, Jeremy Henty

May 1, 2008