Gentlemen,
I'm still fine with the patch.
Thanks,
Andrei
Roman Kennke wrote:
Hi Artem,
Cool! I'm comfortable with this fix now.
We still need second approve vote to push this change in.
Artem...? Others? ;)
Well, I'm fine with the proposed changes in general. Still, some JavaDoc
comments looks to be too implementation-specific. For example, most of
Container's methods contain a statement like this:
This is called from {...@link ...}, before/after ...
and they really called from the specified places, however this may not
be true in future. I'd remove all these statements from JavaDoc, leaving
only behavioral description.
Yeah, you are right. I put these in because I found it very useful while
implementing the peers to see exactly where and when it is called. I
removed those comments and only let the @see references in there. The
updated webrev is here:
http://kennke.org/~roman/docpeers/webrev/
Ok now?
Cheers, Roman
Thanks,
Artem
Thanks,
Andrei
Roman Kennke wrote:
Hi Andrei,
Finally I came around to fix the suggested stuff. See comments inline.
1) src/share/classes/java/awt/peer/CheckboxMenuItemPeer.java
+import java.awt.CheckboxMenuItem;
if that's really needed? I thought that {...@link ...} doesn't require
such stuff.
I don't know. If you fully qualify the stuff in the {...@link } it's not
needed. Should I fully-qualify things everywhere or let the import in
place? I don't care really.
2) src/share/classes/java/awt/peer/ContainerPeer.java
There is a typo in the second word:
- * Indicates availabiltity of restacking operation in this
container.
+ * Indicates availability of restacking operation in this
container.
Fixed.
3) Common thing.
<code>true</code> have a shorter synonym since JDK 5: {...@code true}
Fixed.
4) src/share/classes/java/awt/peer/RobotPeer.java
I noticed that you prefer not to leave "public" modifier in an
interface. But here you are leaving all of them.
Fixed.
5) src/share/classes/java/awt/peer/RobotPeer.java
public int getNumberOfButtons(); has left w/o any comments.
Fixed. I'm not sure if I got the sematics right.
6) src/share/classes/java/awt/peer/ComponentPeer.java
+ /**
+ * Called by {...@link EventQueue#coalescePaintEvent} to let the
component
+ * peer coalesce paint events.
+ *
+ * @param e the paint event to consider to coalesce
+ *
+ * @see EventQueue#coalescePaintEvent
+ */
+ void coalescePaintEvent(PaintEvent e);
I'd say it's not "to let .... coalesce paint events", but "to
coalesce paint events".
Fixed.
7) at the same class.
You wrote:
+ // TODO: Maybe change this to force Graphics2D, since many
things will
+ // break with plain Graphics nowadays.
+ Graphics getGraphics();
Do you know a scenario to show what's exactly might be broken. We
probably need to introduce another peer for that, right?
I already explained my reason in earlier mails. You think this is
reasonable?
8) Similar to 7):
+ // TODO: Maybe make that return a BufferedImage, because some
stuff will
+ // break if a different kind of image is returned.
+ Image createImage(int width, int height);
Dito.
This time I did the work on top of the -awt workspace and created a
webrev for your reviewing pleasure:
http://kennke.org/~roman/docpeers/webrev/
Thanks for having a look at it again,
Roman
