You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@pivot.apache.org by Sandro Martini <sa...@gmail.com> on 2009/08/05 12:24:56 UTC

Small things

Hi to all,
some other small (easy) things to fix, grouped by bug/suggestion type:

Comparator doesn't implement Serializable
org.apache.pivot.wtk.text.Element$NodeOffsetComparator implements
Comparator but not Serializable
org.apache.pivot.wtk.TableView$RowComparator implements Comparator but
not Serializable
org.apache.pivot.wtk.TreeView$PathComparator implements Comparator but
not Serializable
org.apache.pivot.tutorials.TreeNodeComparator implements Comparator
but not Serializable
org.apache.pivot.collections.List$SimpleComparator implements
Comparator but not Serializable
org.apache.pivot.io.Folder$FileNameComparator implements Comparator
but not Serializable
org.apache.pivot.io.FileList$FilePathComparator implements Comparator
but not Serializable

Folder.java:224 Nullcheck of fileList at line 228 of value previously
dereferenced
        files = new ArrayList<File>(fileList.length);
        listListeners.listCleared(this);

        // Add the new files, firing an insert event for each file
        if (fileList != null) {
in this case, the warning tells that if (fileList != null) is
unnecessary, because in case of null we get a NPE in the line
containing fileList.length . So maybe a better check is to move over
the null check and set a length value to zero in that case.

Dead local store:
TerraPanoramaSkin.java:383 Dead store to scrollLeft
TerraPanoramaSkin.java:376 Dead store to scrollTop
TerraActivityIndicatorSkin.java:87 Dead store to increment
Drawing.java:59 Dead store to bounds

Version.java:104 Equals method for org.apache.pivot.util.Version
assumes the argument is of type Version

Non-serializable value stored into instance field of a serializable class
ApplicationContext.java:97
org.apache.pivot.wtk.ApplicationContext$DisplayHost$1 stored into
non-transient field ApplicationContext$DisplayHost.dropTargetListener
ApplicationContext.java:957 org.apache.pivot.wtk.LocalManifest stored
into non-transient field ApplicationContext$DisplayHost.dragManifest
Solution: make the variable transient.

Non-transient non-serializable instance field in serializable class
ArrayQueue.java:33 Class org.apache.pivot.collections.ArrayQueue
defines non-transient non-serializable instance field queueListeners
LinkedStack.java:31 Class org.apache.pivot.collections.LinkedStack
defines non-transient non-serializable instance field stackListeners
ArrayStack.java:33 Class org.apache.pivot.collections.ArrayStack
defines non-transient non-serializable instance field stackListeners
LinkedQueue.java:31 Class org.apache.pivot.collections.LinkedQueue
defines non-transient non-serializable instance field queueListeners
Solution: make the variable transient.


Serializable inner class
LinkedList.java:37 org.apache.pivot.collections.LinkedList$Node is
serializable and an inner class
Solution: make the inner class static.

Should be a static inner class
MenuButton.java:36 Should
org.apache.pivot.wtk.MenuButton$MenuButtonListenerList be a _static_
inner class?
Element.java:81 Should
org.apache.pivot.wtk.text.Element$ElementListenerList be a _static_
inner class?
ChartView.java:327 Should
org.apache.pivot.charts.ChartView$ChartViewSeriesListenerList be a
_static_ inner class?
Menu.java:41 Should org.apache.pivot.wtk.Menu$Item$ItemListenerList be
a _static_ inner class?
TerraCalendarSkin.java:343 Should
org.apache.pivot.wtk.skin.terra.TerraCalendarSkin$DateButtonDataRenderer
be a _static_ inner class?
ChartView.java:297 Should
org.apache.pivot.charts.ChartView$ChartViewCategoryListenerList be a
_static_ inner class?
ChartView.java:255 Should
org.apache.pivot.charts.ChartView$ChartViewListenerList be a _static_
inner class?
MenuBar.java:40 Should
org.apache.pivot.wtk.MenuBar$Item$ItemListenerList be a _static_ inner
class?
MenuBar.java:191 Should
org.apache.pivot.wtk.MenuBar$MenuBarListenerList be a _static_ inner
class?
Canvas.java:36 Should
org.apache.pivot.wtk.media.drawing.Canvas$CanvasListenerList be a
_static_ inner class?
MenuPopup.java:27 Should
org.apache.pivot.wtk.MenuPopup$MenuPopupListenerList be a _static_
inner class?
TextNode.java:27 Should
org.apache.pivot.wtk.text.TextNode$TextNodeListenerList be a _static_
inner class?

Unchecked/unconfirmed cast
TerraPromptSkin.java:133 Unchecked/unconfirmed cast from
org.apache.pivot.wtk.Window to org.apache.pivot.wtk.Prompt
ShutdownTest.java:52 Unchecked/unconfirmed cast from
org.apache.pivot.wtk.Dialog to org.apache.pivot.wtk.Alert
TerraAlertSkin.java:135 Unchecked/unconfirmed cast from
org.apache.pivot.wtk.Window to org.apache.pivot.wtk.Alert
Solution: check with generics, or at least with instanceof .

ApplicationContext.java:384 Method
org.apache.pivot.wtk.ApplicationContext$DisplayHost.paint(Graphics)
uses the nextDouble method of Random to generate a random integer;
using nextInt is more efficient
ApplicationContext.java:383 Method
org.apache.pivot.wtk.ApplicationContext$DisplayHost.paint(Graphics)
uses the nextDouble method of Random to generate a random integer;
using nextInt is more efficient


This is only the (long) index.

Tell me if i can do the patch, and optional if not on some sources ...
or if you prefer the patch sources here before to apply.


Next, there are other categories, but not so much :-) .
My plan for the 1.3 release is to have as much as things fixed.

