Sergiu Dumitriu wrote:
Hi devs, Asiri,
I finally found the time to apply Asiri's patch to integrate the WebDAV
module in the default XE, and here are some comments/ideas:
- I like this new feature, combined with KDE. I was able to start
Konqueror, point it to localhost, and start editing wiki files in my
favorite editor, Kate.
- It seems to be much faster than the web view, which suggests that
indeed most of the time is spent on rendering.
- I wasn't able to create new wiki documents. Is there something special
I should do? Is it not supported yet? Is Konqueror alone guilty?
- Also, the error I get doesn't seem right (File already exists), since
the file I want to create definitely doesn't exist.
- Since it uses Basic authentication, we should advertise that
authentication is not secure, thus it should only be used inside trusted
intranets, over VLANs or over SSL.
Code related:
- Looking a bit over the code, I noticed that we're using only a bit
from the jackrabit-jcr-server library, and I was wondering if it would
be possible to refactor all the code that needs it to use something
else, so that we have one dependency in minus.
- Also, I noticed that you're using slf4j as the logging framework,
which doesn't fit with the rest of the XWiki code, since we're using
commons-logging. Still, this is also the framework used by jackrabbit,
so we can't get rid of it completely. Asiri, could you update the code
to use commons-logging?
- Although there are integration tests, some unit tests would be great.
I know it's hard for the moment to do proper unit testing, since most of
the code works closely with the old core, which doesn't contribute to a
good testing scenario.
Congratulations for the good work, Asiri!
And I forgot one more thing:
- The code is currently in com.xpn.xwiki.plugin.webdav, which I don't
quite agree with. The plugin package is for plugins, and it is somewhat
deprecated (since we're planning to switch completely to components and
remove the notion of 'plugins' per se). Thus, I'd like to rename it to
com.xpn.xwiki.webdav for the moment (except the actual webdav plugin,
which is rightly placed in the plugin package).
--
Sergiu Dumitriu
http://purl.org/net/sergiu/