some dead local store to fix

[Sandro Martini <sa...@gmail.com>]

Hi to all,
take a look at these dead local store:

- TerraListButtonSkin :

    public int getPreferredWidth(int height) {

// TODO: remove this ...
        // Include padding in constraint
        if (height != -1) {
            height = Math.max(height - (padding.top + padding.bottom + 2), 0);
        }

 and the same also in TerraCalendarButtonSkin


- (optional), in Exponential :
instead of
        return change / 2f * ((float)-Math.pow(2, -10 * --time) + 2) + begin;
should be
        return change / 2f * ((float)-Math.pow(2, -10 * (time - 1)) +
2) + begin;


- last, this: Found static field of type java.text.DateFormat in
TableViewDateCellRenderer
instead of
    public static final DateFormat DEFAULT_DATE_FORMAT =
DateFormat.getDateInstance();
i think should be
    protected static final DateFormat DEFAULT_DATE_FORMAT =
DateFormat.getDateInstance();
or if not protected, at least could it private ...

Then,
in the render method, someone has tried to add a synchronized block in
the format call ?
And a last thing: inside the render method, why not handle also Calendar types ?
Or more in general if i could handle the format there with a custom
method (with default behavior as currently) overridable.
What do you think ?


Bye

Re: some dead local store to fix

[Greg Brown <gk...@mac.com>]

>>> - last, this: Found static field of type java.text.DateFormat in
>>> TableViewDateCellRenderer
>> Why not leave it public? I don't see any compelling reason to hide  
>> it.
> To not expose the field outside the class, in this way it's not
> possible to have multithreading problems in case of wrong usage when
> exposed.

This field should only be accessed from the EDT, so it's OK to leave  
it public.

>>> in the render method, someone has tried to add a synchronized  
>>> block in
>>> the format call ?
>>
>> Are you suggesting that we should add a synchronized block here? If  
>> so, why?
>> All UI methods are executed on the EDT, so there shouldn't be any  
>> contention
>> issues.
> Because DateFormats (et similar) are not multithreading safe, but if
> the method are called in this way i see no problem.

The entire org.apache.pivot.wtk package isn't thread-safe, so this is  
also OK.  :-)

Re: some dead local store to fix

[Sandro Martini <sa...@gmail.com>]

Hi Greg,

>> - TerraListButtonSkin :
>> and the same also in TerraCalendarButtonSkin
>
> I have fixed these.
Great, I'll wait your commit.

>> - (optional), in Exponential :
Ok, done.

>> - last, this: Found static field of type java.text.DateFormat in
>> TableViewDateCellRenderer
> Why not leave it public? I don't see any compelling reason to hide it.
To not expose the field outside the class, in this way it's not
possible to have multithreading problems in case of wrong usage when
exposed.

Tell me if protected is Ok (after this Findbugs tell it's Ok), our if
you want to make this patch ...


>> in the render method, someone has tried to add a synchronized block in
>> the format call ?
>
> Are you suggesting that we should add a synchronized block here? If so, why?
> All UI methods are executed on the EDT, so there shouldn't be any contention
> issues.
Because DateFormats (et similar) are not multithreading safe, but if
the method are called in this way i see no problem. I've read that in
Java 7 there should be additional annotations to help code tools to
analyze better the code, to trap these cases for example ... but at
the moment we can live without this.

> I think that, if you want custom rendering behavior, you should write a
> custom renderer.  :-)
Yes, you know, usually i try to handle as much cases as i can,
sometimes with unnecessary complications in existing code :-) .

Byeeee

Re: some dead local store to fix

[Greg Brown <gk...@mac.com>]

> - TerraListButtonSkin :
>
>    public int getPreferredWidth(int height) {
>
> // TODO: remove this ...
>        // Include padding in constraint
>        if (height != -1) {
>            height = Math.max(height - (padding.top + padding.bottom  
> + 2), 0);
>        }
>
> and the same also in TerraCalendarButtonSkin

I have fixed these.

> - (optional), in Exponential :
> instead of
>        return change / 2f * ((float)-Math.pow(2, -10 * --time) + 2)  
> + begin;
> should be
>        return change / 2f * ((float)-Math.pow(2, -10 * (time - 1)) +
> 2) + begin;

That code was ported from Robert Penner's easing equations for Flash.  
Feel free to make this correction. While you are in there, we should  
also correct the spacing: "(float)-Math.pow(" should be "(float) -  
Math.pow(". There are two such cases in this file.

> - last, this: Found static field of type java.text.DateFormat in
> TableViewDateCellRenderer
> instead of
>    public static final DateFormat DEFAULT_DATE_FORMAT =
> DateFormat.getDateInstance();
> i think should be
>    protected static final DateFormat DEFAULT_DATE_FORMAT =
> DateFormat.getDateInstance();
> or if not protected, at least could it private ...

Why not leave it public? I don't see any compelling reason to hide it.

> Then,
> in the render method, someone has tried to add a synchronized block in
> the format call ?

Are you suggesting that we should add a synchronized block here? If  
so, why? All UI methods are executed on the EDT, so there shouldn't be  
any contention issues.

> And a last thing: inside the render method, why not handle also  
> Calendar types ?
> Or more in general if i could handle the format there with a custom
> method (with default behavior as currently) overridable.
> What do you think ?

I think that, if you want custom rendering behavior, you should write  
a custom renderer.  :-)