On Jan 2, 2009, at 6:28 PM, Asiri Rathnayake wrote: > Hi vincent, > > Vincent Massol commented on XAOFFICE-1: >> --------------------------------------- >> >> 31-12-2008 Review: >> * Invalid dep: Log4j > > > Fixed. > > * components must be in an internal package, see >> http://dev.xwiki.org/xwiki/bin/view/Community/DevelopmentPractices#HXWikispecifics > > > Fixed. > > >> * Abstract classes must have Abstract in their name as a prefix. >> OfficeImporterHTMLCleaner is not following this. See >> http://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle > > > Fixed > > * OfficeImporterHTMLCleaner should be named AbstractOfficeHTMLCleaner > > > Fixed. > > * I don't think that the default/strict/moderate thing should be > implemented >> as components. IMO there should be a singme OpenOfficeHTMLCleaner >> and a >> single MicrosoftOfficeHTMLCleaner and the interface signature >> should be >> something like: clean(Reader, CleaningStrategy) where >> CleaningStrategy is an >> enum of STRICT, MODERATE or LENIENT. The reason is that it's a >> property of >> the cleaning IMO and not a different type of cleaner. > > > Fixed. > > * The org.xwiki.officeimporter.html.cleaner package should be instead >> org.xwiki.officeimporter.cleaner. No need for "html" since we don't >> have >> any other cleaner. >> * org.xwiki.officeimporter.html.filter should renamed to >> org.xwiki.officeimporter.cleaner.filter > > > Fixed. > > * Don't use public static final String in an interface since String > are >> always public static final in interfaces! > > > Fixed. > > * Fix javadoc errors > > > Partially Fixed (working on it) > > * I have no idea why we need a special cleaner > WysiwygDefaultHTMLCleaner and >> the javadoc doesn't help to understand the reason. > > > Need to improve the javadoc comments. > > * OfficeImporter.import takes a byte[] array. I don't think this is > a good >> idea since you'd also need to specify the encoding. Better use a >> Reader. > > > As I explained, office documents are binary files. So this has to be a > byte[]. > > * OfficeImporterVelocityContextInitializer should be in the internal > package > > > Fixed. > > * "private static final String[] presentationFormatExtensions" private >> static final strings should be in uppercase. Also this would be >> better as an >> enum. > > > Fixed. > > * I don't understand the notion of Transformer. What's the difference >> between Transformer and Importer? > > > Transformers are now inside internal package and they are not > components > anymore. Importer is composed of DocumentTransformers. > > * components that wants logging must extend AbstractLogEnabled > > > Fixed. > > * filters need to be reorganized. There are too many notions mixed > together: >> strict/lenient/moderate vs ImageFilter/Etc > > > I'm not sure how to do this.
My comment was about the mixing, not about the need for the Document Access bridge. > Unlike other filters, ImageFilter needs to have > access to the original XWikiDocument (via DocumentAccessBridge) > other than > the w3c DOM. This is because when cleaning image links, the src > attribute > need to be replaced by the corresponding attachment url. This is why I > introduced XWikiHTMLCleaner which takes in the OfficeImporterContext > in > addition to the w3c dom. Make them components and have the bridge injected. BTW not sure why you removed the Transformer components. > * there shouldn't be a utils package > > > Fixed. > > * role hints should be simpler and lowercase > > > Fixed. > > * there are no tests! > > > Few added. More unit/integration tests will be added. > > >> * there are test resources but they're not used! >> > > Fixed (Removed) > > Thanks. > > - Asiri Thanks -Vincent http://xwiki.com http://massol.net http://xwiki.org _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
