Hi Sergiu,
> Since you only committed in the trunk, should this be @since 1.9M1? The
> same for all the other @since tags.
Merged with 1.8 branch today.
> This is good for a standalone Java program. When it comes to webapps,
> the lifecycle is much more complex, since a webapp can be stopped and
> reloaded at will, in a clustered environment it can be moved from one
> computer to another, or it can even be stopped from within the
> application itself. This code is good for the moment, but it will have
> to be changed as soon as we introduce lifecycle events in the
> observation component.
>
I agree :)
Creating the bridge for each new context is a waste of CPU and RAM.
> Since the bridge is stateless, it can be a member initialized in the
> constructor.
>
> > + context.put(VELOCITY_CONTEXT_KEY, new
> OpenOfficeServerManagerVelocityBridge(oomanager, docBridge));
> > + }
> > +}
> >
>
Fixed.
> Why not override toString()?
>
> > + public String getDescription()
> > + {
> > + return this.stateDescription;
> > + }
> > + }
>
Fixed.
>
> in in
>
> > + * These configuration properties are defined in in XWiki's global
> configuration file using the prefix of
> > + * "officeimporter".
> > + * </p>
> > + *
> > + * @version $Id$
> > + * @since 1.8RC3
> > + */
> > +public interface OpenOfficeServerConfiguration
>
Fixed.
> I'm not sure that checking for specific values is a good decision.
> Perhaps checking for non-null, non-empty and non-negative?
>
> > +
> assertEquals(OfficeUtils.getDefaultOfficeHome().getAbsolutePath(),
> configuration.getHomePath());
> > +
> assertEquals(OfficeUtils.getDefaultProfileDir().getAbsolutePath(),
> configuration.getProfilePath());
> > + assertEquals(50, configuration.getMaxTasksPerProcess());
> > + assertEquals(30000, configuration.getTaskExecutionTimeout());
> > + }
> > +}
>
Fixed.
> Is this attachment used anywhere?
>
> > +<attachment>
> > +<filename>icon.png</filename>
> > +<filesize>8674</filesize>
> > +<author>XWiki.Admin</author>
> > +<date>1236277246000</date>
> > +<version>1.0</version>
> > +<comment></comment>
> > +</attachment>
>
Yes, it's used as the openoffice server administration icon in
XWikiPreferences page. This icon will be replaced by a better one (I have
asked laurent).
> I usually delete the tag objects from the XML file before committing.
>
Fixed (Removed tag objects)
> > +#if($hasAdmin)
> > + #info($msgConfiguration)
> > + #if($request.action && $request.action=="stop")
>
> This is best done like this:
> #if("$!request.action" == 'stop')
>
> > + #if(!$oomanager.stopServer())
> > + #error($oomanager.lastErrorMessage)
> > + #end
> > + #elseif($request.action && $request.action=="start")
> > + #if(!$oomanager.startServer())
> > + #error($oomanager.lastErrorMessage)
> > + #end
> > + #elseif($request.action && $request.action=="restart")
>
> Since you're using it several times, you could store the action in a
> variable at the top:
> #set($currentAction = "$!request.action")
>
Fixed (learnt about $! just now)
> What's with the paragraphs? IMO they aren't needed, just write:
>
> $msgServerPath | $serverPath
>
> > +<p>$msgServerPath</p>|<p>$serverPath</p>
> > +<p>$msgServerProfile</p>|<p>$serverProfile</p>
> > +<p>$msgServerState</p>|<p>$currentState</p>
>
Well, without the paragraphs the table looks bit too tight.
>
> Better:
>
> Setting for the OpenOffice instance controlled by the office importer
> plugin.
>
> > +# OpenOffice
> >
> +#-------------------------------------------------------------------------------------
> > +
>
Fixed.
>
> Since 1.9M1?
>
> > +#-# [Since 1.8RC3]
> > +#-# Path to openoffice installation.
> > +# openoffice.homePath=/opt/openoffice.org3/
> > +
> > +#-# [Since 1.8RC3]
> > +#-# Path to openoffice execution profile.
>
> Is this right? For me it's /home/user/.ooo3/
>
> > +# openoffice.profilePath=/home/user/.openoffice.org/3
>
This is the default path structure returned by jodconverter. I think this is
valid for most Linux systems.
Thanks.
- Asiri
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
