Re: [xwiki-devs] [xwiki-notifications] r17373 - in platform: core/trunk/xwiki-core/src/main/resources core/trunk/xwiki-officeimporter core/trunk/xwiki-officeimporter/src/main/java/org/xwiki/officeimporter core/trunk/xwiki-officeimporter/src/main/java - xwiki-devs@xwiki.org

9 Mar 2009

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 &amp;&amp; $request.action=="stop") 
 This is best done like this:
 #if("$!request.action" == 'stop')

  +    #if(!$oomanager.stopServer())
 +      #error($oomanager.lastErrorMessage)
 +    #end
 +  #elseif($request.action &amp;&amp; $request.action=="start")
 +    #if(!$oomanager.startServer())
 +      #error($oomanager.lastErrorMessage)
 +    #end
 +  #elseif($request.action &amp;&amp; $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

+&lt;p&gt;$msgServerPath&lt;/p&gt;|&lt;p&gt;$serverPath&lt;/p&gt;

+&lt;p&gt;$msgServerProfile&lt;/p&gt;|&lt;p&gt;$serverProfile&lt;/p&gt;

+&lt;p&gt;$msgServerState&lt;/p&gt;|&lt;p&gt;$currentState&lt;/p&gt;

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