[Zope-dev] [Checkins] SVN: zope.publisher/trunk/ Added some BBB code to setDefaultSkin

[Roger Ineichen]

Hi Hanno

Regards
Roger Ineichen
_____________________________
END OF MESSAGE
  

> -----Ursprüngliche Nachricht-----
> Von: Hanno Schlichting [mailto:hannosch at hannosch.eu] 
> Gesendet: Montag, 27. April 2009 09:16
> An: dev at projekt01.ch
> Betreff: Re: AW: [Checkins] SVN: zope.publisher/trunk/ Added 
> some BBB code to setDefaultSkin
> 
> Hi.
> 
> Roger Ineichen wrote:
> >> Log message for revision 99518:
> >>   Added some BBB code to setDefaultSkin to allow 
> IBrowserRequest's to 
> >> continue to work without configuring any special adapter for 
> >> IDefaultSkin. Lot's of code inside Zope2 relying on using 
> the request 
> >> object without tons of CA setup
> > 
> > I don't comment this.
> > 
> > but,
> > You broke my hole refactoring. My refactoring was making it 
> possible 
> > to use IDefaultBrowserLayer less IBrowserRequest.
> > 
> > Your changes apply now to every IBrowserRequest an 
> > IDefaultBrowserLayer where explicit was no default layer configured 
> > for. This is bad because and makes the previous refactoring 
> obsolate.
> > 
> > Can you revert this changes?
> 
> Then there's some tests and description missing proving that 
> intent. All I could read in the changelog and the skinnable 
> tests was pointing in the other direction: Making it possible 
> to use the Skinnable concept without relying on 
> IBrowserRequest. The whole JSONRequest (which is not a 
> BrowserRequest) tests inside skinnable.txt continue to work.

Yes, I think everything was tested, but probably future
ideas are not documented. There where some discussion about
to split each request into it's own packages.

> All I did here was to move two constructs from ZCML into direct code.
> The lines I added do exactly the same as the default adapter 
> registered as:

Uhh, this is call hard coded and makes it impossible to
exclude the adapter with the <exclude> directive.

> <adapter
>     name="default"
>     factory=".skinnable.getDefaultSkin"
>     for="zope.publisher.interfaces.browser.IBrowserRequest"
>     provides="zope.publisher.interfaces.IDefaultSkin"
>     />
> 
> Unless I got something wrong, no behavior was changed if you 
> load zope.publisher's configure.zcml. For Zope2 it is now 
> just possible to continue to ignore that configure.zcml as it 
> had done before.

I see two things which are bad. Skinnable depends now on
IBrowserRequest. I moved the skinnable concept out of the
browser request part. This allows us to separate the skinnable
and all different request types into own packages if we do future 
refactoring.

And each IBrowserRequest will get typed by a IDefaultBrowserLayer
which doesn't allow to have a default skin less browser request.

I still think we should revert this changes. The skinnable concept
is a base pattern which should not know about the different request
types.

Regards
Roger Ineichen

> Hanno
>