Re: [xwiki-devs] [Issue] Commented: (XAOFFICE-1) Develop the initial feature set for office-importer plugin.
2 Jan
2009
2 Jan
'09
6:28 p.m.
Hi vincent,
Vincent Massol commented on XAOFFICE-1:
---------------------------------------
31-12-2008 Review:
* Invalid dep: Log4j
Fixed.
* components must be in an internal package, see
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
2 Jan
2 Jan
8:01 p.m.
New subject: [xwiki-devs] [Issue] Commented: (XAOFFICE-1) Develop the initial feature set for office-importer plugin.
On Jan 2, 2009, at 6:28 PM, Asiri Rathnayake wrote:
Hi vincent,
Vincent Massol commented on XAOFFICE-1:
My comment was about the mixing, not about the need for the Document
Access bridge.
---------------------------------------
31-12-2008 Review:
* Invalid dep: Log4j
Fixed.
* components must be in an internal package, see
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.
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.
Thanks
-Vincent
http://xwiki.com
http://massol.net
http://xwiki.org
* there are test resources but they're not
used!
Fixed (Removed)
Thanks.
- Asiri
3 Jan
3 Jan
8:09 p.m.
New subject: [xwiki-devs] [Issue] Commented: (XAOFFICE-1) Develop the initial feature set for office-importer plugin.
Hi Vincent,
Ok.
Not only the bridge but also the document name (against which the cleaning
is done) will need to be provided for this filter. But since the signature
of a filter is clean(Document dom), there is no other way to pass additional
information (like the document name) that is required for determining the
attachment URL. I have another way to get around this problem with
cleaningParameters we introduced to the HTMLCleaner (we can pass the
document against which the cleaning is done). Will work on it.
BTW not sure why you removed the Transformer components.
Transformers are specific to office importer. They won't be used by any
other component outside office importer. So I thought there is no point in
keeping them as components.
Thanks.
- Asiri
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.
8:13 p.m.
New subject: [xwiki-devs] [Issue] Commented: (XAOFFICE-1) Develop the initial feature set for office-importer plugin.
On Jan 3, 2009, at 8:09 PM, Asiri Rathnayake wrote:
Hi Vincent,
Ok.
Not only the bridge but also the document name (against which the
cleaning
is done) will need to be provided for this filter.
Just to understand can you explain why you need the document name in
some filters?
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.
But since the signature
of a filter is clean(Document dom), there is no other way to pass
additional
information (like the document name) that is required for
determining the
attachment URL. I have another way to get around this problem with
cleaningParameters we introduced to the HTMLCleaner (we can pass the
document against which the cleaning is done). Will work on it.
BTW not sure why you removed the Transformer components.
Transformers are specific to office importer. They won't be used by
any
other component outside office importer. So I thought there is no
point in
keeping them as components.
Thanks.
- Asiri
_______________________________________________
devs mailing list
devs(a)xwiki.org
http://lists.xwiki.org/mailman/listinfo/devs
Thanks
-Vincent
http://xwiki.com
http://massol.net
http://xwiki.org
8:22 p.m.
New subject: [xwiki-devs] [Issue] Commented: (XAOFFICE-1) Develop the initial feature set for office-importer plugin.
Hi Vincent,
The XHTML->XWiki 2.0 parser expects image links of following format:
<!--startimage:my.png--><img
src="/xwiki/bin/download/currentdoc/my.png"
param1="value1" param2="value2"
alt="my.png"/><!--stopimage-->
But the html documents generated by open office sever contain image links
like:
<img src="my.png" param1="value1" param2="value2"
alt="my.png"/>
So, the filtering done by ImageFilter is to convert the second one to the
first version. To obtain the attachment url for "my.png", we need to know
against which document the cleaning is done (so that
DocumentAccessBridge#getAttachmentURL()) can be used.
Note: ONLY ImageFilter thus far has this requirement.
Thanks.
- Asiri
Not only the bridge but also the document name
(against which the
cleaning
is done) will need to be provided for this filter.
Just to understand can you explain why you need the document name in
some filters?
5788
days inactive
5789
days old
4 comments
2 participants
participants (2)
-
Asiri Rathnayake -
Vincent Massol