Suggestion: focus on the core logic of the plugin, and defer the Menu refactoring since there are several UI tickets (#1048, #1154, and #1358).

The menu refactoring is pure 'php' refactoring to ensure all menus (main analytics menu, top bar menu and admin menu) provide the same features (ordering, etc.). I think this can be done independently of the UI refactoring and other menu improvements.

Ok. Very good.

Then wrtnaming convention, please use Piwik_Menu_Abstract (instead of Piwik_Abstract_Menu).

At the same time I think we should move core/PluginsFunctions/Menu.php and core/PluginsFunctions/AdminMenu.php into core/Menu/

Agreed. For consistency I should also rename Piwik_Menu to Piwik_Menu_Menu and Piwik_AdminMenu to Piwik_Menu_AdminMenu, does that sound reasonable?

Another thing to consider is renaming Piwik_Menu to Piwik_Menu_MainMenu (as we have TopMenu and AdminMenu), but this would definetely break backwards compatibility with 3rd party plugins (because of the hook Menu.add -> MainMenu.add and Piwik_AddMenu -> Piwik_AddMainMenu). I guess this should be left untouched.

I would say Piwik_Menu_Main, Piwik_Menu_Admin and Piwik_Menu_Top, and of course leaving the existing functions unchanged as we need to keep backward compatibility.

Alright. I'd like a code review of my menu restructuring:

• Code moved to core/Menu/
• Abstract class provides core functionality for all three menus (Main, Top, Admin)
• Abstract class provides submenus which are only being used by the MainMenu. I think it's good to provide this functionality for all menu types for later extension.

Note:

• This is only a snapshot, nothing complete!
• Ordering/Renaming/Editing does not work yet, easy to implement though.
• LanguageManager is not viewed yet.

I need suggestions for:

• The Feedback plugin uses "#id=topbar-feedback", instead of providing a link to call. Piwik_AddTopMenu nor the Piwik_Menu_Abstract::add() allow to define a #fragment. I think of modifying urlRewriteWithParameters() or Piwik_Url::getCurrentQueryStringWithParametersModified to produce a fragment when the $url array contains e.g. '_frag'.

$url = array('module' => 'A', 'action' => 'B', '_frag' => 'C=Foo');

would produce ?module=A&action=B#C=Foo

• I had to add $view->topBar = Piwik_GetTopMenu() to a lot of modules. Should the TopMenu be shown by default and possibly be deactivated similar to the normal Menu:

{assign var=showTopMenu value=false}

• For the LanguageManager I've to allow HTML as menu entry. Idea:
  -> Piwik_Abstract::addHtml(..) which allow to add HTML as menu entry OR
  -> Extend Piwik_Abstract::add() with a parameter $isHTML (default=false).

Snapshot of Alerts plugin coming soon.

• The Feedback plugin uses "#id=topbar-feedback", instead of providing a link to call. Piwik_AddTopMenu nor the Piwik_Menu_Abstract::add() allow to define a #fragment. I think of modifying urlRewriteWithParameters() or Piwik_Url::getCurrentQueryStringWithParametersModified to produce a fragment when the $url array contains e.g. '_frag'.

It seems the feedback link doesn't add a fragment, but a HTML id attribute:
id=topbar-feedback

We shouldnt add complicated logic just to handle this specific use case. I think the generated HTML for the top bar would contain some sort of ID that you could use and select instead of topbar-feedback in https://github.com/piwik/piwik/blob/master/plugins/Feedback/templates/feedback.js#L2

• I had to add $view->topBar = Piwik_GetTopMenu() to a lot of modules. Should the TopMenu be shown by default and possibly be deactivated similar to the normal Menu:

{assign var=showTopMenu value=false}

Yes that sounds good (the $view->topBar = Piwik_GetTopMenu() could then be refactored in setGeneralVariablesView maybe?

• For the LanguageManager I've to allow HTML as menu entry. Idea:
  -> Piwik_Abstract::addHtml(..) which allow to add HTML as menu entry OR
  -> Extend Piwik_Abstract::add() with a parameter $isHTML (default=false).

because it is currently only used in the top bar, you can maybe overwrite add() in the Top Menu class to allow for HTML as a menu entry.

Patch updated.

• Editing/Renaming works.
• Ordering works.
• TopMenu automatically generates IDs on the form "topmenu-{$module}", adjusted feedback.js (topbar -> topmenu).
• TopMenu adds function addHtml. Overwriting add() would mean to reimplement the same code from Abstract + extra handling of HTML. I extended Piwik_AddTopMenu instead to take a $isHTML parameter (default: false) which determines if the second parameter is a $url or $html.
• Views can use

{assign var=showTopMenu value=false} 

to disable the TopMenu which is now set by default in setGeneralVariablesView().

looks good! Few questions

• I see following lines seem the same, is the first one needed?

    88                  Piwik_AddMenu('Actions_Actions', '', array('module' => 'Actions', 'action' => 'getPageUrls'), true, 15); 
    89                  Piwik_AddMenu('Actions_Actions', 'Actions_SubmenuPages', array('module' => 'Actions', 'action' => 'getPageUrls'), true, 1); 

I also see the same call twice in plugins/Goals/Goals.php and Referers.php

• patch should delete file core/SmartyPlugins/function.assignTopBar.php
• in plugins/Dashboard/Dashboard.php the comment could be removed and the call to the function Piwik_AddMenu at the bottom removed too?
• unnecessary brackets in

if(($language = Piwik_LanguagesManager_API::getInstance()->getLanguageForSession()) != null) 

Appart from these minor things, it looks good - please confirm that you have tested the code, submit last patch and I will commit it, thanks!

• Yes the line is needed due to the ordering. The last argument differs everytime. The first line adds the first level menu with order 15, the second line adds the submenu "Pages" with order 1.
• Patch now deletes the SmartyPlugin
• Regarding the Dashboard: That was my intention, forgot to review that file.
• Removed the brackets, it works without them. My IDE (Netbeans) complains about an "Possible unwanted assignment in line x" - that was the reason I added the != null in the first place..

I tested all code intensively! - However, I just discovered a nasty bug for which I have no good idea how to fix it. For the "Visits" page the submenus are added by different plugins. As a result the first level menu "Visits" gets added indirectly (as a consequence of adding a submenu). This leads to the URL of UserSettings.index being the one that is associated with the first level menu. A later call to

Piwik_AddMenu('General_Visitors', '', array('module' => 'VisitsSummary', 'action' => 'index'), 10);

gets ignored, as the menu has already been added. The quick-fix in this case would be a call to

Piwik_EditMenuUrl($mainMenuToEdit, $subMenuToEdit, $newUrl)

this however would mean, that we'd need to do this for every first level menu entry, as new plugins could add submenus to them before the first level menu has been added.

Alternative: Allow to directly overwrite menu entries (would make Piwik_Edit*Url) obsolete.

Give me a short response / idea and I adjust it accordingly.

• in Piwik_AddMenu, can you make it work so that it caches calls and then builds the menu once all calls to addMenu have been done (ie. when you request the menu)?
• sorry for the brackets, you were right that they are important, I didn't see the first half of expression

Let me know when bug is fixed, and I'll commit

All fixed. Ready to commit :)

I applied the patch but the top menu didn't display, and the menu was displaying in random orders (see screenshot). is it working for you?

Attachment: menus in random orders, top menu missing except API
Piwik Web Analytics Reports.png

Also you can put the Piwik header files to the new files (eg. see core/Piwik.php header).

Also can you can remove empty comments (or write comments ;) from core/Menu/Abstract.php and other files.
cheers

The ordering works perfectly for me, did you maybe had conflicts in the files? When I updated I had several conflicts that I had to resolve. I upload a new patch containing comments, correct header and diffed against the latest trunk revision (r2400).

the patch is missing Actions/Actions.php menu calls with weights it seems.
Otherwise top menu displays OK for me, just "Actions" and "UI Framework" (plugin ExampleUI) menu is in wrong order.

cheers

Also when I visit the tracker after applying patch, I get the error:
: Class 'Piwik_Menu_Abstract' not found in core\Menu\Main.php on line 16

The menu files should not be loaded in the tracker as it slightly impacts performance.

Actions.php - Yeah :( My IDE screwed up the SVN states and therefore did not include the file in the patch.

ExampleUI - Added ordering.

Added comments to the Piwik_Add*Menu functions.

I agree that the menu should not be loaded when running the tracker - This is a pre-existing condition though as the menu is/was being loaded in core/PluginManager.php. The error you get is due to the autoloader being unavailable in tracking mode. To avoid the error I manually included the Abstract.php in PluginManager.php for now.

I applied the patch on a fresh checkout and it worked for me - Please recheck it though my IDE might've missed some files again.

Attachment: Patch against latest
MenuRestructure.patch

(In [2409]) Fixes #1403 Patch by halfdan: great stuff! Menu code is now clean and refactored.

The "Alerts plugin" is not fixed yet of course, reopening. halfdan, do you know when a beta will be available for Alerts? cheers

Regression: It seems the feedback link doesn't work anymore in trunk. It loads the content instead of opening a popover.

(In [2412]) refs #1403 feedback form is working again

SteveG: Thanks, missed that file in the patch.
matt: Yes, I'm running a little late due to exams/packing up my stuff before returning to Europe, but I'll be having a beta in approx. 3-4 days.

Thanks for commiting the refactored menu!

(In [2514]) Refs #1403
Added support for JSON/Serialized PHP export of Multi dimensional arrays in API responses, used by Alert plugin in the UI (json)