Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1877 (closed defect: fixed)

Opened 11 years ago

Last modified 11 years ago

[PATCH] When controller returns list of widgets, widget JS and CSS are not loaded

Reported by: mw44118 Owned by: chrisz
Priority: normal Milestone:
Component: TurboGears Version: 1.0.4.4
Severity: normal Keywords:
Cc:

Description

I have a controller like this:

    @expose('.templates.employee.search')
    def search(self, **kwargs):
        return dict(f1=forms.emailsearchform, f2=forms.namesearchform)

My template is really boring:

${f1.display()}
${f2.display()}

Here's the (possible) bug. I changed my controller to return a list of widgets, rather than separate widgets, like this:

    @expose('.templates.employee.search')
    def search(self, **kwargs):
        return dict(flist=[forms.emailsearchform,
forms.namesearchform])

And I changed my template to look like this:

<div py:for="f in flist">${f.display()}</div>

I notice that in the second method, my template does not get the required widget javascript files added to the page <head> section.

I read _process_output in turbogears.controllers and discovered that it tests each value in the dictionary returned by my controller for some attributes. Since the list I return doesn't have these attributes, the javascript isn't added.

Below is a simple patch for the problem. I also attached the patch to this ticket.

$ svn diff
Index: controllers.py
===================================================================
--- controllers.py      (revision 4879)
+++ controllers.py      (working copy)
@@ -60,6 +60,25 @@
             css.add_all(widget.retrieve_css())
 
         for value in output.itervalues():
+
+            # Look through any lists to see if they contain widgets.
+            if isinstance(value, (list, tuple)):
+                for element in value:
+                    if hasattr(element, "retrieve_css"):
+                        retrieve = getattr(element, "retrieve_css")
+                        if callable(retrieve):
+                            css.add_all(element.retrieve_css())
+
+
+            # Look through any returned dictionaries to see if they contain
+            # widgets.
+            if isinstance(value, dict):
+                for v in value.itervalues():
+                    if hasattr(v, "retrieve_css"):
+                        retrieve = getattr(v, "retrieve_css")
+                        if callable(retrieve):
+                            css.add_all(v.retrieve_css())
+
             if hasattr(value, "retrieve_css"):
                 retrieve = getattr(value, "retrieve_css")
                 if callable(retrieve):

Attachments

tg-patch Download (2.2 KB) - added by mw44118 11 years ago.
better patch. this one adds javascript and CSS to pages.
widgetslist.patch Download (559 bytes) - added by chrisz 11 years ago.
Patch for simpler WidgetsList? instantiation.
widgetslist_2.patch Download (1.3 KB) - added by Chris Arndt 11 years ago.
Combination of ChrisZ's patch and Alberto's suggestion

Change History

Changed 11 years ago by mw44118

better patch. this one adds javascript and CSS to pages.

comment:1 Changed 11 years ago by mw44118

  • Summary changed from When controller returns list of widgets, widget JS and CSS are not loaded to [PATCH] When controller returns list of widgets, widget JS and CSS are not loaded

comment:2 Changed 11 years ago by mw44118

A few improvements could be made to this patch.

  1. It duplicates the hasattr...getattr code several times. It would be nicer to move this stuff out into a freestanding function, or maybe a method on widgets.
  1. What about lists of lists or dictionaries of lists? The function might be better if it recursed to make sure that all elements to the very bottom are examined.

comment:3 Changed 11 years ago by chrisz

Looks good, but can become a bit expensive since you often pass huge result lists to the controller. Maybe use WidgetList for passing lists of Widgets and check only for these kind of lists?

comment:4 Changed 11 years ago by Chris Arndt

I agree with Chris. Checking each list and dict in every controller output is too expensive. And if we do this, why only check the first level depth? This seems like an arbitrary decision.

If your problem is that it's too cumbersome to add all the widgets to the dict returned by the controller, you might use something like this:

def mypage(self):
    # this is the list of widgets
    widgets = ...
    return dict([
       ('widget%i' % i, widget)
          for i, widget in enumerate(widgets)])

So, -1 on this patch. +-0 on the idea of using WidgetList as a container.

comment:5 Changed 11 years ago by mw44118

Chris Arndt: I want to share one template with several controllers. One controller might return two widgets, and the other might return six widgets. So in the template, I want to iterate across the number of widgets and display each.

I don't think your solution will work for that use case, will it?

