[Yanel-dev] Patch: Make toolbar replaceable

[Michael Wechner]

Hi Rob

On 11/5/10 3:50 PM, Rob Adamson wrote:
> Hi Michael,
>
> Thanks for reviewing. I've made some changes, and here's a new patch.

thanks very much
> Note that I want to check I can implement our new toolbar with this,
> so please review it but maybe don't commit it just yet.

I have commited it now and added a sample custom toolbar (based on your 
DefaultToolbar)

src/realms/from-scratch-realm-template/src/java/org/wyona/yanel/servlet/toolbar/impl/FromScratchRealmToolbar.java

For the moment we just don't announce this officially, hence my WARNING 
to everybody please
use this for the moment at your own risk ;-)


> Note that one thing I've done in the DefaultYanelToolbar is to catch
> non-RuntimeExceptions and wrap them in YanelToolbarExceptions.
> Throwing and catching Exception is a rather bad idea, in general, as
> it is the worst of both worlds (checked&  unchecked) - the programmer
> gets no extra information as to what may be thrown, but still has to
> catch all Exceptions or declare that they may be thrown.

but do you log the original exceptions?


> Anyway, more comments inline below...
>
> On 5 November 2010 08:57, Michael Wechner<michael.wechner at wyona.com>  wrote:
>> On 11/4/10 7:28 PM, Rob Adamson wrote:
>>> It re-uses the<menu>    entry in the realm configuration - if the class
>>> implements the new interface YanelToolbar, this is used, otherwise the
>>> DefaultYanelToolbar is used and the<menu>    entry is treated as a
>>> subclass of org.wyona.yanel.servlet.menu.Menu.
>>
>> I guess this is done within
>>
>> src/webapp/src/java/org/wyona/yanel/servlet/YanelHTMLUI#getYanelToolbar(Resource
>> resource)
>>
>> right?
>>
>> Maybe it would make sense to introduce a new tag called<toolbar/>, but we
>> can always do that later.
> I did think of that, but changing the Realm interface would break
> compatibility :)
> Hence overriding<menu>.

agreed. As soon as we have consolidated your enhancements and got some 
more experience with
it we can explain this within the documentation


>>> I had to give YanelServlet.getIdentity public visibility  (previously
>>> it was package visibility).
> I've done away with this change. Calling
> resource.getRealm().getEnvironment().getIdentity() seems to do the job
> just as well.
>
>> I have three requests in order to commit the patch more easily:
>>
>> 1) Please change the formatting: 4 spaces instead 1 tab
> OK
>
>> 2) Please add some javadoc to the new methods
> OK
>


thanks very much
>> 3) Please create a simple minimal toolbar custom implementation for the
>> from-scratch-realm which we could
>> use as example:
>> https://svn.wyona.com/repos/public/yanel/trunk/src/realms/from-scratch-realm-template/src/java
>>
>> to show the difference between the Default implementation and a custom
>> implementation
> I haven't got time to do this. You will see the implementation in my
> realm, and perhaps you/I could create an example implementation from
> that at a later date.

I have done a "lazy" version to at least test the loading

src/realms/from-scratch-realm-template/src/java/org/wyona/yanel/servlet/toolbar/impl/FromScratchRealmToolbar.java

but need to make it a bit more different, such that one can actually see 
a difference.

Simon has told me off the list that he did some improvements recently 
and I will ask
him to implement these within this implementation.

Thanks again for your patch and please continue :-)

Michael
> Kind regards,
>
> Rob