[Building Sakai] Portal NEO Prefix

[David Adams]

Chuck,

You misunderstand me. I agree with everything you say about the
appropriate place for this logic, and I understand all the potential
uses and implications you mention.

If I got the discussion off track by mentioning how many tools do it
wrong, I apologize. My intention was not to argue that the tools
should be building the CSS links themselves, but that the confusion
about how to do it correctly is extremely commonplace.

The thing Aaron, Steve, and I are asking to be fixed is the logic in
portal that takes the value the deployers set for "skin.default" and
changes it to something else. This has caused extreme confusion for
skin writers and app administrators.

The value of the skin.default property has always mapped directly to
the pathname of the CSS files. That direct relationship is no longer
working. I would call that a bug. A minor one that could be quickly
addressed.

In your first message in this thread, you seemed to be advocating for
waiting to fix this highly confusing but ultimately simple issue until
a total overhaul of the portal code could be wrangled. Waiting so long
will just cause more confusion. Every time someone moves to 2.9 and
wants to build their own skin, they will run into this confusion. Why
wait to fix it?

I understand there may be some reason why sites with custom skins need
this treatment, but there's no reason that should affect what the value
of "skin.default" means.

-dave

Charles Severance wrote:
> 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