I don't see the check across each element as that expensive. Even with a few hundred elements, I doubt that the cost would be as large as, say, the cost of rendering the template itself, but I'm speculating here.

comment:6 Changed 11 years ago by Chris Arndt

I knew you would say this! ;-) Here's a quick hack to do this:

def mypage(self):
    # this is the list of widgets
    widgets = ...
    data = dict([
       ('widget%i' % i, widget)
          for i, widget in enumerate(widgets)])
    data['num_widgets'] = len(widgets)
    data['widgets_basename'] = 'widgets'
    return data
<div py:for="i in range(num_widgets)" py:strip="">
  <div py:replace="value_of(widgets_basename + i).display()" />
</div>

comment:7 Changed 11 years ago by Chris Arndt

Sorry, that should be value_of(widgets_basename + str(i)).

comment:8 Changed 11 years ago by mw44118

I didn't know about value_of or defined. Thanks Chris! Yes, these are fine. However, I think it's worth mentioning that any time somebody puts a widget in a list, then this issue emerges.

comment:9 Changed 11 years ago by Chris Arndt

Yes, but this behavior is documented:

 http://docs.turbogears.org/1.0/WidgetsWithJSAndCSS#how-does-it-work

(I slightly reworded the important sentence there now and added some emphasis.)

I would support a patch that replaces the isinstance(value, (list, tuple)) part with one that checks for isinstance(value, widgets.WidgetList) and doesn't check in dictionaries.

Chris

comment:10 Changed 11 years ago by chrisz

Just noticed that currently you can't simply write

return dict(flist=WidgetsList(forms.emailsearchform, forms.namesearchform))

since WidgetsList allows only class declaration. But this could be fixed easily (see patch) and the above would be a simple and readable alternative.

Changed 11 years ago by chrisz

Patch for simpler WidgetsList? instantiation.

comment:11 Changed 11 years ago by alberto

The cleanest way I can think of to do this without the need to patch TG and no extra performance impact when it is not used is to write a widget container like this:

# Bunch is in turbogears.util IIRC

class WidgetBunch(Bunch):
    # Assumes all attributes that will be bound are actually Widgets (ie: have retrieve_* meths)
    def retrieve_javascript(self):
        return map(widget.retrieve_javascript, self.__dict__.itervalues())
    def retrieve_css(self):
        return map(widget.retrieve_css, self.__dict__.itervalues())

# usage
def some_controller(...):
    widgets = WidgetBunch()
    widgets.form = form
    widgets.sidebar = sidebar
    return dict(widgets=widgets)

<plug> Or switch to ToscaWidgets? which can insert resources from any widget that is actually displayed without the need to scan the output dict... and has more docs than ever ;) </plug>

Alberto

comment:12 Changed 11 years ago by alberto

Oops, code was buggy.

from itertools import chain

# Bunch is in turbogears.util IIRC
class WidgetBunch(Bunch):
    # Assumes all attributes that will be bound are actually Widgets (ie: have retrieve_* meths)
    def retrieve_javascript(self):
        return chain(map(widget.retrieve_javascript, self.__dict__.itervalues()))
    def retrieve_css(self):
        return chain(map(widget.retrieve_css, self.__dict__.itervalues()))

Alberto

comment:13 Changed 11 years ago by alberto

Ok, now for real:

from itertools import chain, imap
from turbogears.widgets import Widget

class WidgetBunch(object):
    # Assumes all attributes that will be bound are actually Widgets (ie: have retrieve_* meths)
    def retrieve_javascript(self):
        return chain(imap(Widget.retrieve_javascript, self.__dict__.itervalues()))
    def retrieve_css(self):
        return chain(imap(Widget.retrieve_css, self.__dict__.itervalues()))

And what the OP wanted (a list):

class WidgetList(list):
    # Assumes all attributes that are in the list are actually Widgets
    def retrieve_javascript(self):
        return chain(imap(Widget.retrieve_javascript, self))
    def retrieve_css(self):
        return chain(imap(Widget.retrieve_css, self))

Alberto

comment:14 Changed 11 years ago by alberto

BTW, last example was broken too (note to myself: goto bed;), chain concatenates *positional args, so it should read: chain(*map(...)) Alberto

comment:15 follow-up: ↓ 17 Changed 11 years ago by mw44118

Does everyone else really believe that testing every element in a list is really unacceptably expensive?

