[XForms] Question #2: Relying on menu subitem IDs after fl_delete_menu_item().

[Jason Cipriani]

To subscribers of the xforms list

On Fri, Jul 4, 2008 at 4:37 PM, Jason Cipriani <jason.cipriani at gmail.com> wrote:
> On Fri, Jul 4, 2008 at 6:57 AM, Jens Thoms Toerring <jt at toerring.de> wrote:
>> To get around that you have to change the file lib/menu.c.
>> I append a version for 1.0.90 that hopefully works but I
>> didn't test the changes with that version, just my newer
>> one. Then add to lib/private/menu.h
>
> Since this change, the functions fl_set_menu_item_mode and
> fl_get_menu_item_mode no longer work unless the menu item's index is
> the same as it's value. It doesn't matter if you specify an index or a
> value to these functions -- if the two are different, they are
> ignored. I suspect that something internally is inappropriately mixing
> item indices and values, and had always relied on them being the same.

I've found the problem, and a partial solution to it (see below, I've
also provided a possible full solution), although I'm not sure if the
solution breaks anything.

SOLUTION #1:

In the new %x handling code in addto_menu, around line 357 there is
this bit of code that removes the %x from the string after processing
it:

	else
	{
		sp->mval[ n ] = strtol( p + 2, &eptr, 10 );
		while ( *eptr && isspace( ( int ) *eptr ) )
			eptr++;
		if ( *eptr )
			memmove( p, eptr, strlen( eptr ) + 1 );
		else
			*p = '\0';
	}

The fix is to not remove the "%x" from the string, changing it to just this:

	else
	{
		sp->mval[ n ] = strtol( p + 2, &eptr, 10 );
	}

The reason this problem was occurring is because every time you
display a menu, it uses the setpup stuff to create a popup menu to
display. This popup menu is set up in do_menu around line 111 of
menu.c. All of the popup menu functions take item IDs, not indices.
When you use %x to modify the menu's ID, this causes sp->mval[n] to
hold the ID, and so all of the MENU functions know about the ID.
However, since the %x is removed from the string, when the popup is
created that corresponds to the menu, the item IDs are not propagated
to the popup menu -- therefore the MENU functions know about the item
ID but the POPUP functions do not. Leaving the "%x" in the string
allows the ID to be passed through fl_addtopup() and then the correct
IDs are set in the popup menu, and everything seems to work.

SOLUTION #2:

The reason the above fix isn't complete is that it only applies when
you've used %x to set the IDs. The problem solved by the above
solution is related to the new feature of parsing %x in menu strings
but is not related to removing menu items. However, removing menu
items does cause a problem if you haven't used %x at all that the
above solution doesn't fix. If that makes sense.

Anyways if you do not use %x then the menu item IDs are, initially,
the item indices. If you remove a menu item, then indices change but
the IDs do not. However, the IDs in this case are never passed to
fl_addtopup at all, and so the POPUP menu uses the item INDICES
instead -- which no longer correspond to the IDs since an item was
removed. This is related to the problem when using %x in that it's
also caused by inconsistency between the item IDs in menu.c and the
item IDs in xpopup.c.

The root cause of all of these problems is that do_menu does not set
the popup menu item IDs to the sp->mval[i] values. All menu ID
information is lost in the popup. AFAICT, there is no API call to
change popup menu item IDs, and therefore there is no easy way for
do_menu() to pass ID information to the popup menu when it sets up the
menu with fl_addtopup().

One solution for this is to continue removing the %x strings in
addto_menu, just as you are doing now, but when creating the popup
menu, create temporary strings with a %x concatenated onto the end of
them along with the corresponding ID. Something like this in do_menu
around line 111:

  + char tempstr[512];
  + snprintf(tempstr, sizeof(tempstr), "%s%%x%i", sp->items[i], sp->mval[i]);
  + fl_addtopup(sp->menu, tempstr);
  - fl_addtopup(sp->menu, sp->items[i]);

This works fine but it does change the behavior of some other stuff.
Specifically, it causes fl_get_menu to return the item ID and NOT the
item index as previous. Meaning that you can just do this:

  int value = fl_get_menu(menu);

Rather than having to do this:

  int value = fl_get_menu_value(menu, fl_get_menu(menu));

Although that now also means that if you're passing the result of
fl_get_menu() to other menu functions, you'll need to convert the
value to an index first:

  int index = fl_get_menu_item_from_value(menu, fl_get_menu(menu));
  fl_delete_menu_item(menu, index); /* for example */

It's important to note, though, that the above changes in behavior
would ONLY happen if the IDs and values are not the same, which can
ONLY happen if you've either used %x to change the IDs, or you've
removed a menu item. Therefore I believe using the above solution
(constructing a temporary string with a %x on the end) will not
significantly affect any existing applications because:

  1) We know nobody has been removing menu items, since
fl_delete_menu_item didn't work at all.
  2) We know nobody has been using %x to set the IDs in regular menus,
since it was never supported.
  3) The only people who may experience changes in behavior are people
that had a previously meaningless "%x" in their menu item text, and
now it means something. However, your addto_menu change already
affected these people, the tempstr thing above doesn't add any new
issues.

What do you think about solution #2? Do you see any way it could
interact with regular popup menus at all? What about fdesign? Things
like that?

Incidentally, I've also noticed that, every once in a while, fdesign
strips the %x from the end of my menu strings. I am not sure why, or
what triggers it. It's happened to me twice out of about 15 saves so
far, though.

Jason
_______________________________________________
To unsubscribe, send any message to
xforms-leave at bob.usuhs.mil or see: 
http://cweblog.usuhs.mil/mailman/listinfo/xforms
List Archive: http://bob.usuhs.mil/pipermail/xforms and
http://bob.usuhs.mil/mailserv/list-archives/
Development: http://savannah.nongnu.org/files/?group=xforms