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]