grok.ViewletManager.render() overrides viewlet sort order

[Jan Wijbrand Kolman]

The render() method of grok.ViewletManager uses util.sort_components just before rendering "its" viewlets. This effectively bypasses the sort() method, part of the IViewletManager interface, which should take care of viewlet order.

The grok.ViewletManager baseclass should implement a sort() method that will use util.sort_components by default, but leaves the application developer with the possibility to implement his own strategy.

[Jan Wijbrand Kolman (janwijbrand)]

I'm about to commit a fix on a branch.

[Jan Wijbrand Kolman (janwijbrand)]

Fix commited on http://svn.zope.org/grok/branches/jw-fix-viewlet-sorting/

Can someone please review this?

[LeoRochael (leorochael)]

I didn't, but your description makes sense and I trust your code. For small fixes like this, unless you get an immediate complaint on grok-dev, you should just assume silence is consent and merge it after a couple of days of announcing it. It can always be rolled back later.

[Philipp von Weitershausen (philikon)]

Good catch, JW. I have just two remarks about your patch:

* I understand having to sort twice (once using util.sort_components
to sort by the grok.order() and once by viewlet's name), but it may
not be obvious to everybody. Thus a comment explaining what this code
actually wants to do would be nice. By the way, I think the (index,
(name, viewlet)) tuple could be flat, i.e. (index, name, viewlet).
Then the list comprehension would also be a little more readable, i.e.
[name, viewlet for index, name, viewlet in sorted(indexed_viewlets)].

* It would be good if the default behaviour of first sorting by
grok.order(), then by viewlet name were tested. In other words, a test
that had two viewlets with the same grok.order() but different names
would be great to have.

[Martijn Faassen (faassen)]

I think you're correct about this bug. That said, I can't find an explicit specification that 'sort' is actually part of the IViewletManager interface. It's in the class, but not in the interface, at least on the trunk.

If I didn't miss it, we should discuss on zope-dev whether we should add the sort() method to the interface of IViewletManager.

[Martijn Faassen (faassen)]

Actually Philipp pointed me to the double sort. I don't think that's needed, as sort_components will sort any sort of component, no matter whether grok.order() is used.

I therefore suspect that we can massively simplify the code to:

def sort(self, viewlets):
   return util.sort_components((viewlet for name, viewlet in viewlets))

We have very extensive tests for this sorting in Grok already, and class name is definitely one of the things that is used already.

I don't understand where all the complexity of indexed_viewlets comes from. What's the motivation?

I see that the standard viewlets code sorts by viewlet name as the viewlet is registered in the CA, not class name. Anyway, if we want this to factor into the sorting behavior as well, we could modify sort_components. First I'd like to hear a good reason that this is actually useful.

With grok order, if you want to make order explicit, use grok.order(some_number). If you don't want to make it explicit but just by definition order of viewlets, use grok.order() (which will sort after explicit order). If you don't care, leave it out entirely, it'll be by class name (which will sort after any actual use of grok.order()). It'll also use module names to have consistent sorting of viewlets defined in different modules.

[Philipp von Weitershausen (philikon)]

On 16 May 2008, at 20:53 , Martijn Faassen wrote:
> Actually Philipp pointed me to the double sort. I don't think that's
> needed, as sort_components will sort any sort of component, no matter
> whether grok.order() is used.

I first thought the same thing, but it's not that simple. JW wants to
sort by whatever grok.order() specifies (even if it's not used the
directive is still used to read a default value) and *then* by the
viewlet's name. At least that's how I read his code. Your code below
only sorts by grok.order(), not by the viewlet's name:

> I therefore suspect that we can massively simplify the code to:
>
> def sort(self, viewlets):
> return util.sort_components((viewlet for name, viewlet in viewlets))
>
> We have very extensive tests for this sorting in Grok already, and
> class
> name is definitely one of the things that is used already.

Yes, actually, now that you mention it, I see that sort_components
sort by class name as the last key as well. Well that's not
necessarily the same as the viewlet name since that can always differ
from the class name when grok.name() has been used, right?

> I see that the standard viewlets code sorts by viewlet name as the
> viewlet is registered in the CA, not class name. Anyway, if we want
> this
> to factor into the sorting behavior as well, we could modify
> sort_components.

That's probably a good idea.

> With grok order, if you want to make order explicit, use
> grok.order(some_number). If you don't want to make it explicit but
> just
> by definition order of viewlets, use grok.order() (which will sort
> after
> explicit order). If you don't care, leave it out entirely, it'll be by
> class name (which will sort after any actual use of grok.order()).
> It'll
> also use module names to have consistent sorting of viewlets defined
> in
> different modules.

That still won't account for viewlet names, only class names. But as
you suggest, modifying util.sort_components might be a good option. (I
still don't know what I'm supposed to think of util.sort_components.
Right now it seems to be it's very specific to viewlets, so I'm
inclined to suggest inlining it into ViewletManager altogether.)

[Martijn Faassen (faassen)]

The point is to have *some* consistent default ordering, which can be overridden by the user. zope.viewlet's default mechanism to sort on the registration name provides a form of consistent ordering (though not actually if two registrations with same name are involved; not sure whether that's possible). It isn't very good to allow it to be overridden by the user; you'd have to start coming up with wacky names. For that reason zope.viewlet *also* provides another base class WeightOrderedViewletManager, where you can put in a 'weight' attribute on your viewlets and have it sort by that. That takes care of user overriding of order, but suddenly it doesn't have consistent default behavior anymore (everything has weight 0 if you don't specify the weight). It also supplies the (underdocumented) option to override the 'sort' method to do your custom ordering. I think it's good to support this, which is where JW is coming from.