Bye

Re: Small things

Posted by Sandro Martini <sa...@gmail.com>.

As you can see from commits, some things have gone.

Tomorrow I'll fix the nextInt suggestion, but before I have to try
some safe high speed solution, and before commit I'll review with all
...

Bye

Re: Small things

Posted by Todd Volkert <tv...@gmail.com>.

>
> Please hold off on this change. It would just add unnecessary complexity to
> the LinkedList class. Node is a private inner class and will never be
> serialized independently of a LinkedList, so the fact that it implicitly
> serializes the outer class as well is not an issue.
>
>
But unless I'm wrong (which is always possible), Node should be made static
anyway, because it doesn't reference any member variables of the containing
class, in which case you might as well save the extra reference to the outer
instance, no?

-T

Re: Small things

Posted by Greg Brown <gk...@mac.com>.

>> Serializable inner class
>> LinkedList.java:37 org.apache.pivot.collections.LinkedList$Node is
>> serializable and an inner class
>> Solution: make the inner class static.
>
>
> Yes, you can make Node a static inner class.

Please hold off on this change. It would just add unnecessary  
complexity to the LinkedList class. Node is a private inner class and  
will never be serialized independently of a LinkedList, so the fact  
that it implicitly serializes the outer class as well is not an issue.

Re: Small things

Posted by Todd Volkert <tv...@gmail.com>.

> Comparator doesn't implement Serializable
> org.apache.pivot.wtk.text.Element$NodeOffsetComparator implements
> Comparator but not Serializable
> org.apache.pivot.wtk.TableView$RowComparator implements Comparator but
> not Serializable
> org.apache.pivot.wtk.TreeView$PathComparator implements Comparator but
> not Serializable
> org.apache.pivot.tutorials.TreeNodeComparator implements Comparator
> but not Serializable
> org.apache.pivot.collections.List$SimpleComparator implements
> Comparator but not Serializable
> org.apache.pivot.io.Folder$FileNameComparator implements Comparator
> but not Serializable
> org.apache.pivot.io.FileList$FilePathComparator implements Comparator
> but not Serializable


Why is it recommended that Comparator implementations also implement
Serializable?  If that were mandatory, Comparator would be a sub-interface
of Serializable...


