Hi,

...

+ /** + * {@inheritDoc} + */ + public String getSpace() + { + return config.getParameter("space", "Main"); + } + + /** + * {@inheritDoc} + */ + public String getWiki() + { + return config.getParameter("wiki", "xwiki"); + }

Actually I don't see a good reason for which I made these functions public. We should make them protected.

+1, I'll do it.

...

+ + /** + * {@inheritDoc} + */ + protected String getFileHelpLabel() + { + return Strings.INSTANCE.importOfficeFileHelpLabel(); + } + + /** + * {@inheritDoc} + */ + protected void onAttachmentUploaded(Attachment attach, final AsyncCallback<Boolean> async) + { + + String fullPageName = getSpace() + "." + getPage();

you should use the ResourceName class to build wiki names, since it's the client wiki reference serializer/deserializer.

...

+ WysiwygService.Singleton.getInstance().officeToXHTML(fullPageName, getHTMLCleaningParams(),

Actually this code will go away when I introduce the new officeToXHTML(Attachment....) method.

+ // Display the error and avoid submit operation from continuing.

...

+ displayError(thrown.getMessage()); + async.onSuccess(false);

As a convention, we used the onFailure to signal an error on the server or in the server-client communication (like a GWT failure in the RPC call), while the submit async.onSuccess(false) is an "application error", a way in which a wizard step prevents the submit to be done because, for example, the user didn't insert valid data in the wizard form and another chance is being given to him.

Now, which are the cases in which the importer service fails (throws exception)? is it correct to do this conversion here (from a server error / exception to a "validation" error)? or you should rather call async.onFailure()? this only depends on the logic of the service function failure.

Thanks. I will look into this and fix it.

+ */

...

+ public EnumSet<NavigationDirection> getValidDirections() + { + return EnumSet.of(NavigationDirection.FINISH);

We also return CANCEL here, even if a button for it is not displayed and it's always possible (through the dialog's close button), but in case we will ever want to display a button, or want to prevent the cancel in certain steps.

Ok, I will fix this.

...

+ public void init(Object data, AsyncCallback< ?> cb) + { + textArea.setHTML(""); + textArea.setFocus(true);

I seriously doubt this works, you should a FocusCommand(), look at its comment for reasons.

Ok.

...

+ protected Map<String, String> getHTMLCleaningParams() + { + Map<String, String> params = new HashMap<String, String>(); + params.put("filterStyles", "strict"); + // For Office2007: Office2007 generates an xhtml document (when copied) which has attributes and tags of + // several namespaces. But the document itself doesn't contain the namespace definitions, which causes + // the HTMLCleaner (the DomSerializer) to fail while performing it's operations. As a workaround we + // force HTMLCleaner to avoid parsing of namespace information. + params.put("namespacesAware", Boolean.toString(false)); + return params; + }

Is this the same function as in the other class?

Yes, I am reluctant to introduce a super class or a utility class to wrap up this functionality. Not sure if I should do it or not.

+ return requestedStep;

Maybe we could create a super class for this, it's the same everywhere.

+1.

Also,

svn propset svn:keywords "Author Id Revision HeadURL" <new-file-path>

for all new added files.

check

http://dev.xwiki.org/xwiki/bin/view/Community/DevelopmentPractices#HSubversi... for how to configure this for your svn client. Note that, even if correctly configured, the settings will not be considered by the eclipse plugin (I didn't manage to make them work), you'll have to do a command-line svn add for that to work right.

Ok, thanks for the tip. - Asiri