harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Oleg Khaschansky" <oleg.v.khaschan...@gmail.com>
Subject Re: [classlib][awt] Revision #465514 broke image decoders.
Date Wed, 25 Oct 2006 09:26:47 GMT
> Since the ImageConsumer accepts Hashtable<?,?> my thought is that the
> the GifDecoder should declare the field based on what it actually
> uses. If the other decoders only use String keys and String values,
> then I would suggest we change the field declaration to match.

Yes, it sounds reasonable.

On 10/25/06, Nathan Beyer <nbeyer@gmail.com> wrote:
> On 10/24/06, Oleg Khaschansky <oleg.v.khaschansky@gmail.com> wrote:
> > Nathan, could you, please tell why you changed the field properties in
> > these classes to
> > Hashtable<Object,Object> in two of them and to
> > Hashtable<String,String> in one of them (GifDecoder)?
> >
> > Look at the declaration in the ImageConsumer class:
> > void setProperties(Hashtable<?,?> props)
> >
> > It'd be better to have Hashtable<?,?> as a type in all 3 classes and
> > Hashtable<Object,Object> as an initial value for this field.
>
> A field type of Hashtable<?,?> would be painful, as you can't perform
> any sets when a type variable is ?. You'd have to cast it to
> Hashtable<Object,Object> before performing any sets.
>
> Since the ImageConsumer accepts Hashtable<?,?> my thought is that the
> the GifDecoder should declare the field based on what it actually
> uses. If the other decoders only use String keys and String values,
> then I would suggest we change the field declaration to match.
>
> -Nathan
>
> >
> > On 10/24/06, Oleg Khaschansky <oleg.v.khaschansky@gmail.com> wrote:
> > > > > Where are the tests for these decoders? How did you determine that
> > > > > they no longer worked?
> > >
> > > Unfortunately, these classes are not covered with the unit tests.
> > >
> > > I was running a simple test application that did something like this:
> > > Toolkit.getDefaultToolkit().getImage("image.jpg");
> > > and if failed with a NPE.
> > >
> > > > I removed the final modifiers
> > > At the first glance it seems like the problem doesn't appear any more.
> > >
> > > > There were only 3-4 other fields that were finalized. Your email
> > > > mentioned "a lot of invalid modifications"; what are the other issues,
> > > > specifically?
> > > Only final fields. No other issues. But 3-4 in 3 classes - it is alot for me
:)
> > >
> > > On 10/24/06, Nathan Beyer <nbeyer@gmail.com> wrote:
> > > > I removed the final modifiers; this only affected PngDecoder,
> > > > GifDecoder and JpegDecoder. I missed the comments in the fields of
> > > > JpegDecoder, that was my mistake.
> > > >
> > > > There were only 3-4 other fields that were finalized. Your email
> > > > mentioned "a lot of invalid modifications"; what are the other issues,
> > > > specifically?
> > > >
> > > > -Nathan
> > > >
> > > > On 10/24/06, Nathan Beyer <nbeyer@gmail.com> wrote:
> > > > > Where are the tests for these decoders? How did you determine that
> > > > > they no longer worked?
> > > > >
> > > > > I'll remove the final modifiers.
> > > > >
> > > > > -Nathan
> > > > >
> > > > > On 10/24/06, Oleg Khaschansky <oleg.v.khaschansky@gmail.com>
wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Rev. 465514 introduced a lot of invalid modifications to the
> > > > > > GifDecoder, PngDecoder and JpegDecoder. There were a number
of fields
> > > > > > modified or initialized from the native code only, but they
were
> > > > > > redeclared as final, so the decoders doesn't work properly any
more.
> > > > > >
> > > > > > This revision has the following comment:
> > > > > >
> > > > > > Cleanup code
> > > > > > * Add if/else braces
> > > > > > * Add missing annotations
> > > > > > * Add type variables
> > > > > > * Use foreach loops
> > > > > > * etc
> > > > > >
> > > > > > I'd suggest to roll back this revision and redo the cleanup
in the
> > > > > > more accurate way.
> > > > > >
> > > > > > Thanks,
> > > > > >   Oleg
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
View raw message