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/21 10:56:04 UTC
Some small things to fix
Hi to all,
I'm just returned, et voilà some other small things to fix :-) ...
ApplicationContext.java:381 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:382 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
=> I'm already doing the patch, I'll post here the code for this
before commit ...
DesktopApplicationContext.java:270 Incorrect lazy initialization and
update of static field
org.apache.pivot.wtk.DesktopApplicationContext.applicationContext
Description:
This method contains an unsynchronized lazy initialization of a static
field. After the field is set, the object stored into that location is
further accessed. The setting of the field is visible to other threads
as soon as it is set. If the further accesses in the method that set
the field serve to initialize the object, then you have a very serious
multithreading bug, unless something else prevents any other thread
from accessing the stored object until it is fully initialized.
=> this is related to the applicationContext variable ... should we
synchronize its access ?
DesktopApplicationContext.java:343
org.apache.pivot.wtk.DesktopApplicationContext.main(String[]) might
ignore java.lang.Exception
=> should we write a message in System.err in this case, like "Web
Start not available", and "User Home not available" ?
FilteredList.java:475 Nullcheck of FilteredList.view at line 480 of
value previously dereferenced
=> view is already used in the line 475, so a check of view != null is
not necessary, or should be done before line 475 ...
MapListTest.java:146 Should
org.apache.pivot.collections.test.MapListTest$TestMapListListener be a
_static_ inner class?
MapListTest.java:160 Should
org.apache.pivot.collections.test.MapListTest$TestComparator be a
_static_ inner class?
HashSet.java:106 Call to unsupported method
org.apache.pivot.collections.HashMap.setComparator(Comparator)
HashMapTest.java:133 Call to unsupported method
org.apache.pivot.collections.HashMap.setComparator(Comparator)
HashMapTest.java:125 Call to unsupported method new
org.apache.pivot.collections.HashMap(Comparator)
=> is it wanted (partial implementation at the moment), or a mistake ?
ArrayList.java:363 org.apache.pivot.wtk.TreeView$BranchHandler doesn't
override org.apache.pivot.collections.ArrayList.equals(Object)
=> Todd should tell me something on this ...
And last, two bugs from the High Priority, we have already discussed
some time ago:
TableViewDateCellRenderer.java:35 Found static field of type
java.text.DateFormat
=> we could change this to protected or private
ApplicationContext.java:1353
org.apache.pivot.wtk.ApplicationContext.resourceCache is or uses a map
or set of URLs, which can be a performance hog
=> this have to be explored ... but instead of using URL as Map keys,
what do you think on use URI, or at least a String representation of
the currently used URL ? This change require small modifications in
many classes under wtk, this is why I'd like to have this in a major
release as 1.3, and not in a bug fix release like the 1.3.1
Tell me what i have to do ...
Bye,
Sandro
Re: Some small things to fix
Posted by Sandro Martini <sa...@gmail.com>.
Ok, thanks for the hints ...
Sandro
Re: Some small things to fix
Posted by Greg Brown <gk...@mac.com>.
Agreed. For major changes to core features, RTC may be preferable, but
for small fixes or new features, CTR is fine.
On Aug 21, 2009, at 11:07 AM, Niclas Hedhman wrote:
> Most projects at ASF uses "Commit-Then-Review" (CTR), and even
> projects with
> strict "Review-Then-Commit" (RTC) normally employ CTR for non-core
> parts,
> such as demos, tests, docs, extensions et cetera.
>
> CTR typically allows for great efficiency, and although it can feel
> intimidating at first to commit directly at first, it is really No
> Big Deal.
> Everyone makes mistakes, and that is Ok, so I urge you all to boost
> your
> self confidence and just go ahead. Each commit will be received as a
> mail on
> pivot-commits@i.a.o mailing list and we are all expected to review
> those
> commits, at least to some degree to ensure noone introduce malicious
> code
> (very unlikely, but still). So, if you go down the wrong path,
> others will
> ask questions and perhaps correct mistakes...
>
> Another common recited mantra at ASF; Don't ask for permission,
> instead ask
> for forgiveness if needed. Some also call it Just Forking Do It.
>
> -- Niclas
>
> On Aug 21, 2009 10:46 PM, "Sandro Martini"
> <sa...@gmail.com> wrote:
>
> Hi,
> this is the patch for the inner classes (trivial):
>
> Index: test/org/apache/pivot/collections/test/MapListTest.java
> ===================================================================
> --- test/org/apache/pivot/collections/test/MapListTest.java
> (revision
> 806566)
> +++ test/org/apache/pivot/collections/test/MapListTest.java
> (working
> copy)
> @@ -143,7 +143,7 @@
> assertEquals(0, mapList.getLength());
> }
>
> - private class TestMapListListener implements
> MapListListener<String, Integer> {
> + private static class TestMapListListener implements
> MapListListener<String, Integer> {
> private MapList<String, Integer> mapList;
> private Map<String, Integer> previousSource;
> private int calls;
> @@ -157,7 +157,7 @@
> }
> }
>
> - private class TestComparator implements Comparator<Pair<String,
> Integer>> {
> + private static class TestComparator implements
> Comparator<Pair<String, Integer>> {
> @Override
> public int compare(Pair<String, Integer> o1, Pair<String,
> Integer> o2) {
> return o1.key.compareTo(o2.key);
>
>
>
> And this is that for the random:
>
> Index: src/org/apache/pivot/wtk/ApplicationContext.java
> ===================================================================
> --- src/org/apache/pivot/wtk/ApplicationContext.java (revision
> 806566)
> +++ src/org/apache/pivot/wtk/ApplicationContext.java (working copy)
> @@ -48,6 +48,7 @@
> import java.net.URISyntaxException;
> import java.net.URL;
> import java.util.Iterator;
> +import java.util.Random;
> import java.util.Timer;
> import java.util.TimerTask;
>
> @@ -89,7 +90,10 @@
> private boolean paintPending = false;
>
> private boolean debugRepaint = false;
> +
> + private Random random = null;
>
> +
> private transient DropTargetListener dropTargetListener = new
> DropTargetListener() {
> public void dragEnter(DropTargetDragEvent event) {
> if (dragDescendant != null) {
> @@ -267,6 +271,9 @@
>
> try {
> debugRepaint =
> Boolean
> .parseBoolean
> (System.getProperty("org.apache.pivot.wtk.debugRepaint"));
> + if (debugRepaint == true)
> + random = new Random();
> +
> } catch (SecurityException ex) {
> // No-op
> }
> @@ -378,8 +385,8 @@
> }
>
> if (debugRepaint) {
> - graphics.setColor(new
> java.awt.Color((int)(Math.random() * 256),
> - (int)(Math.random() * 256),
> (int)(Math.random() * 256), 75));
> + graphics.setColor(new
> java.awt.Color(random.nextInt(256),
> + random.nextInt(256),
> random.nextInt(256), 75));
> graphics.fillRect(0, 0, getWidth(),
> getHeight());
> }
> } catch (RuntimeException exception) {
>
>
>
> If Ok i can commit, tell me.
>
> Bye
Re: Some small things to fix
Posted by Niclas Hedhman <ni...@hedhman.org>.
Most projects at ASF uses "Commit-Then-Review" (CTR), and even projects with
strict "Review-Then-Commit" (RTC) normally employ CTR for non-core parts,
such as demos, tests, docs, extensions et cetera.
CTR typically allows for great efficiency, and although it can feel
intimidating at first to commit directly at first, it is really No Big Deal.
Everyone makes mistakes, and that is Ok, so I urge you all to boost your
self confidence and just go ahead. Each commit will be received as a mail on
pivot-commits@i.a.o mailing list and we are all expected to review those
commits, at least to some degree to ensure noone introduce malicious code
(very unlikely, but still). So, if you go down the wrong path, others will
ask questions and perhaps correct mistakes...
Another common recited mantra at ASF; Don't ask for permission, instead ask
for forgiveness if needed. Some also call it Just Forking Do It.
-- Niclas
On Aug 21, 2009 10:46 PM, "Sandro Martini" <sa...@gmail.com> wrote:
Hi,
this is the patch for the inner classes (trivial):
Index: test/org/apache/pivot/collections/test/MapListTest.java
===================================================================
--- test/org/apache/pivot/collections/test/MapListTest.java (revision
806566)
+++ test/org/apache/pivot/collections/test/MapListTest.java (working
copy)
@@ -143,7 +143,7 @@
assertEquals(0, mapList.getLength());
}
- private class TestMapListListener implements
MapListListener<String, Integer> {
+ private static class TestMapListListener implements
MapListListener<String, Integer> {
private MapList<String, Integer> mapList;
private Map<String, Integer> previousSource;
private int calls;
@@ -157,7 +157,7 @@
}
}
- private class TestComparator implements Comparator<Pair<String,
Integer>> {
+ private static class TestComparator implements
Comparator<Pair<String, Integer>> {
@Override
public int compare(Pair<String, Integer> o1, Pair<String,
Integer> o2) {
return o1.key.compareTo(o2.key);
And this is that for the random:
Index: src/org/apache/pivot/wtk/ApplicationContext.java
===================================================================
--- src/org/apache/pivot/wtk/ApplicationContext.java (revision 806566)
+++ src/org/apache/pivot/wtk/ApplicationContext.java (working copy)
@@ -48,6 +48,7 @@
import java.net.URISyntaxException;
import java.net.URL;
import java.util.Iterator;
+import java.util.Random;
import java.util.Timer;
import java.util.TimerTask;
@@ -89,7 +90,10 @@
private boolean paintPending = false;
private boolean debugRepaint = false;
+
+ private Random random = null;
+
private transient DropTargetListener dropTargetListener = new
DropTargetListener() {
public void dragEnter(DropTargetDragEvent event) {
if (dragDescendant != null) {
@@ -267,6 +271,9 @@
try {
debugRepaint =
Boolean.parseBoolean(System.getProperty("org.apache.pivot.wtk.debugRepaint"));
+ if (debugRepaint == true)
+ random = new Random();
+
} catch (SecurityException ex) {
// No-op
}
@@ -378,8 +385,8 @@
}
if (debugRepaint) {
- graphics.setColor(new
java.awt.Color((int)(Math.random() * 256),
- (int)(Math.random() * 256),
(int)(Math.random() * 256), 75));
+ graphics.setColor(new
java.awt.Color(random.nextInt(256),
+ random.nextInt(256), random.nextInt(256), 75));
graphics.fillRect(0, 0, getWidth(), getHeight());
}
} catch (RuntimeException exception) {
If Ok i can commit, tell me.
Bye
Re: Some small things to fix
Posted by Sandro Martini <sa...@gmail.com>.
Hi,
this is the patch for the inner classes (trivial):
Index: test/org/apache/pivot/collections/test/MapListTest.java
===================================================================
--- test/org/apache/pivot/collections/test/MapListTest.java (revision 806566)
+++ test/org/apache/pivot/collections/test/MapListTest.java (working copy)
@@ -143,7 +143,7 @@
assertEquals(0, mapList.getLength());
}
- private class TestMapListListener implements
MapListListener<String, Integer> {
+ private static class TestMapListListener implements
MapListListener<String, Integer> {
private MapList<String, Integer> mapList;
private Map<String, Integer> previousSource;
private int calls;
@@ -157,7 +157,7 @@
}
}
- private class TestComparator implements Comparator<Pair<String, Integer>> {
+ private static class TestComparator implements
Comparator<Pair<String, Integer>> {
@Override
public int compare(Pair<String, Integer> o1, Pair<String,
Integer> o2) {
return o1.key.compareTo(o2.key);
And this is that for the random:
Index: src/org/apache/pivot/wtk/ApplicationContext.java
===================================================================
--- src/org/apache/pivot/wtk/ApplicationContext.java (revision 806566)
+++ src/org/apache/pivot/wtk/ApplicationContext.java (working copy)
@@ -48,6 +48,7 @@
import java.net.URISyntaxException;
import java.net.URL;
import java.util.Iterator;
+import java.util.Random;
import java.util.Timer;
import java.util.TimerTask;
@@ -89,7 +90,10 @@
private boolean paintPending = false;
private boolean debugRepaint = false;
+
+ private Random random = null;
+
private transient DropTargetListener dropTargetListener = new
DropTargetListener() {
public void dragEnter(DropTargetDragEvent event) {
if (dragDescendant != null) {
@@ -267,6 +271,9 @@
try {
debugRepaint =
Boolean.parseBoolean(System.getProperty("org.apache.pivot.wtk.debugRepaint"));
+ if (debugRepaint == true)
+ random = new Random();
+
} catch (SecurityException ex) {
// No-op
}
@@ -378,8 +385,8 @@
}
if (debugRepaint) {
- graphics.setColor(new
java.awt.Color((int)(Math.random() * 256),
- (int)(Math.random() * 256),
(int)(Math.random() * 256), 75));
+ graphics.setColor(new
java.awt.Color(random.nextInt(256),
+ random.nextInt(256), random.nextInt(256), 75));
graphics.fillRect(0, 0, getWidth(), getHeight());
}
} catch (RuntimeException exception) {
If Ok i can commit, tell me.
Bye
Re: Some small things to fix
Posted by Sandro Martini <sa...@gmail.com>.
Hi Greg,
thanks for the fixes ... I'll post mine later (if I'm able today) ...
Bye
Re: Some small things to fix
Posted by Greg Brown <gk...@mac.com>.
> ApplicationContext.java:381 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
> => I'm already doing the patch, I'll post here the code for this
> before commit ...
OK.
> DesktopApplicationContext.java:270 Incorrect lazy initialization and
> update of static field
> org.apache.pivot.wtk.DesktopApplicationContext.applicationContext
>
> => this is related to the applicationContext variable ... should we
> synchronize its access ?
No, this value will only be accessed from a single thread.
> DesktopApplicationContext.java:343
> org.apache.pivot.wtk.DesktopApplicationContext.main(String[]) might
> ignore java.lang.Exception
> => should we write a message in System.err in this case, like "Web
> Start not available", and "User Home not available" ?
No - getOrigin() is defined to return null when a suitable origin
cannot be found. I added Javadoc to clarify.
> FilteredList.java:475 Nullcheck of FilteredList.view at line 480 of
> value previously dereferenced
> => view is already used in the line 475, so a check of view != null is
> not necessary, or should be done before line 475 ...
Ah, good one. Fixed.
> HashSet.java:106 Call to unsupported method
> org.apache.pivot.collections.HashMap.setComparator(Comparator)
> HashMapTest.java:133 Call to unsupported method
> org.apache.pivot.collections.HashMap.setComparator(Comparator)
>
> HashMapTest.java:125 Call to unsupported method new
> org.apache.pivot.collections.HashMap(Comparator)
> => is it wanted (partial implementation at the moment), or a mistake ?
This is correct - HashMap#setComparator() will be supported soon.
> And last, two bugs from the High Priority, we have already discussed
> some time ago:
>
> TableViewDateCellRenderer.java:35 Found static field of type
> java.text.DateFormat
> => we could change this to protected or private
I'm guessing this is high priority because DateFormats are mutable?
I'll make it protected.
> ApplicationContext.java:1353
> org.apache.pivot.wtk.ApplicationContext.resourceCache is or uses a map
> or set of URLs, which can be a performance hog
> => this have to be explored ... but instead of using URL as Map keys,
> what do you think on use URI, or at least a String representation of
> the currently used URL ? This change require small modifications in
> many classes under wtk, this is why I'd like to have this in a major
> release as 1.3, and not in a bug fix release like the 1.3.1
We haven't seen any performance issues related to this yet, so I'm
inclined to continue to defer this until 1.3.1 or later.