[Building Sakai] Portal NEO Prefix

[Charles Severance]

Dave,

It is wrong for any tool to look at "skin.default" and construct head material from it.  Period.  I am guessing that the tools you found are recent and they copied the pattern from each other as they were being organically grown and fixing them all be similar patterns.

The head material is rather complex actually and tools should do it right.

Take a look in the file SkinnableCharonPortal.java and find the method toolHeaderProperties() (line 1390)

If we are going to tolerate tools replicating portal functionality - they need to replicate *all* of the business logic in this function and the functions it calls and then when we have to make a dramatic change as we did for the new editor, we have to find all the places in the code that cut and pasted some variation of the portal head code that they grabbed over the past 10 years and update to all these rules.   Sometimes skins change on a site by site basis - do the tools replicate the code that figures this out in each tool?

Or do we simply demand that they just bite the bullet and take the head material from portal like they are supposed to?

I know the feeling as a tool writer.   The new iframe tool wanted to use the new editor and I was sooooo tempted (and spent several hours trying) to schlep in some code into the tool markup because I was in a hurry to make 2.9.2.  I reeeeeally wanted to do a view source on a tool that had the editor working and cut-and paste the HTML into the iframe-tool markup.    But I took a deep breath and fixed it right in portal so that it works perfectly for *all* JSR-168 portlets with no effort on the part of the portlet.    And then of course I need to cat-herd the code into 2-9-x before 2.9.2 or my new iframe portlet will fail - Hey Sam - could you merge these pretty soon? :) 

https://jira.sakaiproject.org/browse/SAK-23195
https://jira.sakaiproject.org/browse/SAK-23195

I know many tools run rather indie-ish and feel like they need to fix things in the scope of where they have commit access - I have commit in portal and things are even too slow for me most of the time.  I get it - sometimes people make the short-term choice that lets them move forward.  Our tool moves forward adding a bit of technical debt to the overall system - but just because it happens once in a while does not legitimize breaking abstractions.

One possible example where the abstraction for head material is essential might be a situation where a large hosting company wants to put all the CSS and Javascript in somewhere other than /reference - perhaps they want it on CloudFront.  If we do this properly, they could make a patch one place in the code of portal and instantly all the tools and portal would have CloudFront URLs - except of course those tools that are hand constructing URLs to system-side static content.  Highly scalable systems have ways of even changing the paths of their highly cached materials dynamically without a reboot in order to fix a CSS or JS bug that will be stuck in caches around the planet for 24 hours.

Another example would be a tool is running as an LTI provider and the CSS comes from a completely different system than the one the tool is running on and the tool needs not to be aware of this happening - it is just handed head material and uses it....

If we want some kind of new method to get head material in tools other than the current way we pass in head material - perhaps we could write a cool new static cover for this, that is cool - but it needs to be one place and not cutting and pasting upwards of 50 lines of complex and still evolving code over and over in each tool.  And it is much more than grabbing skin.default and constructing a /reference url.

Catching these mistakes is a *good thing* if we fix them properly and use them as a way to clean up abstraction violations.   It is *bad thing* if we decide that if seven tools do it wrong we will just define the "wrong way" as the "new right way" because it seems easier in the short run.  I want to (post 2.9.2) remove those old files completely and *only* have the neo- prefixed files so everything breaks and we identify and fix the abstraction breakers.  And then if someone wants a better skin than neo (hard to imagine this is possible) then we can call it mega- :)

Now if the use case is that tools have their *own* CSS and they want to build neo and non-neo versions of their own CSS - that makes a lot more sense.  Including a tool's *own* CSS is head material that *belongs to the tool* and is not head material that belongs to the portal.  They could look at the prefix and then have two CSS files in their own servlet that they switch between based on the prefix.  That would be cool - I hope it is not necessary - it is still within-tool technical debt if new skin sets happen - but it would not be an abstraction violation and the tool fully owns the problem and can build code that works across many skin sets if it so desires.

/Chuck

P.S. Because of the lack of abstraction, the simple upgrade to the rich text editor literally took about two years and had to be spread across 2.8 and 2.9 to undo all the technical debt / cruft that had crept into the code base.  Noah's first task was to make the editor switchable from one place and after that point, adding the new editor was pretty straightforward.  And based on the recent Melete 2.9.1 announcement (very cool) - Noah's task of getting tools to treat the editor as an abstraction continued after 2.9 was released and may continue for a while longer as the contribs come up to speed....

On Mar 16, 2013, at 10:32 AM, David Adams wrote:

> Charles Severance wrote:
>> Just schlepping the neo stuff back to the non-prefixed file names is a
>> poor solution so lazy tools that are out of spec don't have to fix
>> themselves is a mistake and adds more technical debt.
> 
> I don't think that's the problem. The problem is that tools and humans
> both are reasonably expecting that the value of "skin.default" is
> actually the skin the portal will offer up. The fix provided for
> STAT-345[1] is to just duplicate the logic from portal for building
> the portal name. Namely: fetching the skin.default, portal.templates,
> and portal.neoprefix properties, testing if portal.templates is
> "neoskin" and skin.default doesn't start with portal.neoprefix, and
> if so prepending portal.neoprefix to portal.skin.
> 
> [1] https://jira.sakaiproject.org/secure/attachment/35410/STAT-345.patch
> 
> That's an ugly hack even if it were in just one place in the code, but
> cloning it to other tools would be a far, far worse solution. Kind of
> like when every tool re-implemented the process for building a
> self-referential URL, only in multiple variations on three primary
> different methods?
> 
>> Because the names never changed for such a long time - folks could do
>> something quick and dirty but wrong in tools - lets not reward the
>> behavior.  Lets catch and fix the tools.
> 
> Sure, fix any tools that are doing it wrong (a quick grep of our 2.9.1
> code reveals eight tools as well as kernel all pull the "skin.default"
> property, none of them except login checks the portal.neoprefix or
> portal.templates properties as well).
> 
> Regardless of where the problem lies, code-wise, the current setup is
> confusing to humans and needs to be changed.
> 
> -dave

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://collab.sakaiproject.org/pipermail/sakai-dev/attachments/20130316/8d8e4482/attachment.html