You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bloodhound.apache.org by Apache Bloodhound <bl...@incubator.apache.org> on 2013/01/02 10:30:49 UTC
[Apache Bloodhound] #330: Lazy evaluation of req.chrome
#330: Lazy evaluation of req.chrome
------------------------+------------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: new
Priority: major | Component: dashboard
Version: | Resolution:
------------------------+------------------------
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: closed
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: fixed | Keywords:
--------------------------+--------------------
Comment (by olemis):
Replying to [comment:8 rjollos]:
> Replying to [comment:7 olemis]:
> > Notice that another check for `sys.exc_info` is needed (see [http
://trac-hacks.org/changeset/12540 this fix for ThemeEnginePlugin] for
further details) . Otherwise error pages seem not to get the proper
styling info .
> >
> > I was tempted to reopen this ticket , but I didn't . First we should
check whether error pages will be broken after r1437967 .
>
> I had noticed the other day that !TracError pages weren't rendering
correctly. I tested again today in an environment created last week, and
saw that the !TracError page was rendering correctly, but the theme had
gone away. I then created a new environment in order to get the latest
version of [trachacks:ThemeEnginePlugin] with Olemis' fixes, and now I see
something that looks nice:
>
[...]
yes , that's correct . I noticed the issue recently .
> I invoked the !TracError by replacing `data` with `None` in
`trac.wiki.web_ui._render_view`. I wanted to force an exception when the
template was rendered.
That's not the kind of errors we are tracking in this ticket . Indeed ,
its goal is not to instantiate `req.chrome` **IF** there is no template
processing involved .
{{{
#!div class="well"
{{{
#!span class="label label-info"
OT
}}}
AFAICT if there's an error rendering a Genshi template then you'll get a
plain-text error message . I guess that happens because the failure took
place at a lower layer (i.e. ''Genshi'' rendering engine)
}}}
> Are there other ways good ways to test this?
>
I usually just go to /ticket/whatever_that_fails ... I do not know if
that's good enough though .
[...]
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:9>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: new
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: | Keywords:
--------------------------+--------------------
Comment (by olemis):
Replying to [ticket:330 peter]:
> For optimal performance req.chrome should be evaluated only if
necessary.
> This currently violated on multiple places in request handlers, filters
and mainly by trachacks:TracThemeEngine plugin.
>
Could you please mention where is it exactly that
trachacks:ThemeEnginePlugin violates lazy instantiation of `req.chrome` ?
[...]
> However there has been no investigation on trachacks:TracThemeEngine
yet. Until this is fixed there is no benefits in fixing BH code at all.
I'm the maintainer of that plugin . I've not noticed something like that ,
so I ask : What's wrong exactly ?
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:2>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: closed
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: fixed | Keywords:
--------------------------+--------------------
Comment (by olemis):
Replying to [comment:12 andrej]:
> I can see the similar behavior when rendering an error page on local env
(rev 1439061). The error occurs when an exception is trigged during
request dispatching by handler.
>
[...]
>
> As far as I understand the code, the problem is in
bloodhound_theme/bhtheme/theme.py, line 217
>
[...]
So yes , this confirms my previous suspicion .
>
> In case of an exception during request processing, the
post_process_request method is called with all None parameters, so method
returns before {{{req.chrome['labels']}}} is filled.
>
That's right . That's what I was talking about .
> I think we need another way to figure out when we can skip theme post
processing. So far my suggestion is:
Did you read [comment:7 my previous comment] ? In there I mention how I
fixed exactly the same issue , but for trachacks:ThemeEnginePlugin .
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:13>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: closed
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: fixed | Keywords:
--------------------------+--------------------
Comment (by olemis):
Replying to [comment:6 peter]:
> Fix in r1437967
Notice that another check for `sys.exc_info` is needed (see [http://trac-
hacks.org/changeset/12540 this fix for ThemeEnginePlugin] for further
details) . Otherwise error pages seem not to get the proper styling info .
I was tempted to reopen this ticket , but I didn't . First we should check
whether error pages will be broken after r1437967 .
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:7>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: closed
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: fixed | Keywords:
--------------------------+--------------------
Comment (by olemis):
milestone = milestone:"Release 4" (not available as milestone seems to be
closed)
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:15>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: closed
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: fixed | Keywords:
--------------------------+--------------------
Comment (by olemis):
Replying to [comment:10 rjollos]:
> Replying to [comment:9 olemis]:
> > > I invoked the !TracError by replacing `data` with `None` in
`trac.wiki.web_ui._render_view`. I wanted to force an exception when the
template was rendered.
> >
> > That's not the kind of errors we are tracking in this ticket . Indeed
, its goal is not to instantiate `req.chrome` **IF** there is no template
processing involved .
>
> Your comment was ''Otherwise error pages seem not to get the proper
styling info ... First we should check whether error pages will be broken
after r1437967.'', therefore I thought that as a side issue we needed to
look at whether !TracError pages were rendering correctly.
Yes , you are right . There was a lot of implicit knowledge in my previous
statement .
Let's focus on `trac.web.chrome.Chrome.render_template` method . In there
`_post_process_request` is called in three places .
1. after request handler contributes any data to expand a given
template (invoked with args `template, data, content_type` )
2. when request handler sends data on its own
(invoked with `None, None, None` )
3. when an error is detected (invoked with `None, None, None`
inside `except` block)
`req.chrome` needs to be instantiated only in case **1** and **3** and
that's what this ticket is for . The way it is now , it's only taking into
account **1** . Basic theme style will be there , but further items added
while post processing the request will not . That was what I meant .
;)
> This seemed relevant since I had noticed recently that they were not
rendering correctly.
>
maybe yes , maybe not ...
> > AFAICT if there's an error rendering a Genshi template then you'll get
a plain-text error message.
>
> My test case results in a genshi template rendering error, and in that
case my screen capture shows that the !TracError page is rendered.
In that case `req.chrome` should be instantiated along the way .
> However, in other cases I only see a plain-text error message. I don't
understand what conditions lead to the different views.
Yes , I guess that depends on the exact point where the error happens e.g.
while expanding variables , you'll get the error page (... but you'll get
a plain text error if ''Genshi'' template cannot be found ? ) . It's a bit
non-deterministic ; or at least it is beyond my current understanding .
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:11>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: closed
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: fixed | Keywords:
--------------------------+--------------------
Comment (by rjollos):
Replying to [comment:9 olemis]:
> > I invoked the !TracError by replacing `data` with `None` in
`trac.wiki.web_ui._render_view`. I wanted to force an exception when the
template was rendered.
>
> That's not the kind of errors we are tracking in this ticket . Indeed ,
its goal is not to instantiate `req.chrome` **IF** there is no template
processing involved .
Your comment was ''Otherwise error pages seem not to get the proper
styling info ... First we should check whether error pages will be broken
after r1437967.'', therefore I thought that as a side issue we needed to
look at whether !TracError pages were rendering correctly. This seemed
relevant since I had noticed recently that they were not rendering
correctly.
> AFAICT if there's an error rendering a Genshi template then you'll get a
plain-text error message.
My test case results in a genshi template rendering error, and in that
case my screen capture shows that the !TracError page is rendered.
However, in other cases I only see a plain-text error message. I don't
understand what conditions lead to the different views.
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:10>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: closed
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: fixed | Keywords:
--------------------------+--------------------
Comment (by rjollos):
Replying to [comment:7 olemis]:
> Notice that another check for `sys.exc_info` is needed (see [http
://trac-hacks.org/changeset/12540 this fix for ThemeEnginePlugin] for
further details) . Otherwise error pages seem not to get the proper
styling info .
>
> I was tempted to reopen this ticket , but I didn't . First we should
check whether error pages will be broken after r1437967 .
I had noticed the other day that !TracError pages weren't rendering
correctly. I tested again today in an environment created last week, and
saw that the !TracError page was rendering correctly, but the theme had
gone away. I then created a new environment in order to get the latest
version of [trachacks:ThemeEnginePlugin] with Olemis' fixes, and now I see
something that looks nice:
[[Image(TracErrorAfterRevision1437967.png,100%)]]
I invoked the !TracError by replacing `data` with `None` in
`trac.wiki.web_ui._render_view`. I wanted to force an exception when the
template was rendered. Are there other ways good ways to test this?
Clicking the **Create** button leads to an interesting error:
[[Image(RequestUriTooLarge.png)]]
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:8>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: closed
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: fixed | Keywords:
--------------------------+--------------------
Comment (by peter):
The error page regression should now be fixed by r1439270
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:14>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: new
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: | Keywords:
--------------------------+--------------------
Comment (by olemis):
Replying to [comment:3 peter]:
> Replying to [comment:2 olemis]:
> > Replying to [ticket:330 peter]:
> > > For optimal performance req.chrome should be evaluated only if
necessary.
> > > This currently violated on multiple places in request handlers,
filters and mainly by trachacks:TracThemeEngine plugin.
> > >
> >
> > Could you please mention where is it exactly that
trachacks:ThemeEnginePlugin violates lazy instantiation of `req.chrome` ?
> >
[...]
>
> When the plugin is installed and active it's post_process_request() will
be invoked for every request. AFAICT there is nothing in that code that
would differentiate requests (API vs UI requests) so add_stylesheet() will
be called for all of them which then calls add_link() that evaluates
req.chrome.
>
I'll check that
;)
https://trac-hacks.org/ticket/10748
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:4>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: new
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: | Keywords:
--------------------------+--------------------
Comment (by peter):
Replying to [comment:2 olemis]:
> Replying to [ticket:330 peter]:
> > For optimal performance req.chrome should be evaluated only if
necessary.
> > This currently violated on multiple places in request handlers,
filters and mainly by trachacks:TracThemeEngine plugin.
> >
>
> Could you please mention where is it exactly that
trachacks:ThemeEnginePlugin violates lazy instantiation of `req.chrome` ?
>
> [...]
> > However there has been no investigation on trachacks:TracThemeEngine
yet. Until this is fixed there is no benefits in fixing BH code at all.
>
> I'm the maintainer of that plugin . I've not noticed something like that
, so I ask : What's wrong exactly ?
When the plugin is installed and active it's post_process_request() will
be invoked for every request. AFAICT there is nothing in that code that
would differentiate requests (API vs UI requests) so add_stylesheet() will
be called for all of them which then calls add_link() that evaluates
req.chrome.
There might be other cases as well (maybe in Trac itself also) but this is
the one I stumbled upon when I was investigating why does my solution for
lazy whitelabeling have no effect at all.
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:3>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: closed
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: fixed | Keywords:
--------------------------+--------------------
Comment (by andrej):
I can see the similar behavior when rendering an error page on local env
(rev 1439061). The error occurs when an exception is trigged during
request dispatching by handler. For example, use built-in search and set
page parameter that is out of range e.g.
http://localhost:8000/main/search?q=test&page=100
You've got a plain text error page indicating that there is an error in
{{{
... bloodhound_theme.html", line 278, in <Expression
u'chrome.labels.footer_left_prefix'>
...
has no member named "labels"
}}}
As far as I understand the code, the problem is in
bloodhound_theme/bhtheme/theme.py, line 217
{{{
def post_process_request(self, req, template, data, content_type):
"""Post process request filter.
Removes all trac provided css if required"""
if template is None and data is None:
return template, data, content_type
...
req.chrome['labels'] = self._get_whitelabelling()
}}}
In case of an exception during request processing, the
post_process_request method is called with all None parameters, so method
returns before {{{req.chrome['labels']}}} is filled.
Should we move line
{{{ req.chrome['labels'] = self._get_whitelabelling()}}}
before the if/return statement?
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:12>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: new
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: | Keywords:
--------------------------+--------------------
Comment (by olemis):
Replying to [comment:4 olemis]:
> Replying to [comment:3 peter]:
[...]
> >
> > When the plugin is installed and active it's post_process_request()
will be invoked for every request. AFAICT there is nothing in that code
that would differentiate requests (API vs UI requests) so add_stylesheet()
will be called for all of them which then calls add_link() that evaluates
req.chrome.
> >
>
> I'll check that
> ;)
>
> https://trac-hacks.org/ticket/10748
In order for you to move forward do you need anything beyond changes
committed in [https://trac-hacks.org/changeset/12508 th:changeset:12508] ?
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:5>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: new
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: | Keywords:
--------------------------+--------------------
Changes (by peter):
* priority: major => minor
Old description:
New description:
For optimal performance req.chrome should be evaluated only if necessary.
This currently violated on multiple places in request handlers, filters
and mainly by TracThemeEngine plugin.
BH's request handlers and filters could be fixed within BH (whitelabeling
solution was already dicussed on dev list). However there has been no
investigation on TracThemeEngine yet. Until this is fixed there is no
benefits in fixing BH code at all.
--
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:1>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker
Re: [Apache Bloodhound] #330: Lazy evaluation of req.chrome
Posted by Apache Bloodhound <bl...@incubator.apache.org>.
#330: Lazy evaluation of req.chrome
--------------------------+--------------------
Reporter: peter | Owner: nobody
Type: enhancement | Status: closed
Priority: minor | Milestone:
Component: dashboard | Version:
Resolution: fixed | Keywords:
--------------------------+--------------------
Changes (by peter):
* status: new => closed
* resolution: => fixed
Comment:
Fix in r1437967
--
Ticket URL: <https://issues.apache.org/bloodhound/ticket/330#comment:6>
Apache Bloodhound <https://issues.apache.org/bloodhound/>
The Apache Bloodhound (incubating) issue tracker