I really, really doubt people will be passing in lists with more than a dozen elements. I understand that theoretically somebody could pass in a list of a million elements, and scanning that would be expensive. I don't believe, that this is a real-world use case.

If it isn't really so expensive, I vote for the original approach. I feel like this approach of defining a new widget bunch increases the complexity of the code in order to avoid a problem (scanning too many list elements) that is a not a real-world problem.

I'll happily go along with the community decision, but I think we should explore if we're building a solution to a problem that will never surface.

comment:16 follow-up: ↓ 18 Changed 11 years ago by Chris Arndt

Yes, I believe that. I propose a patch that is a combination of ChrisZ's and Alberto's (see attachment widgetslist_2.patch). With this, you could do this:

def mypage(self):
    return dict(widgets=widgets.WidgetsList(widget1, widget2, 'evensomethingelse'))

Changed 11 years ago by Chris Arndt

Combination of ChrisZ's patch and Alberto's suggestion

comment:17 in reply to: ↑ 15 ; follow-up: ↓ 19 Changed 11 years ago by alberto

Replying to mw44118:

Does everyone else really believe that testing every element in a list is really unacceptably expensive?

I really, really doubt people will be passing in lists with more than a dozen elements. I understand that theoretically somebody could pass in a list of a million elements, and scanning that would be expensive. I don't believe, that this is a real-world use case.

If it isn't really so expensive, I vote for the original approach. I feel like this approach of defining a new widget bunch increases the complexity of the code in order to avoid a problem (scanning too many list elements) that is a not a real-world problem.

We really have different opinions on what is "complex" :) tg-patch adds 34 lines to a hot execution path which taxes almost all tg responses (all that return a dict). while the solution I proposed is only 5 lines which only tax (minimally) those who actually use it.

Alberto

comment:18 in reply to: ↑ 16 Changed 11 years ago by alberto

Replying to Chris Arndt:

Yes, I believe that. I propose a patch that is a combination of ChrisZ's and Alberto's (see attachment widgetslist_2.patch). With this, you could do this:

def mypage(self):
    return dict(widgets=widgets.WidgetsList(widget1, widget2, 'evensomethingelse'))

These patches combine better than black chocolate and red wine! :) +1

Alberto

comment:19 in reply to: ↑ 17 Changed 11 years ago by mw44118

Replying to alberto:

We really have different opinions on what is "complex" :) tg-patch adds 34 lines to a hot execution path which taxes almost all tg responses (all that return a dict). while the solution I proposed is only 5 lines which only tax (minimally) those who actually use it.

Good point. I'm happy with the new approach. Thanks for the feedback.

comment:20 Changed 11 years ago by chrisz

I also like the last patch. However, my original patch allowed you to initialize the WidgetsList with an ordinary list as well, i.e. WidgetsList([w1,w2]) and WidgetsList(w1, w2) were both possible. While the second is easier to write, the first is more what you expect from a subclass of list, so I allowed both.

@mw44118: You could imagine a list to be displayed in a grid, but the database doesn't support OFFSET and LIMIT, so the whole list is passed around with maybe thousands of objects even when it's not displayed completely. Something like that.

Another point to consider: If somebody passes around a lot of widgets in a dictionary, that does not mean that he intends to display them all on the actual page. Maybe there are "if" conditions or they are all passed for convenience reasons. So only checking the top level of variables and documenting this is ok I think. (Though the ToscaWidgets? solutions sounds even better. I really want to take a look at these, now that there are better docs.)

@ChrisA: Will you commit the patch? (Dont' forget to add a unit test and mention this in the docs). If you haven't enough time, let me know, and I will go for it.

comment:21 Changed 11 years ago by Chris Arndt

  • Owner changed from anonymous to Chris Arndt

@chrisz: you're right, as a list-like class WidgetsList's constructor should accept a single list argument as well. I'll apply the patch with this modification and also write test and docs.

comment:22 Changed 11 years ago by Chris Arndt

  • Owner changed from Chris Arndt to chrisz

@chrisz Turns out that I didn't have the time to test and apply this patch yet. If you have time to do it, I would appreciate if you could handle it.

comment:23 Changed 11 years ago by chrisz

  • Status changed from new to closed
  • Resolution set to fixed

Ok, I've added some tests and checked this in as r5190, also updated the Wiki.

Note: See TracTickets for help on using tickets.