[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
Committed at 9846 - thanks. http://gwt-code-reviews.appspot.com/1355802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
LGTM, SGTM, 10-4 On Fri, Mar 11, 2011 at 7:36 AM, wrote: > > > http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java > File user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java > (right): > > > http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java#newcode31 > user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java:31: > public class MockMessageCatalogContext implements > On 2011/03/11 14:33:53, rjrjr wrote: > >> If this will be useful to non-gwt teams writing their own tests, it >> > should be in > > user/src/.../i18n/server/testing. Note user/src, not user/test >> > > Done. > > http://gwt-code-reviews.appspot.com/1355802/ > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java File user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java (right): http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java#newcode31 user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java:31: public class MockMessageCatalogContext implements On 2011/03/11 14:33:53, rjrjr wrote: If this will be useful to non-gwt teams writing their own tests, it should be in user/src/.../i18n/server/testing. Note user/src, not user/test Done. http://gwt-code-reviews.appspot.com/1355802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/rebind/AnnotationsResource.java
File user/src/com/google/gwt/i18n/rebind/AnnotationsResource.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/rebind/AnnotationsResource.java#newcode133
user/src/com/google/gwt/i18n/rebind/AnnotationsResource.java:133:
private static class KeyGenMessage implements Message {
On 2011/03/11 14:33:53, rjrjr wrote:
A 300 line nested private static class belongs in its own file.
Done.
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/AbstractMessage.java
File user/src/com/google/gwt/i18n/server/AbstractMessage.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/AbstractMessage.java#newcode60
user/src/com/google/gwt/i18n/server/AbstractMessage.java:60: */
On 2011/03/11 14:33:53, rjrjr wrote:
So pass it in, and put this in its own file. That's why God gave us
constructor
arguments.
Ok, though not having to pass "outerThis" in everywhere is why God also
gave us inner classes :).
Does this need to be public? Are subclasses made outside of this
package?
If it is a top-level class, then yes it will have to be public - see
impl/ReflectionMessage, and in the future perhaps an implementation
based on javax.lang.model.
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/AbstractMessage.java#newcode165
user/src/com/google/gwt/i18n/server/AbstractMessage.java:165: private
static class StringMapMessageTranslation
On 2011/03/11 14:33:53, rjrjr wrote:
Please move to a separate file
Done.
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/KeyGenerator.java
File user/src/com/google/gwt/i18n/server/KeyGenerator.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/KeyGenerator.java#newcode34
user/src/com/google/gwt/i18n/server/KeyGenerator.java:34: * {@code
msg.getMethodName()}, {@code msg.getDefaultMessage()},
On 2011/03/11 14:33:53, rjrjr wrote:
These would be more useful as {@link Message#getMethodName}, etc.
Done.
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/impl/ReflectionMessage.java
File user/src/com/google/gwt/i18n/server/impl/ReflectionMessage.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/impl/ReflectionMessage.java#newcode37
user/src/com/google/gwt/i18n/server/impl/ReflectionMessage.java:37: *
Implementation of Message using reflection.
On 2011/03/11 14:33:53, rjrjr wrote:
fyi, you can have a nice @link w/o import thus: {@link
com.google.gwt.i18n.server.Message Message}
Done.
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/impl/ReflectionMessage.java#newcode39
user/src/com/google/gwt/i18n/server/impl/ReflectionMessage.java:39: *
NOTE: THIS CLASS IS CURRENTLY ONLY SUITABLE FOR TESTING
On 2011/03/11 14:33:53, rjrjr wrote:
Unless you plan for this to change, this class belongs in
user/src/.../i18n/server/testing.
Yes, I plan to add source snippet parsing to it so it can be used for
processing i18n messages as a separate build step, or also generating
them on the fly in a server.
I have it working this way in my home app, but it complicates
dependencies and isn't needed for this immediate purpose, so I want to
defer that until later.
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/test/com/google/gwt/i18n/I18NSuite.java
File user/test/com/google/gwt/i18n/I18NSuite.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/test/com/google/gwt/i18n/I18NSuite.java#newcode117
user/test/com/google/gwt/i18n/I18NSuite.java:117: * IDE with
{dev,user}/{src,super} (no test) on the classpath.
On 2011/03/11 14:33:53, rjrjr wrote:
I believe we pay this price w/a number of other tests as well. If this
is the
issue I'm thinking of you can tell GPE to run as GWT Unit Test it will
take care
of the classpath for you, with no performance penalty.
It works fine in GPE, but it doesn't work in ant test because it sees
lots of files from user/test that cause errors. I spent some time
trying to make it work, but failed.
Does the test really not run in the continuous build? If it does,
please
uncomment.
No, it fails with many errors from test code that gets picked up by
TypeOracle.
Perhaps there is a way to use the real TypeOracle to pick up the classes
I want and not everything else, but I haven't found it yet. So, the
approach I know will work is to mock out all the i18n
classes/annotations and all their transitive dependencies, which is a
lot of grunge work that I would like to defer.
http://gwt-code-reviews.appspot.com/1355802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
LGTM with the following addressed
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/rebind/AnnotationsResource.java
File user/src/com/google/gwt/i18n/rebind/AnnotationsResource.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/rebind/AnnotationsResource.java#newcode133
user/src/com/google/gwt/i18n/rebind/AnnotationsResource.java:133:
private static class KeyGenMessage implements Message {
A 300 line nested private static class belongs in its own file.
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/AbstractMessage.java
File user/src/com/google/gwt/i18n/server/AbstractMessage.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/AbstractMessage.java#newcode60
user/src/com/google/gwt/i18n/server/AbstractMessage.java:60: */
So pass it in, and put this in its own file. That's why God gave us
constructor arguments.
Does this need to be public? Are subclasses made outside of this
package?
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/AbstractMessage.java#newcode165
user/src/com/google/gwt/i18n/server/AbstractMessage.java:165: private
static class StringMapMessageTranslation
Please move to a separate file
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/KeyGenerator.java
File user/src/com/google/gwt/i18n/server/KeyGenerator.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/KeyGenerator.java#newcode34
user/src/com/google/gwt/i18n/server/KeyGenerator.java:34: * {@code
msg.getMethodName()}, {@code msg.getDefaultMessage()},
These would be more useful as {@link Message#getMethodName}, etc.
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/impl/ReflectionMessage.java
File user/src/com/google/gwt/i18n/server/impl/ReflectionMessage.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/impl/ReflectionMessage.java#newcode37
user/src/com/google/gwt/i18n/server/impl/ReflectionMessage.java:37: *
Implementation of Message using reflection.
fyi, you can have a nice @link w/o import thus: {@link
com.google.gwt.i18n.server.Message Message}
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/src/com/google/gwt/i18n/server/impl/ReflectionMessage.java#newcode39
user/src/com/google/gwt/i18n/server/impl/ReflectionMessage.java:39: *
NOTE: THIS CLASS IS CURRENTLY ONLY SUITABLE FOR TESTING
Unless you plan for this to change, this class belongs in
user/src/.../i18n/server/testing.
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/test/com/google/gwt/i18n/I18NSuite.java
File user/test/com/google/gwt/i18n/I18NSuite.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/test/com/google/gwt/i18n/I18NSuite.java#newcode117
user/test/com/google/gwt/i18n/I18NSuite.java:117: * IDE with
{dev,user}/{src,super} (no test) on the classpath.
I believe we pay this price w/a number of other tests as well. If this
is the issue I'm thinking of you can tell GPE to run as GWT Unit Test it
will take care of the classpath for you, with no performance penalty.
Does the test really not run in the continuous build? If it does, please
uncomment.
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java
File user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/21001/user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java#newcode31
user/test/com/google/gwt/i18n/server/MockMessageCatalogContext.java:31:
public class MockMessageCatalogContext implements
If this will be useful to non-gwt teams writing their own tests, it
should be in user/src/.../i18n/server/testing. Note user/src, not
user/test
http://gwt-code-reviews.appspot.com/1355802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
I realized the code generator didn't support the new KeyGenerator annotation, and some things were in the rebind package that should have been in server. http://gwt-code-reviews.appspot.com/1355802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
http://gwt-code-reviews.appspot.com/1355802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
LGTM
http://gwt-code-reviews.appspot.com/1355802/diff/15004/user/src/com/google/gwt/i18n/server/AbstractMessage.java
File user/src/com/google/gwt/i18n/server/AbstractMessage.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/15004/user/src/com/google/gwt/i18n/server/AbstractMessage.java#newcode160
user/src/com/google/gwt/i18n/server/AbstractMessage.java:160: public
static class StringMapMessageTranslation
Really needs to be public?
http://gwt-code-reviews.appspot.com/1355802/diff/15004/user/src/com/google/gwt/i18n/server/SelectorTracker.java
File user/src/com/google/gwt/i18n/server/SelectorTracker.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/15004/user/src/com/google/gwt/i18n/server/SelectorTracker.java#newcode26
user/src/com/google/gwt/i18n/server/SelectorTracker.java:26: public
class SelectorTracker {
FormVisitorDriver?
http://gwt-code-reviews.appspot.com/1355802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
http://gwt-code-reviews.appspot.com/1355802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
On 2011/03/04 04:52:16, rjrjr wrote:
http://gwt-code-reviews.appspot.com/1355802/diff/15003/user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java#newcode44
user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java:44: *
mv.visitMessage(["one", "FEMALE"], false, msgStyle, "1");
These are actually visitTranslation, right?
Yes.
http://gwt-code-reviews.appspot.com/1355802/diff/15003/user/src/com/google/gwt/i18n/server/SelectorTracker.java
File user/src/com/google/gwt/i18n/server/SelectorTracker.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/15003/user/src/com/google/gwt/i18n/server/SelectorTracker.java#newcode26
user/src/com/google/gwt/i18n/server/SelectorTracker.java:26: public
class
SelectorTracker {
So I'm responsible for calling methods on this thing from
MIV#visitMessage,
MV#endMessage, and MV#visitTranslation, right? Complicated, although
looking at
LocalizableGeneratorVisitor I can see why.
Do you think it would be possible to build this into a reusable
implementation
of MessageInterfaceVisitor and MessageVisitor? Something that
LocalizableGeneratorVisitor could extend?
abstract class SelectorTrackingVisitor implements
MessageInterfaceVisitor,
MessageVisitor {
/**
* Provides parameter processing via calls to {@link
#processParameter}.
*/
public SelectorTrackingVisitor visitMessage(...) { ... }
/**
* Called from {@link #visitMessage} for each parameter in the
message.
* Returns a FormVisitor for this parameter, or null. FormVisitors
will
* later be run as appropriate from {@link #visitTranslation}.
*/
protected abstract FormVisitor processParameter(Parameter);
/**
* Calls the FormVisitors accumulated from {@link
#processParameter}.
*/
public SelectorTrackingVisitor visitMessage(...) { ... }
}
I'm glossing over the meaning of the int level argument of
SelectorTracker#setFormVisitor(int, MessageFormVisitor), which I don't
understand, hoping it can just be baked in here
Well, I see two ways to do this:
1) create a SelectorTrackerVisitor abstract class which has a
SelectorTracker
instance and makes the calls on it. Subclasses would have to call
the
super methods where appropriate, but then that doesn't seem much
better
than just calling the single method on the SelectorTracker instance.
Also,
for cases where you need different visitors for different selector
types,
such as for code generation, it actually makes things more
complicated.
2) create a new visitor API for the selector to call, so on
visitMessage it
does its thing, then calls the new API method for visiting a
message. Client
code then implement the new API, and visitMessage is final to keep
them from
overriding it. However, this seems duplicative, and makes it a
major change
to change a visitor to care about selectors/forms.
So, at first I thought it was a great suggestion, but after starting to
implement
it I found it didn't really help.
http://gwt-code-reviews.appspot.com/1355802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
http://gwt-code-reviews.appspot.com/1355802/diff/15003/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/15003/user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java#newcode26
user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java:26: *
interface Foo extends Messages {
This is s helpful. Thanks.
http://gwt-code-reviews.appspot.com/1355802/diff/15003/user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java#newcode44
user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java:44: *
mv.visitMessage(["one", "FEMALE"], false, msgStyle, "1");
These are actually visitTranslation, right?
http://gwt-code-reviews.appspot.com/1355802/diff/15003/user/src/com/google/gwt/i18n/server/SelectorTracker.java
File user/src/com/google/gwt/i18n/server/SelectorTracker.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/15003/user/src/com/google/gwt/i18n/server/SelectorTracker.java#newcode26
user/src/com/google/gwt/i18n/server/SelectorTracker.java:26: public
class SelectorTracker {
So I'm responsible for calling methods on this thing from
MIV#visitMessage, MV#endMessage, and MV#visitTranslation, right?
Complicated, although looking at LocalizableGeneratorVisitor I can see
why.
Do you think it would be possible to build this into a reusable
implementation of MessageInterfaceVisitor and MessageVisitor? Something
that LocalizableGeneratorVisitor could extend?
abstract class SelectorTrackingVisitor implements
MessageInterfaceVisitor, MessageVisitor {
/**
* Provides parameter processing via calls to {@link
#processParameter}.
*/
public SelectorTrackingVisitor visitMessage(...) { ... }
/**
* Called from {@link #visitMessage} for each parameter in the
message.
* Returns a FormVisitor for this parameter, or null. FormVisitors
will
* later be run as appropriate from {@link #visitTranslation}.
*/
protected abstract FormVisitor processParameter(Parameter);
/**
* Calls the FormVisitors accumulated from {@link #processParameter}.
*/
public SelectorTrackingVisitor visitMessage(...) { ... }
}
I'm glossing over the meaning of the int level argument of
SelectorTracker#setFormVisitor(int, MessageFormVisitor), which I don't
understand, hoping it can just be baked in here
http://gwt-code-reviews.appspot.com/1355802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
http://gwt-code-reviews.appspot.com/1355802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
On Thu, Mar 3, 2011 at 8:12 AM, 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: *
> 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
>>
>
> 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 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.w
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
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: *
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
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 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 " + msg
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
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: /*
No real change.
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: /*
no real change
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 {
Since this is nested, how about just calling it Context?
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java#newcode64
user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java:64:
public interface MessageCatalogWriter extends Closeable {
ditto, MessageCatalog.Writer
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 {
not thrown
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: *
Isn't this doc redundant with the MessageInterfaceVisitor doc?
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)}.
You're talking about the default message, but it's not used in this
interface at all.
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#newcode24
user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java:24: *
should be, for example:
This example would still be a lot easier to understand if you first
showed the interface that's being visited, where that interface makes an
effort to include the terms like selector and form that appear in the
visitor api.
And it might not be necessary at all if we can get this down to fewer
interfaces and fewer methods.
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);}
{@code} is redundant with
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);
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);}
Don't forget to close that section
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);
Where the write method would be implemented something like:
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/MessageFormatParser.java
File user/src/com/google/gwt/i18n/rebind/MessageFormatParser.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/10001/user/src/com/google/gwt/i18n/rebind/MessageFormatParser.java#newcode32
user/src/com/google/gwt/i18n/rebind/MessageFormatParser.java:32: public
class MessageFormatParser {
I plan to rewrite this one to call the one in the new location before
submitting rather than having mostly-identical implementations.
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#newcode62
user/src/com/google/gwt/i18n/server/MessageInterfaceVisitor.java:62: *
{@code miv.endClass(msgIntf);}
I renamed a couple of calls to make them clearer, and moved beginMessage
into visitMessage.
Other than that one, I think all the hooks make sense and are used by
one of the three implementations.
Doing away with the three places the default message is supplied means a
style of visitor gets a lot more complicated and has to retain state.
For example, if we only had the Before call, a visitor could simply save
the value then and then use it at the endMessage call if it wanted to
process it. Not bad, but one that wanted to process it in-order with
the other implementations gets a lot more complicated to figure out
where it would have gone, noticing when the next set of forms in a
processMessage call has passed the default slot. It also gets a lot
more complicated if you care about nesting of selectors/forms, since we
may have changed an arbitrary number of them at this point.
If you still think these calls should be simplified, can you give a
specific example of what you would like combined?
http://gwt-code-reviews.appspot.com/1355802/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
http://gwt-code-reviews.appspot.com/1355802/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
Re: the type oracle tests, PlaceHistoryGeneratorContextTest is a simpler example. http://gwt-code-reviews.appspot.com/1355802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4026
File user/src/com/google/gwt/i18n/rebind/TypeOracleMessage.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4026#newcode47
user/src/com/google/gwt/i18n/rebind/TypeOracleMessage.java:47: * An
implementation of the {@link Message} API on top of type oracle.
On 2011/02/16 14:25:17, jat wrote:
On 2011/02/16 06:05:36, jat wrote:
> On 2011/02/16 00:13:34, rjrjr wrote:
> > Now is the time to make unit tests of these things. It's pretty
easy to set
up
> a
> > mock type oracle environment these days, via
CompilationStateBuilder and
> > JavaResourceBase.getStandardResources().
> >
> > As an example, look at
> > com.google.gwt.uibinder.elementparsers.ElementParserTester, used
by
> > DockLayoutPanelParserTest
>
> Ok, thanks.
Looking at that example, do I have to mock all the
interfaces/annotations which
are used, plus all the referenced JRE classes? Ideally, I could just
use the
real ones (since there is nothing to mock) and mock just example uses
of them,
but that doesn't seem possible.
Yes, you have to mock them. E.g., UiJavaResources has accumulated as
much of the widget set as has been needed by UiBinder tests. It's worth
the effort, though. This is the only practical way to test fail paths.
And since they're JRE tests they run pretty fast.
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4030
File
user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4030#newcode44
user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java:44:
interface MessageCatalogContext {
On 2011/02/16 06:05:36, jat wrote:
On 2011/02/16 00:13:34, rjrjr wrote:
> Why nest these interfaces?
They are only used in the context of MessageCatalogFactory, so it
seems more
useful to define them here rather than separate classes and make you
figure out
what they are used by.
I guess I was over-sensitized to nesting by the time I wrote this, I
take your point here. With the nested static classes it was getting
difficult to keep track of what I was reading.
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4042
File user/src/com/google/gwt/i18n/server/AbstractMessage.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4042#newcode283
user/src/com/google/gwt/i18n/server/AbstractMessage.java:283: public
void accept(MessageVisitor mv, MessageTranslation trans)
On 2011/02/16 06:05:36, jat wrote:
On 2011/02/16 00:13:34, rjrjr wrote:
> Why is this public? Why does it exist at all, if mv and trans are
always the
> same object?
They are here, but in the code-generation case MessageTranslation will
be
derived from the external translated files, such as property files.
Does it need to be public?
I'm still really unclear on just what it is that a MessageVisitor will
iterate over, what the relationship is between Message and
MessageTranslation. Will it iterate over all translations, one of which
happens to be the defining message?
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4050
File user/src/com/google/gwt/i18n/server/MessageFormVisitor.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4050#newcode58
user/src/com/google/gwt/i18n/server/MessageFormVisitor.java:58: * styles
of visitors.
On 2011/02/16 06:05:36, jat wrote:
On 2011/02/16 00:13:34, rjrjr wrote:
> How is this redundant call making my life simpler?
Look at the 3 implementations (including the internal one) -- they all
have
different needs for when they use that information. Even if I
required the
visitors to save it and use it when they actually needed it, it would
greatly
complicate life for the visitors that want to treat it like any
"other"/"other"/etc form, mixed in with the others -- they would have
to detect
the proper place in the sequence of processMessage calls and inject it
themselves.
> It's also mis-documented. You
> actually have a before and after calls for the default.
Good catch, I originally only had it called after and then discovered
I needed
it before as well.
> Also confusing that most of the methods mentioned here are declared
in other
> interfaces.
I think there should be some documentation somewhere giving the
overall sequence
of visitor calls. Where would you suggest?
I would expect it in the top level visitor, which I guess is
ClassVisitor? The other interfaces should have @see items pointing
there.
In the same place I would expect an explanation of the structure we're
navigating. What's a Message, what's a MessageTranslation, selector, a
parameter, a form, who holds how many of what. Also, your typical
sequence would be easier to read if we could see the interface it's
traversing.
Rather than bare message names, you could qualify them with the
interface names, e.g.:
FormVisitor#beginForm(1, "other")
I looked at the ASM visitors, and they're a lot simpler than this. Every
method
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4026
File user/src/com/google/gwt/i18n/rebind/TypeOracleMessage.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4026#newcode47
user/src/com/google/gwt/i18n/rebind/TypeOracleMessage.java:47: * An
implementation of the {@link Message} API on top of type oracle.
On 2011/02/16 06:05:36, jat wrote:
On 2011/02/16 00:13:34, rjrjr wrote:
> Now is the time to make unit tests of these things. It's pretty easy
to set up
a
> mock type oracle environment these days, via CompilationStateBuilder
and
> JavaResourceBase.getStandardResources().
>
> As an example, look at
> com.google.gwt.uibinder.elementparsers.ElementParserTester, used by
> DockLayoutPanelParserTest
Ok, thanks.
Looking at that example, do I have to mock all the
interfaces/annotations which are used, plus all the referenced JRE
classes? Ideally, I could just use the real ones (since there is
nothing to mock) and mock just example uses of them, but that doesn't
seem possible.
http://gwt-code-reviews.appspot.com/1355802/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
Thanks for taking a look.
Regarding your general comments, the overall structure of the visitors
is modeled after ASM visitors. The default visitor which provides
do-nothing implementations of all the visitors is also straight from
ASM, and provides an easy way to implement just what you want and also
provides future compatibility so if a method is added to an interface
your code will keep working as long as there is a reasonable default
implementation.
For the specifics of why the particular break-down was chosen, please
look at the three implementations of the visitor API --
LocalizableGeneratorVisitor and PropertyCatalogFactory here and the
internal implementation. I don't think your proposed simplification is
sufficient for all three needs.
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4024
File
user/src/com/google/gwt/i18n/rebind/AbstractLocalizableImplCreator.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4024#newcode64
user/src/com/google/gwt/i18n/rebind/AbstractLocalizableImplCreator.java:64:
public static class MessageCatalogContextImpl
Ok. It didn't seem useful outside of this case, which is why I made it
a static inner class.
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4024#newcode313
user/src/com/google/gwt/i18n/rebind/AbstractLocalizableImplCreator.java:313:
if (msgCatFactory != null) {
On 2011/02/16 00:13:34, rjrjr wrote:
This method is already awfully tall Can these two cases be factored
out into
separate methods?
Ok, though it will take a *lot* of parameters :)
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4026
File user/src/com/google/gwt/i18n/rebind/TypeOracleMessage.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4026#newcode47
user/src/com/google/gwt/i18n/rebind/TypeOracleMessage.java:47: * An
implementation of the {@link Message} API on top of type oracle.
On 2011/02/16 00:13:34, rjrjr wrote:
Now is the time to make unit tests of these things. It's pretty easy
to set up a
mock type oracle environment these days, via CompilationStateBuilder
and
JavaResourceBase.getStandardResources().
As an example, look at
com.google.gwt.uibinder.elementparsers.ElementParserTester, used by
DockLayoutPanelParserTest
Ok, thanks.
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4026#newcode49
user/src/com/google/gwt/i18n/rebind/TypeOracleMessage.java:49: public
class TypeOracleMessage extends AbstractMessage implements Message {
On 2011/02/16 00:13:34, rjrjr wrote:
redundant implements. This happens all over the place.
Done.
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4026#newcode244
user/src/com/google/gwt/i18n/rebind/TypeOracleMessage.java:244:
JClassType list = oracle.findType("java.util.List");
On 2011/02/16 00:13:34, rjrjr wrote:
Why strings instead of List.class.getCanonicalName(), etc.?
Done.
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4030
File
user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java
(right):
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4030#newcode44
user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java:44:
interface MessageCatalogContext {
On 2011/02/16 00:13:34, rjrjr wrote:
Why nest these interfaces?
They are only used in the context of MessageCatalogFactory, so it seems
more useful to define them here rather than separate classes and make
you figure out what they are used by.
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4030#newcode71
user/src/com/google/gwt/i18n/rebind/format/MessageCatalogFactory.java:71:
void close() throws IOException;
On 2011/02/16 00:13:34, rjrjr wrote:
Why redeclare?
I personally have found it useful to declare these explicitly to make it
clear what contract an implementation has to fulfill. I agree with the
IDE's help it is of limited value, so if you prefer I will remove it.
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4042
File user/src/com/google/gwt/i18n/server/AbstractMessage.java (right):
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4042#newcode55
user/src/com/google/gwt/i18n/server/AbstractMessage.java:55: public
abstract class AbstractParameter implements Parameter {
On 2011/02/16 00:13:34, rjrjr wrote:
Does this actually need to be an inner class? All it uses is
localeFactory, why
not just pass that in to its constructor?
For subclasses, they will generally need more information from the
*Message class. I originally had it separate, and then found in my
implementations I had to just pass the corresponding *Message
implementation into the *Parameter implementation, so I figured just
make it a non-static class and let the compiler do that for me.
http://gwt-code-reviews.appspot.com/1355802/diff/4012/4042#newcode283
user/src/com/google/gwt/i18n/server/AbstractMessage.java:283: public
void accept(MessageVisitor mv, MessageTranslation trans)
On 2011/02/16 00:13:34, rjrjr wrote:
Why is this public? Why does it exist at all, if mv and trans
[gwt-contrib] Re: Phase 1 of I18n Rewrite - support extended plurals/etc for export to property/etc files (issue1355802)
http://gwt-code-reviews.appspot.com/1355802/diff/1/17 File user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java (right): http://gwt-code-reviews.appspot.com/1355802/diff/1/17#newcode2 user/src/com/google/gwt/i18n/rebind/MessagesMethodCreator.java:2: * Copyright 2008 Google Inc. Ignore the changes here, some auto-formatting cheese obscures an insignificant change (removing an unused variable). http://gwt-code-reviews.appspot.com/1355802/diff/1/36 File user/src/com/google/gwt/i18n/shared/AlternateMessageSelector.java (right): http://gwt-code-reviews.appspot.com/1355802/diff/1/36#newcode121 user/src/com/google/gwt/i18n/shared/AlternateMessageSelector.java:121: // int getArgumentIndex(); Sorry, all the commented stuff will be removed. http://gwt-code-reviews.appspot.com/1355802/diff/1/5 File user/test/com/google/gwt/i18n/rebind/format/PropertyCatalogFactoryTest.java (right): http://gwt-code-reviews.appspot.com/1355802/diff/1/5#newcode74 user/test/com/google/gwt/i18n/rebind/format/PropertyCatalogFactoryTest.java:74: PropertyCatalogFactory.SELECTOR_BOILERPLATE_2, I think this boilerplate is probably misleading, so I will figure out a way to do something different for the Map case. The import also needs to be modified to support this format, which aside from consistency with the rest avoids key conflicts with map keys. http://gwt-code-reviews.appspot.com/1355802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
