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.
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.
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.
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? 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.
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.
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?
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.
http://gwt-code-reviews.appspot.com/1355802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
