J. Wolfgang Kaltz wrote:
Andreas Hartmann schrieb:
Hi Lenya devs,
some comments on the new DocumentManager.add() method:
General comment on the changes: the goals were to
- ensure Lenya meta-data creation in all use cases
- remove usage of java.io.File
- document the process & make simplifications if possible
Frankly, I still haven't understood all of the existing code involved,
so much of the functionality is exactly as it was before (just
refactored). As admitted in my previous mail, I definitely agree there
is room for improvement / clarification - for the time being I was happy
to get the above goals working (I found it quite challenging to get
something working at the same time in the default publication and in the
blog publication)
Thank you for these improvements, they're really appreciated.
I hope you don't see my comments as criticism, actually I'm very
happy that you're that active in developing 1.4.
> @param parentDocument The parent document.
This must be optional to allow creation of top-level documents.
Should we allow to pass null objects? Then it should be noted in
the javadoc comment.
We currently need the parent
- to construct the new URI for a default publication entry. So I guess
for this the parent could be null, in which case we would simply use "/"
as the base for the new entry (just committed that)
But isn't this clearly definded by the document ID of the newly
created document, or am I missing something?
- to access the DocumentIdentityMap. Like previously said, I don't
understand why the DocumentIdentityMap is accessed via an existing
Document instance. If it were accessed via the Publication, we could
pass that instead.
Currently, the publication object is not designed to care about
transactions. IMO it shouldn't know anything about identity maps etc.
> @param newDocumentNodeName the name of the node representing the new
> document
> @param newDocumentId the id of the new document
Why do we need both nodeName and documentId?
Actually the document ID should be sufficient, that would even
mean that the parent is unambigously defined.
The nodeName is used to build the new URI, whereas the newDocumentId is
used to look up a new instance via the IdentityMap. I suppose these
could be merged, but was looking understanding of the IdentityMap to
risk it.
OK, I'll take a deeper look at the code.
[...]
> @param parameters any parameters the caller needs to pass to the
creator
Can't we get rid of that? IMO it's a bad practise to allow polymorphism
on the basis of arbitrary parameter sets.
It is in the creator interface, and used by the blog entry creator in
the transformXML() method. I am not sure how the parameters would be
passed otherwise ?
I'll take a look at this, maybe a pattern can be applied to the
situation.
> @param useSiteManager set to true if the site manager is used in the
> publication; if set the site manager will be notified about
the new
> document
IMO the site manager must be mandatory.
The problem is that publication.getSiteManagerHint() returns null when
in the blog publication. I agree we should remove that parameter once
the blog pub has a site manager.
That should work now.
Thx for your feedback & please make any improvements you feel are
appropriate !
I lack understanding of the IdentityMap, site manager & blog pub
internals to do them at this time.
I'll try to add more docs about the internals when I find the time.
To me the important things for now
are that Document.getResourceType() is reliable in all situations (so we
can remove the DocumentTypeResolver)
IMO it would be valid to throw an exception in getResourceType() if
the document does not exist.
and that creation goes through
lenya:// instead of File.
+1
-- Andreas
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
