juliocamarero/liferay-portal — pull request #601 by epgarcia (2012-10-15 14:26:32+00:00)
Title: LPS-29796 Homogenise RSS settings and views

Hey Julio,

Thanks for your hints. I should have explained some of the decisions I took in my previous commit. It's never too late...

>There are some things which I don't understand in this pull... I see at least 4 times a pattern that I don't understand:

if ((delta != SearchContainer.DEFAULT_DELTA) || !displayStyle.equals(RSSUtil.DISPLAY_STYLE_FULL_CONTENT)) {
if (delta != SearchContainer.DEFAULT_DELTA) {
baseResourceURL.setParameter("max", String.valueOf(delta));
}

if (!displayStyle.equals(RSSUtil.DISPLAY_STYLE_FULL_CONTENT)) {
baseResourceURL.setParameter("displayStyle", displayStyle);
}
}

>Is there a reason why we need to check twice everything? I am wondering what is the benefit of this....

I took that pattern from the init.jsp of the Wiki and the Message Boards portlets while extracting the RSS logic from the portlets into the new taglibs. I couldn't find a benefit for the double-check neither, but since it appeared twice I decided to leave it so. As you consider it unnecessary too, I've finally removed the wrapping "if".

>Something that Brian has been asking me more and more is that we keep all the logic of the taglibs in the java class and make the jsps as simple as possible, so that they only do presentation and not logic. For the rss taglib, I tend to think all the logic about baseResourceURL and baseURL should be in the java class and then pass the attribute "url" to the jsp which would simple use it. The other taglib is ok.

@JorgeFerrer told me the same, but I had two reasons for leaving it that way. (1) This is how most taglibs work (I know it's not the best reason!) (2) The url is not unique, but it depends on the type and version of the feed. Thus, I need to build it in the for loop that renders the links for the different formats. I could have extracted the first part (adding delta and display format params to the URL) but I thought I'd be better to keep all the URL-param logic in the same place. If you still consider that this should be moved somewhere else, just tell me and I'll do it.

>Something that I also find strange is that now we have the new constants for FEED_FORMATS which contains actually a subset of all the formats we used to have (2 for atom and 3 for rss). In some places in the portal we still generate the rss formats based on these variables. I tend to think we should consolidate all this logic to use just one way (if you prefer the new one, we should probably replace the code where we were using the other to use this). I think it is mainly journal feeds.... edit_feed.jsp and if we do this, this should probably be a separate commit.

I hadn't changed anything related to Journal Feeds, since the logic here is different (control panel portlet, not using preferences). But I see what you mean and I have changed the way in which feed formats are obtained and process at edit_feed.jsp and EditFeedAction.java. I've also removed unused constants RSS_VERSIONS and ATOM_VERSIONS from RSSUtil, since they were used only in this case.

>Something I also don't understand is why we are using aui:field-wrappers instead of aui:inputs. aui:field-wrappers are our last resource for having labels for something which we don't know how to group, but they are not good for accessibility so we should try to avoid them as much as possible and use the other elements or aui:fieldsets

I know, but I think this is one of those cases where we have to use field-wrappers as last resource. I tried using aui-input, but in this case we're using the checkboxes to represent a literal value instead of a boolean one. Plus this literal values are to be managed as a comma-separated set of strings for backward compatibility (originally the rssFormat portlet preference was used to store one single value). With the code you suggested, you will end by having this in the preferences:

rssFormatfalse,false,atom10

It's actually the same case as for the conversion format checkboxes in the Asset Publisher configuration view, from where I took this pattern.

>Also, for obtaining the default String of arrays we could use GetterUtil (it was missing a method that I just added, so we should use that instead: getStringValues)

I also missed that method, thanks for adding it. As it's not yet in trunk, I've included your commit as a part of this PR.

Finally, I've splitted my changes in smaller commits and taken into account many of your suggestions and corrections (except for those explained above). Please check it and tell me if more changes are required.

Thanks!