On Thu, 04 Apr 2013, Daniel Déchelotte escribió: > > From: "Rodolfo García Peñas (kix)" <[email protected]> > > > > The function dockMenuCreate is splitted in three functions: > > > > - create_clip_main_menu(WScreen*) > > - create_dock_main_menu(WScreen*) > > - dockMenuCreate(WScreen*, int*) > > First I liked the patch but now I don't :) My biggest complaint is > that it introduces code duplication. Readability is a bit better, but > in retrospect, the if (type != WM_CLIP) { ... } else { ... } is quite > readable already. The last benefit of your patch is that it separated > the clip menu caching (scr->clip_menu) from the actual menu creation, > but that point alone doesn't warrant creating a new function IMHO. So > my recommendation would be to leave the original code unchanged.
I did this patch because this function has about 150 lines, with two different codes: 1. Select the Dock type 2. Create the menu for that type. The new code includes 2 functions more, but the code is easer to understand. All the behaviour for the clip is in the clip function, and all the dock things are in the dock function. Now we don't have a "if (type == WM_CLIP)" lost at the end of the function, after the WM_CLIP block because there is a common code with the WM_DOCK. Of course, the code is the same. About the switch-case, I don't like too much the idea of "something" (if) "the rest" (else) when we know that or is WM_DOCK or is WM_CLIP. But tomorrow could be WM_DRAWER (probably with their create_drawer_main_menu() function). I did some parts of my code thinking in the future drawer (You used switch-case in one of your patches). But I like a lot your recommendation!! I would like the people vote for patches or not (now: +1 -> me, -1 -> You) :-) kix > -- Daniel > > > The function create_clip_main_menu() creates the menu and the menu > > items > > when the menu is for a Clip. > > > > The function create_dock_main_menu() creates the menu and the menu > > items > > when the menu is for a Dock. > > > > The function dockMenuCreate() is now smaller, and more clear. The > > function > > select if the menu creation is for a clip or for a dock, and then > > call the > > right function. The function uses switch-case instead if-else and > > controls > > if the Clip was previously created. The menu and menuitem creation is > > outside > > then, is easier to understand it. > > > > Signed-off-by: Rodolfo García Peñas (kix) <[email protected]> > > --- > > src/dock.c | 114 > > ++++++++++++++++++++++++++++++++++++++---------------------- > > 1 file changed, 73 insertions(+), 41 deletions(-) > > > > diff --git a/src/dock.c b/src/dock.c > > index 21d6aee..d4d25ce 100644 > > --- a/src/dock.c > > +++ b/src/dock.c > > @@ -961,59 +961,73 @@ static WMenu *makeClipOptionsMenu(WScreen *scr) > > return menu; > > } > > > > -static WMenu *dockMenuCreate(WScreen *scr, int type) > > +static WMenu *create_dock_main_menu(WScreen *scr) > > { > > WMenu *menu; > > WMenuEntry *entry; > > > > - if (type == WM_CLIP && scr->clip_menu) > > - return scr->clip_menu; > > + menu = wMenuCreate(scr, NULL, False); > > + > > + entry = wMenuAddCallback(menu, _("Keep on Top"), > > toggleLoweredCallback, NULL); > > + entry->flags.indicator = 1; > > + entry->flags.indicator_on = 1; > > + entry->flags.indicator_type = MI_CHECK; > > + > > + wMenuAddCallback(menu, _("Launch"), launchCallback, NULL); > > + wMenuAddCallback(menu, _("Unhide Here"), unhideHereCallback, NULL); > > + > > + entry = wMenuAddCallback(menu, _("Hide"), hideCallback, NULL); > > + wfree(entry->text); > > + entry->text = _("Hide"); > > + > > + wMenuAddCallback(menu, _("Settings..."), settingsCallback, NULL); > > + wMenuAddCallback(menu, _("Kill"), killCallback, NULL); > > + > > + return menu; > > +} > > + > > +static WMenu *create_clip_main_menu(WScreen *scr) > > +{ > > + WMenu *menu; > > + WMenuEntry *entry; > > > > menu = wMenuCreate(scr, NULL, False); > > - if (type != WM_CLIP) { > > - entry = wMenuAddCallback(menu, _("Keep on Top"), > > toggleLoweredCallback, NULL); > > - entry->flags.indicator = 1; > > - entry->flags.indicator_on = 1; > > - entry->flags.indicator_type = MI_CHECK; > > - } else { > > - entry = wMenuAddCallback(menu, _("Clip Options"), NULL, NULL); > > - scr->clip_options = makeClipOptionsMenu(scr); > > - if (scr->clip_options) > > - wMenuEntrySetCascade(menu, entry, scr->clip_options); > > > > - entry = wMenuAddCallback(menu, _("Rename Workspace"), > > renameCallback, NULL); > > - wfree(entry->text); > > - entry->text = _("Rename Workspace"); > > + entry = wMenuAddCallback(menu, _("Clip Options"), NULL, NULL); > > + scr->clip_options = makeClipOptionsMenu(scr); > > + if (scr->clip_options) > > + wMenuEntrySetCascade(menu, entry, scr->clip_options); > > > > - entry = wMenuAddCallback(menu, _("Selected"), selectCallback, > > NULL); > > - entry->flags.indicator = 1; > > - entry->flags.indicator_on = 1; > > - entry->flags.indicator_type = MI_CHECK; > > + entry = wMenuAddCallback(menu, _("Rename Workspace"), > > renameCallback, NULL); > > + wfree(entry->text); > > + entry->text = _("Rename Workspace"); > > > > - entry = wMenuAddCallback(menu, _("Select All Icons"), > > selectIconsCallback, NULL); > > - wfree(entry->text); > > - entry->text = _("Select All Icons"); > > + entry = wMenuAddCallback(menu, _("Selected"), selectCallback, > > NULL); > > + entry->flags.indicator = 1; > > + entry->flags.indicator_on = 1; > > + entry->flags.indicator_type = MI_CHECK; > > > > - entry = wMenuAddCallback(menu, _("Keep Icon"), > > keepIconsCallback, > > NULL); > > - wfree(entry->text); > > - entry->text = _("Keep Icon"); > > + entry = wMenuAddCallback(menu, _("Select All Icons"), > > selectIconsCallback, NULL); > > + wfree(entry->text); > > + entry->text = _("Select All Icons"); > > > > - entry = wMenuAddCallback(menu, _("Move Icon To"), NULL, NULL); > > - wfree(entry->text); > > - entry->text = _("Move Icon To"); > > - scr->clip_submenu = makeWorkspaceMenu(scr); > > - if (scr->clip_submenu) > > - wMenuEntrySetCascade(menu, entry, scr->clip_submenu); > > + entry = wMenuAddCallback(menu, _("Keep Icon"), keepIconsCallback, > > NULL); > > + wfree(entry->text); > > + entry->text = _("Keep Icon"); > > > > - entry = wMenuAddCallback(menu, _("Remove Icon"), > > removeIconsCallback, NULL); > > - wfree(entry->text); > > - entry->text = _("Remove Icon"); > > + entry = wMenuAddCallback(menu, _("Move Icon To"), NULL, NULL); > > + wfree(entry->text); > > + entry->text = _("Move Icon To"); > > + scr->clip_submenu = makeWorkspaceMenu(scr); > > + if (scr->clip_submenu) > > + wMenuEntrySetCascade(menu, entry, scr->clip_submenu); > > > > - wMenuAddCallback(menu, _("Attract Icons"), colectIconsCallback, > > NULL); > > - } > > + entry = wMenuAddCallback(menu, _("Remove Icon"), > > removeIconsCallback, NULL); > > + wfree(entry->text); > > + entry->text = _("Remove Icon"); > > > > + wMenuAddCallback(menu, _("Attract Icons"), colectIconsCallback, > > NULL); > > wMenuAddCallback(menu, _("Launch"), launchCallback, NULL); > > - > > wMenuAddCallback(menu, _("Unhide Here"), unhideHereCallback, NULL); > > > > entry = wMenuAddCallback(menu, _("Hide"), hideCallback, NULL); > > @@ -1021,11 +1035,29 @@ static WMenu *dockMenuCreate(WScreen *scr, > > int type) > > entry->text = _("Hide"); > > > > wMenuAddCallback(menu, _("Settings..."), settingsCallback, NULL); > > - > > wMenuAddCallback(menu, _("Kill"), killCallback, NULL); > > > > - if (type == WM_CLIP) > > - scr->clip_menu = menu; > > + return menu; > > +} > > + > > +static WMenu *dockMenuCreate(WScreen *scr, int type) > > +{ > > + WMenu *menu = NULL; > > + > > + switch (type) { > > + case WM_DOCK: > > + menu = create_dock_main_menu(scr); > > + break; > > + case WM_CLIP: > > + if (scr->clip_menu) { > > + /* If the common menu for Clips exists, use it */ > > + menu = scr->clip_menu; > > + } else { > > + /* Else, create and save for other Clips */ > > + menu = create_clip_main_menu(scr); > > + scr->clip_menu = menu; > > + } > > + } > > > > return menu; > > } > > -- > > 1.7.10.4 > > > > > > -- > > To unsubscribe, send mail to > > [email protected]. > > -- ||// //\\// Rodolfo "kix" Garcia ||\\// //\\ http://www.kix.es/ -- To unsubscribe, send mail to [email protected].
