incubator-bloodhound-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Olemis Lang <ole...@gmail.com>
Subject Re: [Whitelabeling] Quick assessment of (current) whitelabelling implementation .
Date Mon, 17 Dec 2012 18:08:59 GMT
On 12/17/12, Peter Koželj <peter@digiverse.si> wrote:
> On 13 December 2012 22:45, Olemis Lang <olemis@gmail.com> wrote:
>
>> Subjects to be discussed (IRC summary)
>>
>>   1. whethet it's better to use c = self.env.config ;
>> application_short = c.get('labels', 'application_short', "Bloodhound")
>> ; ... instead of trac.config.options
>>
>
> I have evaluated the config.options and as elegant as that is, there is no
> way to specify defaults that way.

No defaults ? Since the very first time I used Trac I noticed this

{{{
#!py

class Option(object):
    """Descriptor for configuration options."""

[...]

    def __init__(self, section, name, default=None, doc='',
                 doc_domain='tracini'):
        """Create the configuration option.

        @param section: the name of the configuration section this option
            belongs to
        @param name: the name of the option
        @param default: the default value for the option
        @param doc: documentation of the option
        """
}}}

Notice {{{default}}} argument. Isn't it just enough ? Am I missing something ?

> That would mean that we would have to
> change trac.ini during upgrade of older installations and is not as robust
> agains users mistakes when changing the config file manually.
>

If my comment above is accurate : no need to do nothing of this . This
is happening every day , every where in Trac

Maybe I didn't understand the initial statement about option defaults . CMIIW

>>   2. on the subject of where to put labels data maybe it should be
>> made available in request callbacks (just like req.chrome works at the
>> moment ;)
>>
>
> BH uses it's own templates from bloodhound_them together with original Trac
> request handlers. Current approach works for this combination without a
> need to change Trac codebase.
>

Believe me , I know ... and this has nothing to do with my suggestion
. I never even thought of patching Trac in any way . So , please
re-read my suggestion above and review in detail request chrome and
callbacks purpose and lifecycle .

> But we could move the labels dict from request.chrome to data inside
> BloodhoundTheme.post_process_request():
>

No .

> data['labels'] = self._get_whitelabelling()
> if that is considered as a more appropriate solution.
>

data is not always a dictionary ... and you might be overwriting real
data provided by a real request handler .

>
>>   3. there's another observation and it is to instantiate req. chrome
>> inside theme's post_process_request post only if
>>       * bloodhound theme is enabled and active
>>       * request handler actually needs it ... e.g. chrome should be
>>          useless most of the time for /rpc requests and alike .
>>          That's exactly why instantiation is lazy in first place .
>>
>>
> Sounds sensible but isn't "chrome" a Trac thingy?

well , indeed ... 90% is a Trac thing ;)

> AFAICT it is already
> there when request lifecycle methods form BloodhoundTheme are called.

yes and no ... it's lazy => instantiated on demand .

> Does
> moving labels from "chrome" to "data" help here in any way?
>

Please review the implementation of trac.web.chrome.Chrome.dispatch
method and request callbacks . Using data may lead to clashes with
other active request handlers

-- 
Regards,

Olemis.

Blog ES: http://simelo-es.blogspot.com/
Blog EN: http://simelo-en.blogspot.com/

Featured article:

Mime
View raw message