grok.order() and sort_components take care of consistent, user adjustable ordering, through an explicit directive. If you don't use the directive, ordering is still consistent, using class name and module name. You suggest we also should sort on viewlet registration name (and JW's code also suggests this). I don't understand: why would you *want* to sort by viewlet registration name at all? (note also that by default, the registration name and the class name will be the same). I'm not saying I have a better reason to sort by class name by the way, except that it's more generic. I'd *prefer* to always sort by definition order in the module, but unfortunately this is hard to accomplish, so we fall back on class name.

About inlining sort_components into the viewlet manager: grok.order and sort_components are not specific to viewlets, and are tested with an independent test suite. Inlining it would make testing harder. The intent of having this infrastructure is to allow its use for other sortable things as well besides viewlets in the future, such as menu items. Yes, if you'd put the registration name in there it *would* become viewlet specific, but you'll need to give a good reason why this is useful first.

[Martijn Faassen (faassen)]

Anyway, all this means I continue to stand by my suggested simplification. :)

[Philipp von Weitershausen (philikon)]

On 16 May 2008, at 21:48 , Martijn Faassen wrote:
> Yes, if you'd put the registration name in there it *would*
> become viewlet specific, but you'll need to give a good reason why
> this
> is useful first.

I don't, because I didn't write the code, JW did :). I just critiqued
it...

Chickening-out-of-the-discussion-ly,
Philipp

[Martijn Faassen (faassen)]

Yes, and I'm happy you did, as your critique made me notice a point that resulted in my critique, which you then critiqued from the perspective of JW's code. Anyway, let's see what JW has to say. :)

[Jan Wijbrand Kolman (janwijbrand)]

It is late and I'm tired so I might very well be missing the point, however... I think you both overlook the fact that the sort() method is expected to return a list of (name, viewlet) tuples which util.sort_components() won't.

So, I have to jump through some hoops to get this list of (name, viewlet) tuples sorted just like util.sort_components() would sort the list of just viewlets.

If this explanation still makes sense that is.

Anyway, tomorrow I'll have a better look.

