On Thu, Mar 3, 2011 at 8:12 AM, <j...@google.com> wrote: > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/client/impl/plurals/DefaultRule.java > File user/src/com/google/gwt/i18n/client/impl/plurals/DefaultRule.java > (right): > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/client/impl/plurals/DefaultRule.java#newcode1 > user/src/com/google/gwt/i18n/client/impl/plurals/DefaultRule.java:1: /* > I'll revert. > > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/AbstractResource.java > File user/src/com/google/gwt/i18n/rebind/AbstractResource.java (right): > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/AbstractResource.java#newcode1 > user/src/com/google/gwt/i18n/rebind/AbstractResource.java:1: /* > On 2011/03/03 01:07:39, rjrjr wrote: > >> no real change >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java > File > user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java > (right): > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java#newcode44 > user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java:44: > interface MessageCatalogContext { > On 2011/03/03 01:07:39, rjrjr wrote: > >> Since this is nested, how about just calling it Context? >> > > I think the normal case for someone using it this is just to use > auto-import in their IDE, so the source is likely to jus t have Context, > which seems likely to conflict with other uses and less understandable. > > If you still would prefer shortening the name, I am happy to do it.
If you don't mind. > > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/format/PropertyCatalogFactory.java > File > user/src/com/google/gwt/i18n/rebind/format/PropertyCatalogFactory.java > (right): > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/format/PropertyCatalogFactory.java#newcode219 > user/src/com/google/gwt/i18n/rebind/format/PropertyCatalogFactory.java:219: > String fileName) throws MessageProcessingException { > On 2011/03/03 01:07:39, rjrjr wrote: > >> not thrown >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/server/MessageFormVisitor.java > File user/src/com/google/gwt/i18n/server/MessageFormVisitor.java > (right): > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/server/MessageFormVisitor.java#newcode54 > user/src/com/google/gwt/i18n/server/MessageFormVisitor.java:54: * </ul> > Yes, just failed to delete it when I added it there. > > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/server/MessageFormVisitor.java#newcode63 > user/src/com/google/gwt/i18n/server/MessageFormVisitor.java:63: * {@link > #processDefaultMessage(MessageStyle, String)}. > On 2011/03/03 01:07:39, rjrjr wrote: > >> You're talking about the default message, but it's not used in this >> > interface at > >> all. >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java > File user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java > (right): > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java#newcode27 > user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java:27: * > {@code mv = miv.visitMessage(msg, msgTrans);} > On 2011/03/03 01:07:39, rjrjr wrote: > >> {@code} is redundant with <pre> >> > > If you show the types of these visitors (declare them), it would be >> > easier to > >> see what the types are. e.g. >> > > {@link MessageVisitor} mv = miv.visitMessage(msg, msgTrans); >> > > Done. > > > > http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java#newcode62 > user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java:62: * > {@code miv.endClass(msgIntf);} > On 2011/03/03 01:07:39, rjrjr wrote: > >> Don't forget to close that <pre> section >> > > Done. > > > I still think the MessageInterfaceVisitor adds noise, not value. >> > > Look at this from >> > AbstractLocalizableImplCreator.generateToMsgCatFactory(), > >> which uses MessageCatalogWriter, which has the only implementation of >> > this > >> interface. >> > > catWriter = msgCatFactory.getWriter(ctx, catalogName); >> msgIntf.accept(catWriter.visitClass()); >> > > As a client of the writer I have to know about visitors and the accept >> > method, > >> where those could easily be implementation details. It's not as easy >> > to > >> understand as: >> > > catWriter = msgCatFactory.getWriter(ctx, catalogName); >> catWriter.write(msgIntf); >> > > That removes the ability to visit multiple classes with the same visitor > (admittedly not used at present). > > How about retaining the existing API, but providing this as a > convenience method? > > > Where the write method would be implemented something like: >> > > void write(MessageInterface msgIntf) { >> writer.println("# Messages from " + msgIntf.getQualifiedName()); >> writer.println("# Source locale " + sourceLocale); >> msgIntf.accept(new MyMessagesVisitor(writer)); >> } >> > > Those two println()s are from your only implementation of beginClass. >> > You have > >> no actual implementation of endClass(). >> > > See the internal implementation for an example of non-trivial > begin/endClass. Is that missing from the patch? Eclipse is only showing me TestMessageInterfaceVisitor and a one line method in LocalizableGenerator > > > Other renaming and consolidation suggestions: >> > > getFormVisitor() -> visitForms() >> > > Note that these are not called in sequence -- they are called once for a > message to get the visitors to use for each level. You can think of it > as "control breaks" on when a level of a form changes, since more > complicated formats (and code generation) need to build the nested > structures around the messages themselves. > I think I get that, and I'd still find visitForms clearer. That's a good spot for some detailed javadoc, based on the paragraph above, with an example of a message with various forms with various levels, and what calls to visitForms would be made. Such doc would be more useful here than in the mondo here-is-an-entire-visit-sequence thing above. > > The reason I was doing it this way (along with a number of other things) > is to make the visitor implementations simpler and hide the complexity > in the shared library code where it is implemented once. It would be > possible to simply push this complexity down into the visitor > implementations to make the API simpler -- for example, this could be > eliminated and you just call visitSelector/visitForm on the main > MessageVisitor. However, that would require the code generator visitor > (at least, perhaps others) to implement its own method of redirecting > these calls to the appropriate type of visitor for a given selector > level. > So I guess my question to you is, do you think it is better to have a > simple API and a slightly simpler implementation (in AbstractMessage, > for example), but requiring each visitor that needs that functionality > to implement it themselves? I don't think I'm suggesting that. In fact, I'm trying hard to restrict myself to dinging redundancies and naming confusion. At this point you've got me down to renames and snipping the default message methods. > It seems better to me to do that once, > provide more hook points than any one visitor will need, and then a > particular visitor can make use of the hook points that make sense for > them and rely on DefaultVisitor implementations for the rest. > I think we've converged, but let me hold forth a bit on the philosophical point: providing more than is needed puts a cognitive burden on your client. I have to understand all of my options in order to decide which subset I need, and that's hard, especially for someone who is only, uhm, visiting your api. Think of a geeky app with fifteen tabs of preferences, each a wall of checkboxes. Redundant methods are the api equivalent of that. RISC > CISC > > beginSelector() -> visitSelector() >> beginForm() -> visitForm() >> > > So you think having a sequence like: > > visitSelector(0) > visitForm(0, "one") > visitSelector(1) > visitForm(1, "one") > visitContent("one|one translation") > endForm(1, "one") > visitForm(1, "other") > visitContent("one|other translation") > endForm(1, "other") > endSelector(1) > endForm(0, "one") > ... > endSelector(0) > > is clearer than begin/end? They mostly mark positions in the sequence > of processMessage/visitContent calls, to prevent visitor implementations > that care from having to do their own control-break implementation, so > begin/end seems better to me but if you think this is better I am > willing to change it. > I find it easier, yeah > > processMessage -> visitMessage() >> > > This would seem to cause confusion between viisiting a Message object > (MessageInterface.visitMessage) and visiting the translation. Perhaps > visitContent() would avoid that confusion? Yes, that helps. Or visitTranslation? Anything to avoid overloading your terms, especially message. > > > Re: the processDefaultMessageBefore() and processDefaultMessageAfter() >> > calls, > >> why not add their two arguments to visitMessage() and endMessage(), or >> visitForms() and endForms()? For that matter, if the default message >> > is > >> available as a getter from Message, there is no reason for it to >> > appear in args > >> at all. >> > > Ok. It is slightly awkward that the Before call would be processed in a > different visitor, but I suppose they can always either do it in a > constructor (if they build a new MessageVisitor each time) or make their > own Before call and call it from MessageInterfaceVisitor.visitMessage if > they care. That's my point in a nutshell. Methods like these save almost no work for those who might use them, but put a cognitive processing tax on every user of your api. The cost/benefit ratio is out of whack. > > http://gwt-code-reviews.appspot.com/1355802/ > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
