[Yanel-dev] Patch: Make toolbar replaceable

[simon]

Am 10.11.2010 09:20, schrieb Michael Wechner:
> Hi Simon
>
> Your ideas sound great. Please turn them into patches ;-)
if i would do patches,  which will not be the case very soon. but it is 
on my list (there are items above this issue).
but i still would like to know what you think about following menu:

http://pupunzi.com/#mb.components/mb._menu/menu.html

cheers
simon
>
> I am confident that we can make the way how we integrate the toolbar 
> even more configurable,
> such that we can give people the choice how they want to do it (like 
> now or with a hook or whatever)
> and that by "evolution" a best practice will "materialize".
>
> Thanks
>
> Michael
>
> On 11/8/10 3:24 PM, simon wrote:
>> hi all
>>
>> thanks for the patch. last week i played around with the toolbar and 
>> had some ideas:
>>
>> - instead of inserting the toolbar structure into every html page. 
>> inventing a menu resource-type which would provide the menu structur. 
>> this way the structure could be used by an ajax menu which would 
>> probably be faster.
>> - yanel only inserting one javascript file, one css file and one 
>> hook-element into a html page. get rid of the inline style used by 
>> the toolbar.
>> - i think there are better menu scripts available [1] which are 
>> crossbrowser compatible without the quirks we use today for IE6
>> - the menu should delay the disappearance after the mouse leaves the 
>> menu. at the moment i's hard to navigate if the menu has many levels.
>> - a contract between menu and resource-types so that resource-types 
>> can add menu items. resp the menu asks the current resource-type for 
>> menu items. e.g. Menuable
>>
>> [1]http://www.dynamicdrive.com/dynamicindex1/ddsmoothmenu.htm
>> http://pupunzi.com/#mb.components/mb._menu/menu.html
>>
>>
>> simon
>>
>> Am 08.11.2010 11:08, schrieb 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
>>>
>>
>