The unspeakable horror of UserPreferences

[Andrew Jaquith <ar...@mac.com>]

Hi all --

I'm working my way through the various JSPs, in an effort to make them  
a little more Stripes-friendly. The side-effect of re-working the JSPs  
is that I get to do a little code-reviewing while I'm at it.

I've been looking at PreferencesTab.jsp in particular. It's a  
little... complex. When about 3/4 of a JSP is scriptlet code, that  
tends to suggest an opportunity to refactor.

Taking a look at the code, I've got a few reactions and  
recommendations -- in order of priority.

First, I'd like to see the various user preferences bits become part  
of a proper Java class, rather than continue to get bigger and bigger  
as scriptlet code in the JSPs. As happens, the preferences themselves  
are just String key/value pairs.

It seems to me that the logical thing to do is to make it a Map that  
is returned (or settable) via something like WikiSession. Thus, in  
JSPs or other classes, all you'd need to do is grab the WikiSession  
reference and know that you could also retrieve the user prefs easily.  
Doing it this way would also eliminate the need to store the  
preferences in the session directly. This would also mean we could  
dismantle the Preferences class and package and move its functions  
into UserManager and SessionMonitor.

Recommended priority: for 2.8.

Second, we should start using JSP expression language more for prefs,  
and use 'injection' techniques via filters to make things like  
preferences available as variables. In particular, methods that must  
execute on "all pages," and only execute once, are candidates for  
injection. Example: Preferences.setupPreferences is called ONLY ONCE  
in a JSP that all pages load: commonheader. It would be better to  
simply use WikiServletFilter to inject the preferences Map as "$ 
{wikiPreferences}" or something similar.

Recommended priority: for 2.8.

Third, a long (long!) time ago, Janne and I discussed adding some  
capabilities to the UserManager/UserDatabase classes that would allow   
persistent storage of preferences. To keep things flexible, I think  
the best way to store these things is to simply create an additional  
table (in the JDBC case) that would stash key/value pairs. In the XML  
case, we could add 'pref' elements as children of each 'user' element.  
Doing it this way would enable us to store an unlimited number of  
preferences, for anything we could imagine. I suspect Dirk can imagine  
quite a lot of things. :)

Recommended priority: for 3.0.

Lastly, I'd like to understand the JSON side of things a bit more.  
What are we doing with it?

I'd like to get the dev team's thoughts on all this. Before I do ANY  
of this, I've got to get the Selenium webtests working, so that's  
priority 1.

Andrew

Re: The unspeakable horror of UserPreferences

[Dirk Frederickx <di...@gmail.com>]

On Mon, Feb 25, 2008 at 7:56 AM, Janne Jalkanen
<Ja...@ecyrd.com> wrote:
> > First, I'd like to see the various user preferences bits become
>  > part of a proper Java class, rather than continue to get bigger and
>  > bigger as scriptlet code in the JSPs. As happens, the preferences
>  > themselves are just String key/value pairs.
>

Actually, I have done already some refactoring on my local jspwiki to
move all scriptlets from the preferences tab to the
TemplateManager.class.
Just waiting for 2.7.xx  to add this stuff.

>  > Third, a long (long!) time ago, Janne and I discussed adding some
>  > capabilities to the UserManager/UserDatabase classes that would
>  > allow  persistent storage of preferences. To keep things flexible,
>  > I think the best way to store these things is to simply create an
>  > additional table (in the JDBC case) that would stash key/value
>  > pairs. In the XML case, we could add 'pref' elements as children of
>  > each 'user' element. Doing it this way would enable us to store an
>  > unlimited number of preferences, for anything we could imagine. I
>  > suspect Dirk can imagine quite a lot of things. :)

yeah yeah some more is coming :-D


dirk

Re: The unspeakable horror of UserPreferences

[Janne Jalkanen <Ja...@ecyrd.com>]

> First, I'd like to see the various user preferences bits become  
> part of a proper Java class, rather than continue to get bigger and  
> bigger as scriptlet code in the JSPs. As happens, the preferences  
> themselves are just String key/value pairs.

Not "just".  In order not to have another unspeakable horror like any  
MBean tool out there, you have to mind the user experience and  
actually design the layout a bit.

But yeah, loads of scriptlets mean that it might be better to move  
them to a PreferencesBean or something.

> It seems to me that the logical thing to do is to make it a Map  
> that is returned (or settable) via something like WikiSession.  
> Thus, in JSPs or other classes, all you'd need to do is grab the  
> WikiSession reference and know that you could also retrieve the  
> user prefs easily. Doing it this way would also eliminate the need  
> to store the preferences in the session directly. This would also  
> mean we could dismantle the Preferences class and package and move  
> its functions into UserManager and SessionMonitor.

Well, Preferences *is* a Map.  Why should we deprecate it in favour  
of another Map?  It's essentially just a Map with a number of  
convenience methods...

I also think that it is good practice to keep preferences a separate  
entity as not to pollute and bloat other classes.

> Second, we should start using JSP expression language more for  
> prefs, and use 'injection' techniques via filters to make things  
> like preferences available as variables. In particular, methods  
> that must execute on "all pages," and only execute once, are  
> candidates for injection. Example: Preferences.setupPreferences is  
> called ONLY ONCE in a JSP that all pages load: commonheader. It  
> would be better to simply use WikiServletFilter to inject the  
> preferences Map as "${wikiPreferences}" or something similar.

Probably true.  On the other hand, commonheader.jsp *is* meant for  
places where common things are set up.  It's really a question of  
whether this is something which should be allowed to be varied from  
one template to another (probably not).  If it's meant to be common  
across all templates, it should be injected.  If it's something that  
we can think of a template writer want to change, then it should be  
in commonheader.jsp.

> Third, a long (long!) time ago, Janne and I discussed adding some  
> capabilities to the UserManager/UserDatabase classes that would  
> allow  persistent storage of preferences. To keep things flexible,  
> I think the best way to store these things is to simply create an  
> additional table (in the JDBC case) that would stash key/value  
> pairs. In the XML case, we could add 'pref' elements as children of  
> each 'user' element. Doing it this way would enable us to store an  
> unlimited number of preferences, for anything we could imagine. I  
> suspect Dirk can imagine quite a lot of things. :)

Ah, but there are some limitations to this - some preferences need to  
be kept browser-specific as well (e.g. my mobile phone browser likes  
slightly different settings).

We can do this in two ways

a) have a separate JCR tree for prefrences (/wiki:prefs/<username>/ 
fontsize, /wiki:prefs/<username>/skin), or
b) have a subnode under the user's own page (which would be  
automatically created), e.g. /wiki:pages/<users page>/wiki:prefs/ 
fontsize, etc

> Lastly, I'd like to understand the JSON side of things a bit more.  
> What are we doing with it?

All in all or just in preferences context?

/Janne (I'll be off to Linz, Austria for the next few days so don't  
expect a speedy response...)