[Martijn Faassen (faassen)]

On Sat, May 17, 2008 at 12:19 AM, Jan Wijbrand Kolman
<email address hidden> wrote:
> It is late and I'm tired so I might very well be missing the point,
> however... I think you both overlook the fact that the sort() method is
> expected to return a list of (name, viewlet) tuples which
> util.sort_components() won't.

Ah, true, you are right. How annoying.

> So, I have to jump through some hoops to get this list of (name,
> viewlet) tuples sorted just like util.sort_components() would sort the
> list of just viewlets.

I think you should consider writing it like this instead:

s_viewlets = viewlets
for name, viewlet in viewlets:
     # stuff away viewlet name so we can later retrieve it
     viewlet.__viewlet_name__ = name
     s_viewlets.append(viewlet)

s_viewlets = util.sort_components(s_viewlets)

return [(viewlet.__viewlet_name__, viewlet) for viewlet in s_viewlets]

still ugly of course, but viewlet instances will never be persistent,
and are created on the fly each time you view them, so there's little
risk. It avoids the necessity of the 'index' call, which Python
implements by looping through sorted_components (and this is in an
inner loop, so there'll be lots of those loops). I don't think
performance-wise it would matter very much as the list of viewlets is
usually probably going to be small, but it's good to avoid such
potential scalability snags anyway. The code might also become
slightly easier to read. I didn't read the code very well before and
had no clue what was really going on, actually, silly me. Anyway, it
definitely needs a few comments, no matter what we end up writing;
this code is non-trivial either.

Note that there's a risk with both pieces of code: the same viewlet
instance might be in the list of viewlets twice. This would confuse
this procedure, as the name would be overwritten. It would also
confuse the .index() procedure, however. Perhaps a comment about this
risk should be in the code too.

[Jan Wijbrand Kolman (janwijbrand)]

On Sat, May 17, 2008 at 4:48 PM, Martijn Faassen <email address hidden> wrote:
> On Sat, May 17, 2008 at 12:19 AM, Jan Wijbrand Kolman
> <email address hidden> wrote:
>> It is late and I'm tired so I might very well be missing the point,
>> however... I think you both overlook the fact that the sort() method is
>> expected to return a list of (name, viewlet) tuples which
>> util.sort_components() won't.
>
> Ah, true, you are right. How annoying.

Yes, I know it's annoying when I am right for once ;)

But yes, this situation is annoying. Especially since it is not really
clear to me what the name part in the tuple is supposed to be. I would
need to to study the contentprovider/viewlet code a bit more to
understand the intentions there.

>> So, I have to jump through some hoops to get this list of (name,
>> viewlet) tuples sorted just like util.sort_components() would sort the
>> list of just viewlets.
>
> I think you should consider writing it like this instead:
>
> s_viewlets = viewlets
> for name, viewlet in viewlets:
> # stuff away viewlet name so we can later retrieve it
> viewlet.__viewlet_name__ = name
> s_viewlets.append(viewlet)
>
> s_viewlets = util.sort_components(s_viewlets)
>
> return [(viewlet.__viewlet_name__, viewlet) for viewlet in s_viewlets]

Yeah, I'll do it like that. I'll add comments all over the place as well.

Glad I had this code reviewed after all - something unconsciously made
me uneasy about it, I guess.

[Jan Wijbrand Kolman (janwijbrand)]

On Fri, May 16, 2008 at 7:01 PM, Martijn Faassen <email address hidden> wrote:
> I think you're correct about this bug. That said, I can't find an
> explicit specification that 'sort' is actually part of the
> IViewletManager interface. It's in the class, but not in the interface,
> at least on the trunk.
>
> If I didn't miss it, we should discuss on zope-dev whether we should add
> the sort() method to the interface of IViewletManager.

Ok.

[Jan Wijbrand Kolman (janwijbrand)]

Fix commited on the trunk and on the 0.12 branch.