launchpad-dev team mailing list archive

[Michael Hudson <michael.hudson@xxxxxxxxxxxxx>]

On Wed, 25 Aug 2010 21:00:40 -0400, Curtis Hovey <curtis@xxxxxxxxxx> wrote:
> In reply to Michael Hudson:
> > > I have seen menus show up in oopses, I have seen engineers on IRC
> > > surprised to see a query cause by a menu, but I have not seen an outline
> > > of the menu problem. I think I can summarise the problem and provide
> > > some guidelines about how we want menus to behave.
> > 
> > I think I'm lacking some context here, like what a menu is in the
> > Launchpad context.  Let me guess a little: a Menu is a collection of
> > Links.  There is one menu per instance of a LaunchpadView subclass.
> > Links can be disabled or enabled.
> 
> There are several menu types. Menus were for context objects. In 2.0 we
> added the NavigationMenu for context objects and views. The menu is
> responsible for creating the URL of a link, checking permissions, and
> enabling the link.
> 
> ContextMenu (context) A menu to render the links in the sidebar that are
> appropriate for all facets. This is now used as a bag to create inline
> links.
> tales: ISourcePackageRecipeBuild/menu:rescore

Er.  Are there many links that are appropriate in all facets?

> ApplicationMenu (context) A menu for the context when viewed on a facet
> (overview, bugs, branches, etc..). This was a list in the side bar, but
> now used like a bag to render inline links.
> tales: IPerson/menu:branches/active_reviews

Can you omit ":branches" to get the ApplicationMenu for the current
facet?

> NavigationMenu (context or view) A menu or a context or a view of a
> context. It contains links that bind pages with a common theme together.
> It may also have a submenu (a linking a menu on a context object to a
> menu for view). We tend to use this now to render action links in the
> side bar (context), or as group a related pages inline (view).
> tales: IPerson/menu:navigation/assignedbugs
> tales: PersonRelatedSoftwareView/menu:navigation/projects
> 
> ^^^ There looks to be a lot over overlap between these menus type.

I'd say.

> FacetMenu (context) The applications that are available to the context.
> It is the tabs/app buttons in the header. There is one direct
> implementer, StandardLaunchpadFacet,s and each object gets a subclass to
> define its links. I would say this works well if it were not for the
> fact that the tooltips for the links are very inconsistent for objects. 

I don't think inconsistent tooltips is really fixable at the machinery
level is it?

> > The Menu itself can be rendered, and renders each link one after the
> > other, but we don't ever do this these days.
> > 
> > >       * I recently saw Martin Pitt could not see his +participation page
> > >         because of a timeout reached querying all his uploads. This is
> > >         because a link in the person overview menu is only enabled if
> > >         the person has uploads. The page does *not* use this link.
> > 
> 
> > Because the menu instance is associated with the view instance,
> > rendering a portlet, even if it's a view on the same view class, will
> > create the menu over again?
> 
> They are associate via adaption of interfaces (Request + Context or
> View) The <context|view>/menu: call always instantiates using the
> current request. The path adapter always creates a new instance.

If we make menu creation really cheap though, this is fine I guess?

Caching Links might not be needed if we don't render them multiple times
on the same page very often?  I'd guess this is the case, but don't
know.

> Many pages do this
>     tal:define="overview_menu context/menu:overview;"
> so that a single instance of the overview menu will be used in the
> template. Each context/@@/+a-portlet needs to create its own instance of
> the menu. 

Right.

> > >       * Even though a link is defined in the overview menu (mainsite),
> > >         and a link's view defines its facet, the menu makes the link URL
> > >         to the current host eg. we had a 404 on code.lp.net for
> > >         +downloads and we could not use the menu to get the correct
> > >         link.
> > 
> > Doesn't the link just use morally the same code as fmt:link?  If not, it
> > should?  If it does, this bug should be fixed there?
> > 
> > facet is an obsolete idea of course. s/facet/layer/g.
> 
> Yes, but the menu is still looking to __launchpad_facetname__ that was
> hacked as a view attribute by our metazcml. Facet will be obsolete when
> we remove the metazcml directives and remove the two uses in menu.py and
> tales.py (the menu path adapter). 

Whee, fun.  We should still be able to s/facet/layer/ though, although
we might need to keep some customizations of the zcml directive.

> ...
> 
> > >       * The menus assume that they are instantiated once per request
> > >         because of the previous point.
> > 
> > This can never have been really true.
> 
> It was in the 1.0 layout because menus were only shown in the sidebar.
> We rendered them like a list. The list was composed of the context's
> ContextMenu and its ApplicationMenu. 

Oh ok, and no links where rendered in the portlets.

> Inline links were crafted in tales, just string concatenation with
> little to no guarantee that link should be enabled, that the user has
> permission, that the link goes to the correct host.

Heh.

> In 2.0 we decided to move the links into the page content, quietly
> subverting the menu's list feature into a bag. We also added
> NavigationMenus to manage lists for for a group of views. 

OK.

> > > Guidelines for expected behaviour
> > > 
> > >       * We build menus once per request. Getting an object's menu is
> > >         free once it is instantiated.
> > 
> > So this means menus are associated with context objects, not with view
> > instances?  (Maybe this is already true).
> 
> Only the NavigationMenu can be used with a view, and in 3.0 it is
> usually called once per request to render a list of links. This is no
> abused since we still use it as it much like it was intended. 

OK.

> > >       * We know that at the point we start rendering, that no link can
> > >         change--once a link object is setup, we must never do it
> > >         again.
> > 
> > Right.  In fact, getting the object's menu should be trivial -- it's
> > only getting a link out of that menu that should be non-trivial, and we
> > should cache the link in the request once its accessed.
> 
> I think so.

Well, given some of the above, I'm not sure any more.

----

It strikes me that there is a quite a lot of machinery here, possibly
more than we need.  I certainly don't see the difference between an
ApplicationMenu and a NavigationMenu clearly... 

It seems to me that mostly we get links by doing something like
getMultiAdapter((context, request), ILink, name=menu_name).  Links that
are appropriate for any facet can be registered against IBrowserRequest
or something.  In the case where we still want to render the whole menu,
maybe we should do something else.  Guesstimated tales:

person/link:assignedbug <!-- might blow up on non bugs domain? -->
person/menu:edit_actions <!-- would render a list of links -->

Sorry, this is getting a bit incoherent, I need to EOD, but I hope I'm
prodding you along in a useful direction :-)

Cheers,
mwh