[Building Sakai] SiteService.getSiteSkin and neoportal

[Steve Swinsburg]

Is adding one method to the API really that much bloat when every tool that needs to handle a null has to add code to deal with the value?

This is a pretty moot argument anyway. The method in question is used in these locations:

metaobj/metaobj-util/tool-lib/src/java/org/sakaiproject/metaobj/shared/control/SakaiStyleSheetInterceptor.java
portal/portal-impl/impl/src/java/org/sakaiproject/portal/charon/CharonPortal.java
portal/portal-impl/impl/src/java/org/sakaiproject/portal/charon/handlers/SiteHandler.java
portal/portal-impl/impl/src/java/org/sakaiproject/portal/charon/SkinnableCharonPortal.java
webservices/axis/src/webapp/SakaiScript.jws
webservices/axis/src/webapp/SakaiScriptNet.jws


https://jira.sakaiproject.org/browse/KNL-960

cheers,
Steve




On 05/09/2012, at 10:43 AM, Aaron Zeckoski <azeckoski at unicon.net> wrote:

> I disagree. The javadocs in much of the kernel are inaccurate and if
> we changed all the APIs to match the javadocs we would make life hell
> for most of the developers in this project. The current behavior is
> the API and the javadocs should be switched instead of doing it the
> other way around (at least, this is what we have done in the past when
> this has come up).
> 
> Either way, like I said, create a JIRA and let's get it on the topics
> for the CLE team meeting this week.
> 
> -AZ
> 
> 
> On Tue, Sep 4, 2012 at 8:29 PM, Steve Swinsburg
> <steve.swinsburg at gmail.com> wrote:
>> If those are the javadocs then that is what it should do, so it should
>> return the default skin specified via skin.default
>> 
>> Query re the patch. Should the neo- be specifically prepended? Can't we just
>> have this:
>> 
>> skin.default=neo-default
>> 
>> cheers,
>> Steve
>> 
>> 
>> 
>> On 05/09/2012, at 1:46 AM, Adrian Fish <adrian.r.fish at gmail.com> wrote:
>> 
>> 
>> 
>>> 
>>> 1) We generally do not change API behavior unless there is a really
>>> good reason. In this case the change results in a loss of information
>>> (i.e. there would no longer be a way to determine if a site has no
>>> skin set and therefore is using the default skin, in other words,
>>> assigning a site the same skin as the current default one and having
>>> no skin assigned will appear to be the same).
>> 
>> 
>> Is anybody actually using it in that way? The docs for getSiteSkin state
>> this:
>> 
>> 'Compute the skin to use for the (optional) site specified in the id
>> parameter. If no site specified, or if the site has no skin defined, use the
>> configured default skin.'
>> 
>> This implies it should always return something whereas under neoportal it
>> returns null.
>> 
>> Cheers,
>> Adrian.
>> 
>>> 
>>> 
>>> 
>>> On Tue, Sep 4, 2012 at 11:05 AM, Adrian Fish <adrian.r.fish at gmail.com>
>>> wrote:
>>>> Hi Aaron,
>>>> 
>>>> I don't know if there is a JIRA.
>>>> 
>>>> This isn't changed behaviour; I think it's always behaved like that.
>>>> I've
>>>> basically migrated a 2.8 db to 2.9.x and set all of our explicitly
>>>> skinned
>>>> sites to blank skins. In other words they should get the default now and
>>>> in
>>>> most cases they do perfectly well. The problem lies with tool logic that
>>>> calls getSiteSkin and expects to get something other than null, which,
>>>> in a
>>>> system where a default skin always applies, seems reasonable. I've only
>>>> spotted this since removing the explicit skins from sites.
>>>> 
>>>> Obviously, there would be ramifications for tools that handle the
>>>> possibility of null being returned if getSiteSkin respected the default
>>>> skin
>>>> and returned it.
>>>> 
>>>> Cheers,
>>>> Adrian.
>>>> 
>>>> 
>>>> On 4 September 2012 15:44, Aaron Zeckoski <azeckoski at unicon.net> wrote:
>>>>> 
>>>>> Adrian,
>>>>> Is there a JIRA related to this?
>>>>> 
>>>>> Also, this doesn't sound like changed behavior but I could be mistaken
>>>>> there. Are you saying this used to be the case but that it changed and
>>>>> if so, when do you think it changed?
>>>>> -AZ
>>>>> 
>>>>> 
>>>>> On Tue, Sep 4, 2012 at 10:16 AM, Adrian Fish <adrian.r.fish at gmail.com>
>>>>> wrote:
>>>>>> When setSiteSkin is called it returns null if a skin has not been
>>>>>> explicitly
>>>>>> set on a site. Is this the expected behaviour? Shouldn't it return
>>>>>> the
>>>>>> name
>>>>>> of the default skin as set by the skin.default property? Shouldn't it
>>>>>> also
>>>>>> prefix the skin with 'neo-' if 'portal.templates=neoskin' is set?
>>>>>> 
>>>>>> The reason I'm asking is that I've always depended on getSiteSkin
>>>>>> returning
>>>>>> the actual skin name that is in use; now I've stopped explicitly
>>>>>> specifying
>>>>>> skins on sites, tools like YAFT, CLOG and ROSTER2 don't get styled.
>>>>>> It
>>>>>> seems
>>>>>> odd having to implement logic in a tool to massage the skin
>>>>>> appropriately
>>>>>> when it could happen in getSiteSkin. SiteStats also implements some
>>>>>> logic to
>>>>>> sort the skin name out.
>>>>>> 
>>>>>> I've attached a patch which mods BaseSiteService in the way I
>>>>>> describe.
>>>>>> 
>>>>>> Cheers,
>>>>>> Adrian.
>>>>>> 
>>>>>> _______________________________________________
>>>>>> sakai-dev mailing list
>>>>>> sakai-dev at collab.sakaiproject.org
>>>>>> http://collab.sakaiproject.org/mailman/listinfo/sakai-dev
>>>>>> 
>>>>>> TO UNSUBSCRIBE: send email to
>>>>>> sakai-dev-unsubscribe at collab.sakaiproject.org
>>>>>> with a subject of "unsubscribe"
>>>>> 
>>>>> 
>>>>> 
>>>>> --
>>>>> Aaron Zeckoski - Software Architect - http://tinyurl.com/azprofile
>>>> 
>>>> 
>>> 
>>> 
>>> 
>>> --
>>> Aaron Zeckoski - Software Architect - http://tinyurl.com/azprofile
>> 
>> 
>> _______________________________________________
>> sakai-dev mailing list
>> sakai-dev at collab.sakaiproject.org
>> http://collab.sakaiproject.org/mailman/listinfo/sakai-dev
>> 
>> TO UNSUBSCRIBE: send email to sakai-dev-unsubscribe at collab.sakaiproject.org
>> with a subject of "unsubscribe"
>> 
>> 
> 
> 
> 
> -- 
> Aaron Zeckoski - Software Architect - http://tinyurl.com/azprofile