Subject: Re: comments on the recent ter-off menu patches

From: Olivier Chapuis <[email-masked]>
Date: Tue, 1 Jul 2003 07:14:40 +0200

On Tue, Jul 01, 2003 at 12:53:34AM +0200, [email-masked] wrote:
> There are some parts of the patch I do not understand or that are
> bugs.
> 
> 1) In menu_tear_off():
> 
> 	ms = menustyle_find(buffer);
> 	if (!ms)
> 	{
> 		/* this must never happen */
> 		fvwm_msg(
> 			ERR, "menu_tear_off",
> 			"impossible to create %s menu style", buffer);
> 		free(buffer);
> 		DestroyMenu(mr, False, False);
> 		return;
> 	}
> 
> Is there a specific reason why you use menustyle_find() instead of
> "MR_STYLE(mr_to_copy) " which is much simpler and can not fail?
> Even if there is a reason, I don't think you can return from the
> function.
>

ms is not MR_STYLE(mr_to_copy) it is the menu style just created
by menustyle_parse_style(F_PASS_ARGS) where action is the address
of mr. Maybe, it will be good that menustyle_parse_style return
the menu style that it creates or modifies ... done.
 
> 2) In clone_menu_root_static():
> 
> What is the logic of changing
> 
> 	MR_COPIES(dest_mr) = 0;
> 
> to
> 
> 	MR_COPIES(dest_mr) = 1;
> 
> ?  Was it a bug?
>

When, you create a menu (with AddToMenu) MR_COPIES(mr) = 1.  The menu
code logic implies that MR_COPIES is never 0 but in DestroyMenu just
before it is totally destroyed (and maybe re-created and in this case
MR_COPIES is re-incremented).

Yes, I think it was a bug. What happen if you enter DestroyMenu
with MR_COPIES(dest_mr) = 0?

...
MR_COPIES(dest_mr)--;  <-- MR_COPIES is -1
...
if (MR_COPIES(dest_mr) == 0)  <-- leaks
{
   free some stuff
}

Any way if you change the test 

MR_COPIES(dest_mr) == 0

by 

MR_COPIES(dest_mr) <= 0

it is not a big problem to set MR_COPIES to 0 for a tear off
menu. However, it seems that the logic is to set MR_COPIES
to 1 when you "create" a tear off menu.
 
> 3) This is a bug:
> 
> 	do_menu_close_tear_off_menu(pmp->menu, *pmp);
> 	pmret->rc = MENU_ABORTED;
> 	discard_window_events(
> 		MR_WINDOW(pmp->menu), EnterWindowMask);
> 
> do_menu_close_tear_off_menu() destroys the MR_WINDOW(pmp->menu)
> and thus discard_window_events() operates on a random Xid.
>

Yes. Fixed.
 
> 4) Please name all functions exported in menustyle.c
> "menustyle_something()".  (I renamed copy_menu_style() to
> menustyle_copy()).
> 

Yep, such naming scheme is good. Ha 2 questions.
Under which condition you start the function with "__"?
(a private version of an exported function?).
What is the goal of the TearMenuOff command, how can
we use it?

Regards, Olivier
--
Visit the official FVWM web page at <URL:http://www.fvwm.org/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked]

---

From: Dominik Vogt <[email-masked]>
Date: Tue, 1 Jul 2003 10:09:50 +0200

> to 1 when you "create" a tear off menu.

Yes, it may have been a bug.  However, testing for <= 0 hides
potential problems elsewhere.  Instead, we should call abort() if
it ever becomes < 0.

> (a private version of an exported function?).

When I feel like it :-)  Seriously, the __ indicates its an
internal helper function that encapsulates some work done by
higher level functions of the same module.  Only static functions
can begin with __.  It indicates that the function is really,
*really* internal and must never be exported.

> What is the goal of the TearMenuOff command, how can

It is a dummy command only defined in menus, exactly like "Title".
Clicking on an item that uses it tears off the menu.  It's not
implemented yet, though.  If you try to write the command, the
following should be taken into account:

  a) It should work on all visible menu items:
      - normal items ("+ foo tearmenuoff")
      - separators ("+ bar nop tearmenuoff"?)
      - titles ("+ baz title tearmenuoff"?)

  b) An item that can tear off a menu must be selectable, i.e.
     must be hilighted when the mouse is over it.  This must be
     somehow merged into the IS_SELECTABLE flag.  Perhaps we need
     a new flag too.

  c) It would be nice if it worked in the dynamic menu actions.
     This is probably hard to implement, and thus optional.

Bye

Dominik ^_^  ^_^

---

From: Dominik Vogt <[email-masked]>
Date: Tue, 1 Jul 2003 15:07:15 +0200

>      This is probably hard to implement, and thus optional.

I have to take back everything.  TearMenuOff is already
implemented.  Try

  AddToMenu ...
  + "" TearMenuOff

for a tear off bar, and

  + "Tear me off!" for a normal item that reas out the menu.

Bye

Dominik ^_^  ^_^