> Folder.java:224 Nullcheck of fileList at line 228 of value previously
> dereferenced
>        files = new ArrayList<File>(fileList.length);
>        listListeners.listCleared(this);
>
>        // Add the new files, firing an insert event for each file
>        if (fileList != null) {
> in this case, the warning tells that if (fileList != null) is
> unnecessary, because in case of null we get a NPE in the line
> containing fileList.length . So maybe a better check is to move over
> the null check and set a length value to zero in that case.


Hmm - I'm not sure if we should throw if fileList is null, because that
signals some error condition in listRoots() or listFiles().  Either that, or
we should set files to a new list of length zero.  Greg?


> Dead local store:
> TerraPanoramaSkin.java:383 Dead store to scrollLeft
> TerraPanoramaSkin.java:376 Dead store to scrollTop
> TerraActivityIndicatorSkin.java:87 Dead store to increment
> Drawing.java:59 Dead store to bounds


Hold off on these changes, as they should be investigated further.

Version.java:104 Equals method for org.apache.pivot.util.Version
> assumes the argument is of type Version


Can you add an instanceof check?


> Non-serializable value stored into instance field of a serializable class
> ApplicationContext.java:97
> org.apache.pivot.wtk.ApplicationContext$DisplayHost$1 stored into
> non-transient field ApplicationContext$DisplayHost.dropTargetListener
> ApplicationContext.java:957 org.apache.pivot.wtk.LocalManifest stored
> into non-transient field ApplicationContext$DisplayHost.dragManifest
> Solution: make the variable transient.


Ok, you can make those transient.


> Non-transient non-serializable instance field in serializable class
> ArrayQueue.java:33 Class org.apache.pivot.collections.ArrayQueue
> defines non-transient non-serializable instance field queueListeners
> LinkedStack.java:31 Class org.apache.pivot.collections.LinkedStack
> defines non-transient non-serializable instance field stackListeners
> ArrayStack.java:33 Class org.apache.pivot.collections.ArrayStack
> defines non-transient non-serializable instance field stackListeners
> LinkedQueue.java:31 Class org.apache.pivot.collections.LinkedQueue
> defines non-transient non-serializable instance field queueListeners
> Solution: make the variable transient.


Yes, you can make those fields transient.


> Serializable inner class
> LinkedList.java:37 org.apache.pivot.collections.LinkedList$Node is
> serializable and an inner class
> Solution: make the inner class static.


Yes, you can make Node a static inner class.


> Should be a static inner class
> MenuButton.java:36 Should
> org.apache.pivot.wtk.MenuButton$MenuButtonListenerList be a _static_
> inner class?
> Element.java:81 Should
> org.apache.pivot.wtk.text.Element$ElementListenerList be a _static_
> inner class?
> ChartView.java:327 Should
> org.apache.pivot.charts.ChartView$ChartViewSeriesListenerList be a
> _static_ inner class?
> Menu.java:41 Should org.apache.pivot.wtk.Menu$Item$ItemListenerList be
> a _static_ inner class?
> TerraCalendarSkin.java:343 Should
> org.apache.pivot.wtk.skin.terra.TerraCalendarSkin$DateButtonDataRenderer
> be a _static_ inner class?
> ChartView.java:297 Should
> org.apache.pivot.charts.ChartView$ChartViewCategoryListenerList be a
> _static_ inner class?
> ChartView.java:255 Should
> org.apache.pivot.charts.ChartView$ChartViewListenerList be a _static_
> inner class?
> MenuBar.java:40 Should
> org.apache.pivot.wtk.MenuBar$Item$ItemListenerList be a _static_ inner
> class?
> MenuBar.java:191 Should
> org.apache.pivot.wtk.MenuBar$MenuBarListenerList be a _static_ inner
> class?
> Canvas.java:36 Should
> org.apache.pivot.wtk.media.drawing.Canvas$CanvasListenerList be a
> _static_ inner class?
> MenuPopup.java:27 Should
> org.apache.pivot.wtk.MenuPopup$MenuPopupListenerList be a _static_
> inner class?
> TextNode.java:27 Should
> org.apache.pivot.wtk.text.TextNode$TextNodeListenerList be a _static_
> inner class?


Those can all be static inner classes - can you make this change?


> Unchecked/unconfirmed cast
> TerraPromptSkin.java:133 Unchecked/unconfirmed cast from
> org.apache.pivot.wtk.Window to org.apache.pivot.wtk.Prompt
> ShutdownTest.java:52 Unchecked/unconfirmed cast from
> org.apache.pivot.wtk.Dialog to org.apache.pivot.wtk.Alert
> TerraAlertSkin.java:135 Unchecked/unconfirmed cast from
> org.apache.pivot.wtk.Window to org.apache.pivot.wtk.Alert
> Solution: check with generics, or at least with instanceof .


These are false positives - they may be ignored.


> ApplicationContext.java:384 Method
> org.apache.pivot.wtk.ApplicationContext$DisplayHost.paint(Graphics)
> uses the nextDouble method of Random to generate a random integer;
> using nextInt is more efficient
> ApplicationContext.java:383 Method
> org.apache.pivot.wtk.ApplicationContext$DisplayHost.paint(Graphics)
> uses the nextDouble method of Random to generate a random integer;
> using nextInt is more efficient


Wow - I didn't even know of the existence of Random.nextInt(int).  Can you
make this change as well?

Thanks Sandro,
-T