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. 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. * 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 _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
