Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.4
    • Fix Version/s: 1.6
    • Component/s: Engine
    • Labels:
      None

      Description

      Currently, all reference objects in a Velocity template file are put out using the objects toString method, with no means to control this mechanism. I propose to change this by having interceptors that can pick up variables before they are run .toString on, and write them using their own means instead.

      If I figure out how to attach files to this after posting, I will add a patch with all the code necessary to implement this.

      1. valuewriter.patch
        48 kB
        Ståle Undheim

        Activity

        Hide
        Ståle Undheim added a comment -

        This patch when applied to Velocity 1.4 release will add support for a ValueWriter.

        Changes made:
        RuntimeConstants.java:
        New property added: "valuewriter.classes"

        RuntimeServices.java:
        New method: getValueWriters();

        RuntimeInstances.java:
        New instance variable: valueWriters
        New method: initializeValueWritters

        • This method is run with the other intializers and will load the ValueWriters from the "valeuewriter.classes" configuration parameter. It creates instances, checks that they implement ValueWriter, run initialize on them and add them to the valueWriters list.

        ASTReference:
        I couldn't figure out where I could change this in the jjt file, so I changed ASTReference directly instead. The render method was changed so that before it checked for null, it ran through all ValueWriters, and checked if any of them could write the object. If a ValueWriter could write the object, escPrefixes will be added to the stream as normal, and the ValueWriter will have an opportunity to write the object out to the writer.

        New interface: ValueWriter
        This is the interface you implement if you want to handle certain objects in a specific way rather than just have toString run on them.

        Show
        Ståle Undheim added a comment - This patch when applied to Velocity 1.4 release will add support for a ValueWriter. Changes made: RuntimeConstants.java: New property added: "valuewriter.classes" RuntimeServices.java: New method: getValueWriters(); RuntimeInstances.java: New instance variable: valueWriters New method: initializeValueWritters This method is run with the other intializers and will load the ValueWriters from the "valeuewriter.classes" configuration parameter. It creates instances, checks that they implement ValueWriter, run initialize on them and add them to the valueWriters list. ASTReference: I couldn't figure out where I could change this in the jjt file, so I changed ASTReference directly instead. The render method was changed so that before it checked for null, it ran through all ValueWriters, and checked if any of them could write the object. If a ValueWriter could write the object, escPrefixes will be added to the stream as normal, and the ValueWriter will have an opportunity to write the object out to the writer. New interface: ValueWriter This is the interface you implement if you want to handle certain objects in a specific way rather than just have toString run on them.
        Hide
        Will Glass-Husain added a comment -

        Kind of a neat idea. Modifying ASTReference directly is the way to go – we ignore all Node files generated by jjtree.

        Can you give a use case or two explaining why this feature is useful? Are you using this in your own apps? How?

        Show
        Will Glass-Husain added a comment - Kind of a neat idea. Modifying ASTReference directly is the way to go – we ignore all Node files generated by jjtree. Can you give a use case or two explaining why this feature is useful? Are you using this in your own apps? How?
        Hide
        Nathan Bubna added a comment -

        I love the idea. In fact, i've thought of this "reference rendering interceptor" type thing before and have occasionally toyed around with ideas for implementing it. If we're anywhere near being able to release 1.5 (i haven't checked the roadmap lately), then i think this should wait. But if we've got a few months or something, then i'm definitely interested in working with you on this. Here's a couple nitpicky comments to start, followed by a few more practical ones:

        • your patch is messy, with lots of unneeded formatting changes. makes it hard to read.
        • i don't like the ValueWriter.writeObject() naming. i'd prefer either something more explanatory like ReferenceInterceptor with methods initialize(RuntimeServices), isSupported(Object), and render(Object, Writer, Context).
        • i don't want to hurt performance. with a high number of reference interceptors in use, the rendering of each and every reference (whether it is being intercepted or not) will be slowed by the search for an appropriate interceptor. so...
        • if possible, we should try to cache the last interceptor used for each reference. when merging with a new context, we just double check to make sure the Class of the reference hasn't changed. usually they don't, but they are allowed to do so.
        • it would probably be quicker to keep a map of reference classes to their supporting interceptor. if we don't find a match for the class in the map, then we iterate through all the interceptors until we find one and then store that in the map.
        • actually, it might even be better to do the above mapping during RuntimeInstance initialization. we might just need to change the "boolean isSupported(Object)" method to a "List<Class> supports()" method. Then we can directly ask each ReferenceInterceptor what it supports.

        anyway, i think this is a great idea that can make very common tasks like standardizing date and number formatting across all your templates very easy. (there's two quick examples, Will . i'm glad to see someone taking the time to work up some code for it!

        hmm. what about something shorter and more symbolic like "ReferenceLens" instead of "ReferenceInterceptor"?

        Show
        Nathan Bubna added a comment - I love the idea. In fact, i've thought of this "reference rendering interceptor" type thing before and have occasionally toyed around with ideas for implementing it. If we're anywhere near being able to release 1.5 (i haven't checked the roadmap lately), then i think this should wait. But if we've got a few months or something, then i'm definitely interested in working with you on this. Here's a couple nitpicky comments to start, followed by a few more practical ones: your patch is messy, with lots of unneeded formatting changes. makes it hard to read. i don't like the ValueWriter.writeObject() naming. i'd prefer either something more explanatory like ReferenceInterceptor with methods initialize(RuntimeServices), isSupported(Object), and render(Object, Writer, Context). i don't want to hurt performance. with a high number of reference interceptors in use, the rendering of each and every reference (whether it is being intercepted or not) will be slowed by the search for an appropriate interceptor. so... if possible, we should try to cache the last interceptor used for each reference. when merging with a new context, we just double check to make sure the Class of the reference hasn't changed. usually they don't, but they are allowed to do so. it would probably be quicker to keep a map of reference classes to their supporting interceptor. if we don't find a match for the class in the map, then we iterate through all the interceptors until we find one and then store that in the map. actually, it might even be better to do the above mapping during RuntimeInstance initialization. we might just need to change the "boolean isSupported(Object)" method to a "List<Class> supports()" method. Then we can directly ask each ReferenceInterceptor what it supports. anyway, i think this is a great idea that can make very common tasks like standardizing date and number formatting across all your templates very easy. (there's two quick examples, Will . i'm glad to see someone taking the time to work up some code for it! hmm. what about something shorter and more symbolic like "ReferenceLens" instead of "ReferenceInterceptor"?
        Hide
        Will Glass-Husain added a comment -

        Ok, targeting this for v1.6. Nathan's suggestions are good ones.

        Show
        Will Glass-Husain added a comment - Ok, targeting this for v1.6. Nathan's suggestions are good ones.
        Hide
        Ståle Undheim added a comment -

        The naming bit:

        Nothing final about it. But I feel its better to have actual code to discuss rather than just theory. And sorry about the messy patch, I just did a diff and output it to a file without actually reading it. And its the first time I have made a patch.

        I am also not certain about the output of escape and more strings that happens in render, so someone a bit more knowledgable needs to look into that. I also could not figure out the jjt code for where to alter to affext the ASTReference, but I am sure others will be able to.

        For usage, we intended to use it in a project where we have a tree of objects, each object has its own associated template. Currently we will have to use toString on every level, and no context information can be inherited. With this we could better control how objects where written to the Writer, reuse the same writer instead of creating alot of strings, and inherit the Context. And another argument is that is always nice to be able to plug in to core mechanics of a framework to make it more flexible in use, as long as this is not hackish.

        Some other suggestions some work colleagues suggested was to have some core-interceptors for for example String and NULL, so this system is used for everything (and it is now possible to override null and String).

        Another core feature would be to add a Renderable interface with just the method render(Writer, Context), and an interceptor that just called the render method. This way you could use this without having to alter the default configuration.

        I am not sure if I like the List<Class> supports() though, as it won't be possible to pick up on interfaces and subclasses correctly then. It could be changed to isClassSupported(Class) rather than isObjectSupported(Object), so you would dissalow reading of instance properties to determine if an Object can be rendered.

        Show
        Ståle Undheim added a comment - The naming bit: Nothing final about it. But I feel its better to have actual code to discuss rather than just theory. And sorry about the messy patch, I just did a diff and output it to a file without actually reading it. And its the first time I have made a patch. I am also not certain about the output of escape and more strings that happens in render, so someone a bit more knowledgable needs to look into that. I also could not figure out the jjt code for where to alter to affext the ASTReference, but I am sure others will be able to. For usage, we intended to use it in a project where we have a tree of objects, each object has its own associated template. Currently we will have to use toString on every level, and no context information can be inherited. With this we could better control how objects where written to the Writer, reuse the same writer instead of creating alot of strings, and inherit the Context. And another argument is that is always nice to be able to plug in to core mechanics of a framework to make it more flexible in use, as long as this is not hackish. Some other suggestions some work colleagues suggested was to have some core-interceptors for for example String and NULL, so this system is used for everything (and it is now possible to override null and String). Another core feature would be to add a Renderable interface with just the method render(Writer, Context), and an interceptor that just called the render method. This way you could use this without having to alter the default configuration. I am not sure if I like the List<Class> supports() though, as it won't be possible to pick up on interfaces and subclasses correctly then. It could be changed to isClassSupported(Class) rather than isObjectSupported(Object), so you would dissalow reading of instance properties to determine if an Object can be rendered.
        Hide
        Ståle Undheim added a comment -

        > Modifying ASTReference directly is the way to go – we ignore all Node files generated by jjtree.

        I thought you where beeing sarcastic at first. So why does the comments say you should modify the jjt files and not the class files themselves?

        Show
        Ståle Undheim added a comment - > Modifying ASTReference directly is the way to go – we ignore all Node files generated by jjtree. I thought you where beeing sarcastic at first. So why does the comments say you should modify the jjt files and not the class files themselves?
        Hide
        Will Glass-Husain added a comment -

        No - not intentionally sarcastic at all! Where are the confusing comments? We should probably fix them.

        The original Node files were generated by JJTRee, but then they've been customized and moved into a different package. JJTree continues to create these Nodes but the newly created ones are redundant and need to be deleted.

        Did you read:
        src\java\org\apache\velocity\runtime\parser\BUILD_README.txt

        There's also an ant task that handles this automatically. (ant parser).

        WILL

        Show
        Will Glass-Husain added a comment - No - not intentionally sarcastic at all! Where are the confusing comments? We should probably fix them. The original Node files were generated by JJTRee, but then they've been customized and moved into a different package. JJTree continues to create these Nodes but the newly created ones are redundant and need to be deleted. Did you read: src\java\org\apache\velocity\runtime\parser\BUILD_README.txt There's also an ant task that handles this automatically. (ant parser). WILL
        Hide
        Nathan Bubna added a comment -

        Yeah, it definitely helped to have code to see how you were doing things, whether the patch is ugly or not.

        I too agree it would be cool to have some "core interceptors" for things like NULL and String. I think there will be many more uses for this that others will think of.

        The Renderable interface idea is a good one too. It would be lower priority for me, but it sounds like it might be useful for your particular use case. More functionality with less configuration is good.

        Regarding the List<Class> supports() method. Yeah, i think you're right; we would still need an isSupported() method to pick up on subclasses at runtime. I didn't fully think that through. We should still save those in a Map as we match them in order to speed interceptor lookup for later references.

        I instinctually like the idea of limiting matching to isSupported(Class) rather than the particular instance, but i can't articulate why that seems better or why allowing isSupported(Object) would be bad. we'll have to think about that.

        Show
        Nathan Bubna added a comment - Yeah, it definitely helped to have code to see how you were doing things, whether the patch is ugly or not. I too agree it would be cool to have some "core interceptors" for things like NULL and String. I think there will be many more uses for this that others will think of. The Renderable interface idea is a good one too. It would be lower priority for me, but it sounds like it might be useful for your particular use case. More functionality with less configuration is good. Regarding the List<Class> supports() method. Yeah, i think you're right; we would still need an isSupported() method to pick up on subclasses at runtime. I didn't fully think that through. We should still save those in a Map as we match them in order to speed interceptor lookup for later references. I instinctually like the idea of limiting matching to isSupported(Class) rather than the particular instance, but i can't articulate why that seems better or why allowing isSupported(Object) would be bad. we'll have to think about that.
        Hide
        Nathan Bubna added a comment -

        descheduling this unless/until it gets more momentum (and justification, since this largely overlaps the event handler stuff for reference insertion.

        Show
        Nathan Bubna added a comment - descheduling this unless/until it gets more momentum (and justification, since this largely overlaps the event handler stuff for reference insertion.
        Hide
        Claude Brisson added a comment -

        You at least have the Renderable interface, now.
        I needed it to allow #defined references to still use a writer when rendered (while also having a normal toString() method).

        The funny thing is I re-read all this thread afterwards

        Show
        Claude Brisson added a comment - You at least have the Renderable interface, now. I needed it to allow #defined references to still use a writer when rendered (while also having a normal toString() method). The funny thing is I re-read all this thread afterwards
        Hide
        Claude Brisson added a comment -

        By the way, isn't this interface everything you need to do what you want, that is have templates associated with client objects, and be able to re-use the main writer?
        I'm not sure you'd need some other interception mechanism. The ValueWriter stuff looks really too heavy to me.
        So I'm resolving this issue - but feel free to reopen it afterwards otherwise.

        Show
        Claude Brisson added a comment - By the way, isn't this interface everything you need to do what you want, that is have templates associated with client objects, and be able to re-use the main writer? I'm not sure you'd need some other interception mechanism. The ValueWriter stuff looks really too heavy to me. So I'm resolving this issue - but feel free to reopen it afterwards otherwise.

          People

          • Assignee:
            Unassigned
            Reporter:
            Ståle Undheim
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development