Re: [Jsynthlib-devel] XML patch saving....

[Rib R. <ri...@gm...>]

On 3/9/06, Joe Emenaker <jo...@em...> wrote:
> Rib Rdb wrote:
> > On 3/8/06, Joe Emenaker <jo...@em...> wrote:
> >
> >> Rib Rdb wrote:
> >>
> >>> I still think this has problems.  How do you handle transient data or
> >>> data which has multiple getters and setters?
> >>>
> >> Well, I think I hinted, at one point, at the idea of, instead of tryin=
g
> >> to autodetect the public getters and setters, maintaining an array or
> >> vector of the property names to save. For example, if the vector
> >> contained "Name", "Author", and "Date", then the XML load and save cod=
e
> >> would only try to call getName, setName, getAuthor, setAuthor, etc. Bu=
t,
> >> if my suggestions thus far have made you uncomfortable, then *this* id=
ea
> >> has probably got you squirming. :)
> >>
> >
> > Hmmm.  The more I think about it the more I like this idea.  It's more
> > uniform than having setters used for loading, but one method used for
> > writing.  We could add something like "List getXmlPropertyNames()" to
> > IPatch.  But then we still have the super problem.
> Well, yes and no. If the property names are kept in a vector or
> something similar, then the vector would be created in the base class
> and populated with some values via some add() method, like:
>    public Patch() {
>      addPropertyName("Author");
>      addPropertyName("Comment");
>      etc.
>    }

That would work, but that's probably something that should go in an
implementation not the actual interface.

>
> The default constructor is supposed to be called for all subclasses that
> don't explicitly specify a super constructor in their own constructor.
> So, if I subclass Patch in a class called SpecialPatch and write:
>    public SpecialPatch() {
>      addPropertyName("SomeSpecialData");
>    }
>
> then the Author and Comment ones should still get put into the vector.

Do you really think anyone is ever going to subclass Patch.  Have you
seen how much of it is final?  The main reason IPatch was created was
so people don't have to subclass Patch.  I think the main thing we
need to worry about is new implementations of IPatch (which are still
going to be rare and only done by people with an advanced knowledge of
JSynthLib), not subclasses of the existing two implementations.

> But now it's getting fairly cumbersome. We've got to have matching
> getters and setters, they have to be public, and their name has to match
> a String value in the PropertyName vector. So, at this point, I think it
> would then be easier to just use a special data structure for all data
> that we want to save (except the sysex. That's always special).
>
> How about this. Patch would have some object of Name/Value pairs which
> would contain all of the data that should be saved. This gets around all
> of the getters/setters. In fact, a class wouldn't have to change their
> code much at all. Things like:
>      patchName =3D getPatchNameFromSysexData();
> would become:
>     setValue("PatchName",getPatchNameFromSysexData());
>
> In support of this kind of method, I point out that the JAXP DOM stuff
> already has something like this in NamedNodeMap.
>
> The three problems I see with this are:
> 1 - The simple implementation I've proposed here only deals with
> strings. But there's nothing to keep us from making setIntValue(String
> name, int val), and setBoolVal(..) etc.
> 2 - There's nothing to purge old deprecated data. If Patch has a field
> called "Author" and then we decide, later, that we don't want an
> "Author" field anymore and strip any mention of it out of Patch, the
> Author data items already existing in XML files will continue to get
> loaded and saved.... even though they're never used by JSL. It's a weird
> version of a memory leak, if you will.
> 3 - Name collision. There's nothing to keep subclasses from stomping on
> values in the superclass. If you subclassed Patch, you'd need to know
> that Patch already uses "Author" so that you didn't try to use it
> yourself for a different purpose. Even worse, if we went in and added a
> property name to the Patch superclass, it could collide with a property
> name in one of Patch's subclasses.

I really don't like this idea.  And how do you enforce it with an
interface?  You would have to add getter and setter methods for this
data structure.  Read and write methods seems a lot simpler.

> > But as I said
> > before, I really don't think new Patch classes are going to be made
> > often, so we should just pick one.  And calling super.write seems more
> > like the java way to do it.  Java doesn't have much magic.
> >
> Well, if you don't feel like we're working our way any closer to a
> better solution, then we can just go with super.write().

Actually I meant super.getXmlPropertyNames.

> The one thing I'd want to change, however, is that I think we should have=
 a read()
> method as well. Here's why...
>
> The current "super.write()" proposal, as I understand it, is to have
> write() do things like:
>
>   xml.writeProperty("author", self.author);
>
> and then we need a setAuthor(String) method. I'm still wincing over the r=
equirement that "author" in the call to writeProperty match "setAuthor". Ho=
wever, if we had a read() method that did things like:
>
>   self.author =3D xml.readProperty("author");
>
> *then* we can put "author" in a constant, so we'd end up with code like t=
his:
>
>   private static final String AuthorXMLName =3D "author";
>
>   xml.writeProperty(AuthorXMLName, self.author);
>
>   xml.readProperty(AuthorXMLName);
>
> which is what you can see I did in my XMLFileUtils class. It makes it *mu=
ch* more likely that your IDE will catch any typos. Also, reading is done j=
ust like writing.

I certainly like the idea of having a read method. But the parser I
wrote is event based (SAX), so a read method doesn't easily translate.
 But I suppose we could make it work.

> Lastly, the readProperty() method(s) should be really easy to write, sinc=
e we can have the "xml." part of "xml.readProperty" holding the DOM Node fo=
r just that patch... and readProperty could just turn right around return t=
he value returned by "getAttribute()" for that node.

Do you mean to store all the data as attributes of an element instead
of children?  That sounds like a bad idea, and my parser certainly
won't be able to handle that.