On Tue, Jul 01, 2003 at 12:53:34AM +0200, [EMAIL PROTECTED] 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 PROTECTED]
To report problems, send mail to [EMAIL PROTECTED]
