You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pivot.apache.org by Christopher Brind <br...@brindy.org.uk> on 2009/08/05 17:18:49 UTC
PIVOT-180
Hi,
This is aimed at Todd specifically, but just to throw it out there...
http://svn.apache.org/repos/asf/incubator/pivot/trunk/core/src/org/apache/pivot/util/Resources.java
(None of this is committed yet.)
I've added a load of extra constructors which take a parent resource as the
first argument, e.g.
> public Resources(String baseName) throws IOException,
> SerializationException {
> this(null, baseName, Locale.getDefault(),
> Charset.defaultCharset());
> }
>
Now has a matching constructor:
> public Resources(Resources parent, String baseName) throws IOException,
> SerializationException {
> this(parent, baseName, Locale.getDefault(),
> Charset.defaultCharset());
> }
>
Ultimately all constructors call the full argument constructor via this()
passing the parent as null as appropriate.
This is the general approach I'm taking for delegating to the parent
resources:
Previous:
> public String getString(String key) {
> return JSONSerializer.getString(resourceMap, key);
> }
>
New:
> public String getString(String key) {
> String s = JSONSerializer.getString(resourceMap, key);
> if (s == null && parent != null) {
> return parent.getString(key);
> }
> return s;
> }
>
Have written a test which confirms one of the new constructors and
getString() but plan to add more tests to cover all the constructors and
each of the getXXX types.
Do you need anything more than that, Todd? Does any of the WTK code need to
change to support your requirement?
Cheers,
Chris
Re: PIVOT-180
Posted by Christopher Brind <br...@brindy.org.uk>.
No problem, that's in now.
Collections unit tests next, unless there is something more urgent. :)
Cheers,
Chris
2009/8/5 Todd Volkert <tv...@gmail.com>
> Yep - that patch looks correct. That's a lot simpler in code than it was in
> English :)
>
> Thanks Chris,
> -T
>
> On Wed, Aug 5, 2009 at 5:03 PM, Christopher Brind <brindy@brindy.org.uk
> >wrote:
>
> > So Todd, are you basically talking about the patch below? If so, let me
> > know and I'll commit.
> >
> >
> > Index:
> >
> >
> /Users/brindy/Workspaces/pivot/wtk/src/org/apache/pivot/wtkx/WTKXSerializer.java
> > ===================================================================
> > ---
> >
> >
> /Users/brindy/Workspaces/pivot/wtk/src/org/apache/pivot/wtkx/WTKXSerializer.java
> > (revision 801388)
> > +++
> >
> >
> /Users/brindy/Workspaces/pivot/wtk/src/org/apache/pivot/wtkx/WTKXSerializer.java
> > (working copy)
> > @@ -524,7 +524,7 @@
> > if
> > (attribute.localName.equals(INCLUDE_SRC_ATTRIBUTE)) {
> > src =
> attribute.value;
> > } else if
> > (attribute.localName.equals(INCLUDE_RESOURCES_ATTRIBUTE)) {
> > - resources = new
> > Resources(attribute.value);
> > + resources = new
> > Resources(resources, attribute.value);
> > } else if
> > (attribute.localName.equals(INCLUDE_ASYNCHRONOUS_ATTRIBUTE)) {
> > // TODO
> > throw new
> > UnsupportedOperationException("Asynchronous includes are not"
> >
> >
> >
> > Greg, I know you don't like creating private methods, but that readObject
> > method is 574 lines long really hard to read and follow. Is there any
> > chance we can break it down in to something more manageable, please?
> >
> > Cheers,
> > Chris
> >
> >
> > 2009/8/5 Greg Brown <gk...@mac.com>
> >
> > > Yes - thanks!
> > >
> > > On Wednesday, August 05, 2009, at 04:13PM, "Christopher Brind" <
> > > brindy@brindy.org.uk> wrote:
> > > >I've committed the change and a test, feel free to have a look and let
> > me
> > > >know what you think.
> > > >
> > > >I notice the WTKXSerializer class has been updated - is it OK for me
> to
> > > >change that now?
> > > >
> > > >Cheers,
> > > >Chris
> > > >
> > > >
> > > >2009/8/5 Todd Volkert <tv...@gmail.com>
> > > >
> > > >> > I've added a load of extra constructors which take a parent
> resource
> > > as
> > > >> the
> > > >> > first argument, e.g.
> > > >> >
> > > >> > > public Resources(String baseName) throws IOException,
> > > >> > > SerializationException {
> > > >> > > this(null, baseName, Locale.getDefault(),
> > > >> > > Charset.defaultCharset());
> > > >> > > }
> > > >> > >
> > > >> >
> > > >> > Now has a matching constructor:
> > > >> >
> > > >> > > public Resources(Resources parent, String baseName) throws
> > > >> > IOException,
> > > >> > > SerializationException {
> > > >> > > this(parent, baseName, Locale.getDefault(),
> > > >> > > Charset.defaultCharset());
> > > >> > > }
> > > >> > >
> > > >> >
> > > >> > Ultimately all constructors call the full argument constructor via
> > > this()
> > > >> > passing the parent as null as appropriate.
> > > >>
> > > >>
> > > >> That sounds great.
> > > >>
> > > >>
> > > >> > This is the general approach I'm taking for delegating to the
> parent
> > > >> > resources:
> > > >> >
> > > >> > Previous:
> > > >> >
> > > >> > > public String getString(String key) {
> > > >> > > return JSONSerializer.getString(resourceMap, key);
> > > >> > > }
> > > >> > >
> > > >> >
> > > >> > New:
> > > >> >
> > > >> > > public String getString(String key) {
> > > >> > > String s = JSONSerializer.getString(resourceMap, key);
> > > >> > > if (s == null && parent != null) {
> > > >> > > return parent.getString(key);
> > > >> > > }
> > > >> > > return s;
> > > >> > > }
> > > >> > >
> > > >>
> > > >>
> > > >> Looks good to me.
> > > >>
> > > >> Have written a test which confirms one of the new constructors and
> > > >> > getString() but plan to add more tests to cover all the
> constructors
> > > and
> > > >> > each of the getXXX types.
> > > >> >
> > > >> > Do you need anything more than that, Todd? Does any of the WTK
> code
> > > need
> > > >> > to
> > > >> > change to support your requirement?
> > > >>
> > > >>
> > > >> Yeah, there needs to be a small change to WTKXSerializer, where we
> > deal
> > > >> with
> > > >> <wtkx:include>. Right now, the logic is that we pass our existing
> > > >> resources
> > > >> down to the include serializer, unless a resources="..." attribute
> was
> > > >> found
> > > >> in our include tag, in which case we *replace* the resources that's
> > > passed
> > > >> to the include serializer with the specified resources. The logic
> > > should
> > > >> change such that instantiate a new resources with our existing
> > resources
> > > >> *as
> > > >> its parent*, then use that new resources to pass to the include
> > > serializer.
> > > >>
> > > >> However, I think Greg's making a somewhat significant change to
> > > >> WTKXSerializer right now, so you could commit the rest first, then
> > > update
> > > >> WTKXSerializer after you see Greg's update.
> > > >>
> > > >> -T
> > > >>
> > > >
> > > >
> > >
> >
>
Re: PIVOT-180
Posted by Todd Volkert <tv...@gmail.com>.
Yep - that patch looks correct. That's a lot simpler in code than it was in
English :)
Thanks Chris,
-T
On Wed, Aug 5, 2009 at 5:03 PM, Christopher Brind <br...@brindy.org.uk>wrote:
> So Todd, are you basically talking about the patch below? If so, let me
> know and I'll commit.
>
>
> Index:
>
> /Users/brindy/Workspaces/pivot/wtk/src/org/apache/pivot/wtkx/WTKXSerializer.java
> ===================================================================
> ---
>
> /Users/brindy/Workspaces/pivot/wtk/src/org/apache/pivot/wtkx/WTKXSerializer.java
> (revision 801388)
> +++
>
> /Users/brindy/Workspaces/pivot/wtk/src/org/apache/pivot/wtkx/WTKXSerializer.java
> (working copy)
> @@ -524,7 +524,7 @@
> if
> (attribute.localName.equals(INCLUDE_SRC_ATTRIBUTE)) {
> src = attribute.value;
> } else if
> (attribute.localName.equals(INCLUDE_RESOURCES_ATTRIBUTE)) {
> - resources = new
> Resources(attribute.value);
> + resources = new
> Resources(resources, attribute.value);
> } else if
> (attribute.localName.equals(INCLUDE_ASYNCHRONOUS_ATTRIBUTE)) {
> // TODO
> throw new
> UnsupportedOperationException("Asynchronous includes are not"
>
>
>
> Greg, I know you don't like creating private methods, but that readObject
> method is 574 lines long really hard to read and follow. Is there any
> chance we can break it down in to something more manageable, please?
>
> Cheers,
> Chris
>
>
> 2009/8/5 Greg Brown <gk...@mac.com>
>
> > Yes - thanks!
> >
> > On Wednesday, August 05, 2009, at 04:13PM, "Christopher Brind" <
> > brindy@brindy.org.uk> wrote:
> > >I've committed the change and a test, feel free to have a look and let
> me
> > >know what you think.
> > >
> > >I notice the WTKXSerializer class has been updated - is it OK for me to
> > >change that now?
> > >
> > >Cheers,
> > >Chris
> > >
> > >
> > >2009/8/5 Todd Volkert <tv...@gmail.com>
> > >
> > >> > I've added a load of extra constructors which take a parent resource
> > as
> > >> the
> > >> > first argument, e.g.
> > >> >
> > >> > > public Resources(String baseName) throws IOException,
> > >> > > SerializationException {
> > >> > > this(null, baseName, Locale.getDefault(),
> > >> > > Charset.defaultCharset());
> > >> > > }
> > >> > >
> > >> >
> > >> > Now has a matching constructor:
> > >> >
> > >> > > public Resources(Resources parent, String baseName) throws
> > >> > IOException,
> > >> > > SerializationException {
> > >> > > this(parent, baseName, Locale.getDefault(),
> > >> > > Charset.defaultCharset());
> > >> > > }
> > >> > >
> > >> >
> > >> > Ultimately all constructors call the full argument constructor via
> > this()
> > >> > passing the parent as null as appropriate.
> > >>
> > >>
> > >> That sounds great.
> > >>
> > >>
> > >> > This is the general approach I'm taking for delegating to the parent
> > >> > resources:
> > >> >
> > >> > Previous:
> > >> >
> > >> > > public String getString(String key) {
> > >> > > return JSONSerializer.getString(resourceMap, key);
> > >> > > }
> > >> > >
> > >> >
> > >> > New:
> > >> >
> > >> > > public String getString(String key) {
> > >> > > String s = JSONSerializer.getString(resourceMap, key);
> > >> > > if (s == null && parent != null) {
> > >> > > return parent.getString(key);
> > >> > > }
> > >> > > return s;
> > >> > > }
> > >> > >
> > >>
> > >>
> > >> Looks good to me.
> > >>
> > >> Have written a test which confirms one of the new constructors and
> > >> > getString() but plan to add more tests to cover all the constructors
> > and
> > >> > each of the getXXX types.
> > >> >
> > >> > Do you need anything more than that, Todd? Does any of the WTK code
> > need
> > >> > to
> > >> > change to support your requirement?
> > >>
> > >>
> > >> Yeah, there needs to be a small change to WTKXSerializer, where we
> deal
> > >> with
> > >> <wtkx:include>. Right now, the logic is that we pass our existing
> > >> resources
> > >> down to the include serializer, unless a resources="..." attribute was
> > >> found
> > >> in our include tag, in which case we *replace* the resources that's
> > passed
> > >> to the include serializer with the specified resources. The logic
> > should
> > >> change such that instantiate a new resources with our existing
> resources
> > >> *as
> > >> its parent*, then use that new resources to pass to the include
> > serializer.
> > >>
> > >> However, I think Greg's making a somewhat significant change to
> > >> WTKXSerializer right now, so you could commit the rest first, then
> > update
> > >> WTKXSerializer after you see Greg's update.
> > >>
> > >> -T
> > >>
> > >
> > >
> >
>
Re: PIVOT-180
Posted by Greg Brown <gk...@mac.com>.
>Greg, I know you don't like creating private methods, but that readObject
>method is 574 lines long really hard to read and follow. Is there any
>chance we can break it down in to something more manageable, please?
I actually don't have any specific opposition to private methods. I'm just not sure that there is a clean way to break that method up. I've revised that class a number of times already, and I think it's currently pretty clean. It just does a lot. :-)
Re: PIVOT-180
Posted by Christopher Brind <br...@brindy.org.uk>.
So Todd, are you basically talking about the patch below? If so, let me
know and I'll commit.
Index:
/Users/brindy/Workspaces/pivot/wtk/src/org/apache/pivot/wtkx/WTKXSerializer.java
===================================================================
---
/Users/brindy/Workspaces/pivot/wtk/src/org/apache/pivot/wtkx/WTKXSerializer.java
(revision 801388)
+++
/Users/brindy/Workspaces/pivot/wtk/src/org/apache/pivot/wtkx/WTKXSerializer.java
(working copy)
@@ -524,7 +524,7 @@
if
(attribute.localName.equals(INCLUDE_SRC_ATTRIBUTE)) {
src = attribute.value;
} else if
(attribute.localName.equals(INCLUDE_RESOURCES_ATTRIBUTE)) {
- resources = new
Resources(attribute.value);
+ resources = new
Resources(resources, attribute.value);
} else if
(attribute.localName.equals(INCLUDE_ASYNCHRONOUS_ATTRIBUTE)) {
// TODO
throw new
UnsupportedOperationException("Asynchronous includes are not"
Greg, I know you don't like creating private methods, but that readObject
method is 574 lines long really hard to read and follow. Is there any
chance we can break it down in to something more manageable, please?
Cheers,
Chris
2009/8/5 Greg Brown <gk...@mac.com>
> Yes - thanks!
>
> On Wednesday, August 05, 2009, at 04:13PM, "Christopher Brind" <
> brindy@brindy.org.uk> wrote:
> >I've committed the change and a test, feel free to have a look and let me
> >know what you think.
> >
> >I notice the WTKXSerializer class has been updated - is it OK for me to
> >change that now?
> >
> >Cheers,
> >Chris
> >
> >
> >2009/8/5 Todd Volkert <tv...@gmail.com>
> >
> >> > I've added a load of extra constructors which take a parent resource
> as
> >> the
> >> > first argument, e.g.
> >> >
> >> > > public Resources(String baseName) throws IOException,
> >> > > SerializationException {
> >> > > this(null, baseName, Locale.getDefault(),
> >> > > Charset.defaultCharset());
> >> > > }
> >> > >
> >> >
> >> > Now has a matching constructor:
> >> >
> >> > > public Resources(Resources parent, String baseName) throws
> >> > IOException,
> >> > > SerializationException {
> >> > > this(parent, baseName, Locale.getDefault(),
> >> > > Charset.defaultCharset());
> >> > > }
> >> > >
> >> >
> >> > Ultimately all constructors call the full argument constructor via
> this()
> >> > passing the parent as null as appropriate.
> >>
> >>
> >> That sounds great.
> >>
> >>
> >> > This is the general approach I'm taking for delegating to the parent
> >> > resources:
> >> >
> >> > Previous:
> >> >
> >> > > public String getString(String key) {
> >> > > return JSONSerializer.getString(resourceMap, key);
> >> > > }
> >> > >
> >> >
> >> > New:
> >> >
> >> > > public String getString(String key) {
> >> > > String s = JSONSerializer.getString(resourceMap, key);
> >> > > if (s == null && parent != null) {
> >> > > return parent.getString(key);
> >> > > }
> >> > > return s;
> >> > > }
> >> > >
> >>
> >>
> >> Looks good to me.
> >>
> >> Have written a test which confirms one of the new constructors and
> >> > getString() but plan to add more tests to cover all the constructors
> and
> >> > each of the getXXX types.
> >> >
> >> > Do you need anything more than that, Todd? Does any of the WTK code
> need
> >> > to
> >> > change to support your requirement?
> >>
> >>
> >> Yeah, there needs to be a small change to WTKXSerializer, where we deal
> >> with
> >> <wtkx:include>. Right now, the logic is that we pass our existing
> >> resources
> >> down to the include serializer, unless a resources="..." attribute was
> >> found
> >> in our include tag, in which case we *replace* the resources that's
> passed
> >> to the include serializer with the specified resources. The logic
> should
> >> change such that instantiate a new resources with our existing resources
> >> *as
> >> its parent*, then use that new resources to pass to the include
> serializer.
> >>
> >> However, I think Greg's making a somewhat significant change to
> >> WTKXSerializer right now, so you could commit the rest first, then
> update
> >> WTKXSerializer after you see Greg's update.
> >>
> >> -T
> >>
> >
> >
>
Re: PIVOT-180
Posted by Greg Brown <gk...@mac.com>.
Yes - thanks!
On Wednesday, August 05, 2009, at 04:13PM, "Christopher Brind" <br...@brindy.org.uk> wrote:
>I've committed the change and a test, feel free to have a look and let me
>know what you think.
>
>I notice the WTKXSerializer class has been updated - is it OK for me to
>change that now?
>
>Cheers,
>Chris
>
>
>2009/8/5 Todd Volkert <tv...@gmail.com>
>
>> > I've added a load of extra constructors which take a parent resource as
>> the
>> > first argument, e.g.
>> >
>> > > public Resources(String baseName) throws IOException,
>> > > SerializationException {
>> > > this(null, baseName, Locale.getDefault(),
>> > > Charset.defaultCharset());
>> > > }
>> > >
>> >
>> > Now has a matching constructor:
>> >
>> > > public Resources(Resources parent, String baseName) throws
>> > IOException,
>> > > SerializationException {
>> > > this(parent, baseName, Locale.getDefault(),
>> > > Charset.defaultCharset());
>> > > }
>> > >
>> >
>> > Ultimately all constructors call the full argument constructor via this()
>> > passing the parent as null as appropriate.
>>
>>
>> That sounds great.
>>
>>
>> > This is the general approach I'm taking for delegating to the parent
>> > resources:
>> >
>> > Previous:
>> >
>> > > public String getString(String key) {
>> > > return JSONSerializer.getString(resourceMap, key);
>> > > }
>> > >
>> >
>> > New:
>> >
>> > > public String getString(String key) {
>> > > String s = JSONSerializer.getString(resourceMap, key);
>> > > if (s == null && parent != null) {
>> > > return parent.getString(key);
>> > > }
>> > > return s;
>> > > }
>> > >
>>
>>
>> Looks good to me.
>>
>> Have written a test which confirms one of the new constructors and
>> > getString() but plan to add more tests to cover all the constructors and
>> > each of the getXXX types.
>> >
>> > Do you need anything more than that, Todd? Does any of the WTK code need
>> > to
>> > change to support your requirement?
>>
>>
>> Yeah, there needs to be a small change to WTKXSerializer, where we deal
>> with
>> <wtkx:include>. Right now, the logic is that we pass our existing
>> resources
>> down to the include serializer, unless a resources="..." attribute was
>> found
>> in our include tag, in which case we *replace* the resources that's passed
>> to the include serializer with the specified resources. The logic should
>> change such that instantiate a new resources with our existing resources
>> *as
>> its parent*, then use that new resources to pass to the include serializer.
>>
>> However, I think Greg's making a somewhat significant change to
>> WTKXSerializer right now, so you could commit the rest first, then update
>> WTKXSerializer after you see Greg's update.
>>
>> -T
>>
>
>
Re: PIVOT-180
Posted by Christopher Brind <br...@brindy.org.uk>.
I've committed the change and a test, feel free to have a look and let me
know what you think.
I notice the WTKXSerializer class has been updated - is it OK for me to
change that now?
Cheers,
Chris
2009/8/5 Todd Volkert <tv...@gmail.com>
> > I've added a load of extra constructors which take a parent resource as
> the
> > first argument, e.g.
> >
> > > public Resources(String baseName) throws IOException,
> > > SerializationException {
> > > this(null, baseName, Locale.getDefault(),
> > > Charset.defaultCharset());
> > > }
> > >
> >
> > Now has a matching constructor:
> >
> > > public Resources(Resources parent, String baseName) throws
> > IOException,
> > > SerializationException {
> > > this(parent, baseName, Locale.getDefault(),
> > > Charset.defaultCharset());
> > > }
> > >
> >
> > Ultimately all constructors call the full argument constructor via this()
> > passing the parent as null as appropriate.
>
>
> That sounds great.
>
>
> > This is the general approach I'm taking for delegating to the parent
> > resources:
> >
> > Previous:
> >
> > > public String getString(String key) {
> > > return JSONSerializer.getString(resourceMap, key);
> > > }
> > >
> >
> > New:
> >
> > > public String getString(String key) {
> > > String s = JSONSerializer.getString(resourceMap, key);
> > > if (s == null && parent != null) {
> > > return parent.getString(key);
> > > }
> > > return s;
> > > }
> > >
>
>
> Looks good to me.
>
> Have written a test which confirms one of the new constructors and
> > getString() but plan to add more tests to cover all the constructors and
> > each of the getXXX types.
> >
> > Do you need anything more than that, Todd? Does any of the WTK code need
> > to
> > change to support your requirement?
>
>
> Yeah, there needs to be a small change to WTKXSerializer, where we deal
> with
> <wtkx:include>. Right now, the logic is that we pass our existing
> resources
> down to the include serializer, unless a resources="..." attribute was
> found
> in our include tag, in which case we *replace* the resources that's passed
> to the include serializer with the specified resources. The logic should
> change such that instantiate a new resources with our existing resources
> *as
> its parent*, then use that new resources to pass to the include serializer.
>
> However, I think Greg's making a somewhat significant change to
> WTKXSerializer right now, so you could commit the rest first, then update
> WTKXSerializer after you see Greg's update.
>
> -T
>
Re: PIVOT-180
Posted by Todd Volkert <tv...@gmail.com>.
> I've added a load of extra constructors which take a parent resource as the
> first argument, e.g.
>
> > public Resources(String baseName) throws IOException,
> > SerializationException {
> > this(null, baseName, Locale.getDefault(),
> > Charset.defaultCharset());
> > }
> >
>
> Now has a matching constructor:
>
> > public Resources(Resources parent, String baseName) throws
> IOException,
> > SerializationException {
> > this(parent, baseName, Locale.getDefault(),
> > Charset.defaultCharset());
> > }
> >
>
> Ultimately all constructors call the full argument constructor via this()
> passing the parent as null as appropriate.
That sounds great.
> This is the general approach I'm taking for delegating to the parent
> resources:
>
> Previous:
>
> > public String getString(String key) {
> > return JSONSerializer.getString(resourceMap, key);
> > }
> >
>
> New:
>
> > public String getString(String key) {
> > String s = JSONSerializer.getString(resourceMap, key);
> > if (s == null && parent != null) {
> > return parent.getString(key);
> > }
> > return s;
> > }
> >
Looks good to me.
Have written a test which confirms one of the new constructors and
> getString() but plan to add more tests to cover all the constructors and
> each of the getXXX types.
>
> Do you need anything more than that, Todd? Does any of the WTK code need
> to
> change to support your requirement?
Yeah, there needs to be a small change to WTKXSerializer, where we deal with
<wtkx:include>. Right now, the logic is that we pass our existing resources
down to the include serializer, unless a resources="..." attribute was found
in our include tag, in which case we *replace* the resources that's passed
to the include serializer with the specified resources. The logic should
change such that instantiate a new resources with our existing resources *as
its parent*, then use that new resources to pass to the include serializer.
However, I think Greg's making a somewhat significant change to
WTKXSerializer right now, so you could commit the rest first, then update
WTKXSerializer after you see Greg's update.
-T