Hi Jim,
On 2/14/13 6:26 AM, Jim Graham wrote:
I've been busy with FX things, but I just got a chance to look at some
of the new interfaces.
Ok, thanks =)
Here are some (hopefully) minor comments:
LightweightContent:
You never really define "image origin". The imageBufferReset() takes
an x,y, but it doesn't state what those are referring to. Is that the
x,y on the screen/scene where the image should be rendered to? Are
they the values to use to figure out what the starting offset in the
data array for the data for the image should be? One thing that would
help would be to include a formula in the method comments that
indicates how the data for pixels is retrieved from the buffer so
there is no confusion, something like:
-----
The {w} and {h} should match the width and height of the component
returned from {getComponent()} with the pixel at the origin of the
component, {(0, 0)} in the coordinate space of the component,
appearing at {data[y * linestride + x]}. All indices {data[(y + j) *
linestride + (x + i)]} where {0 <= i < w} and {0 <= j < h} will
represent valid pixel data for the component.
-----
Did I interpret that correctly?
Yes, the formula is correct. I've put it into the doc:
/**
* {@code JLightweightFrame} calls this method to notify the client
application that a new data buffer
* has been set as a content pixel buffer. Typically this occures
when a buffer of a larger size is
* created in response to a content resize event. The method
reports a reference to the pixel data buffer,
* the content image bounds within the buffer and the line stride
of the buffer. These values have the
* following correlation.
* <p>
* The {@code width} and {@code height} matches the size of the
content (the component returned from the
* {@link #getComponent} method). The {@code x} and {@code y} is
the origin of the content, {@code {0, 0}}
* in the coordinate space of the content, appearing at {@code
data[y * linestride + x]} in the buffer.
* All indices {@code data[(y + j) * linestride + (x + i)]} where
{@code 0 <= i < width} and {@code 0 <= j < height}
* will represent valid pixel data, {@code {i, j}} in the
coordinate space of the content.
*
* @param data the content pixel data buffer of INT_ARGB_PRE type
* @param x the x coordinate of the image
* @param y the y coordinate of the image
* @param width the width of the image
* @param height the height of the image
* @param linestride the line stride of the pixel buffer
*/
public void imageBufferReset(int[] data, int x, int y, int width,
int height, int linestride);
Is that enough clear now?
Then when you refer to xywh in imageReshaped I'm guessing it is just
supplying 4 new parameters to replace the identical parameters that
were in the Reset() method?
That's right. There're three distinct events: buffer recreation (always
or usually connected with new image bounds), image reshape (may not be
connected to the first event)
and image update (may not be connected to the first and second events).
So, I thought all the three events should be reflected separately.
Then in imageUpdated(), are the xywh relative to the coordinate system
of the Component? Or are they in the same space as the original xywh
were supplied to imageBufferReset? When you say they are "relative to
the origin" I think you mean the former. The thing that makes it
difficult to describe that is that you have the parameters to Reset
and Reshape both named x,y and the parameters to Updated are also
named x,y and one set of x,y parameters is relative to the other set
and you end up having to say "The x and y are relative to the x and
y". One of the sets of parameters should be renamed to make it easier
to discuss how they relate. Some sort of "All indices in the range
..." statement would help to show how all of the numbers relate to
each other.
OK, I put it into the formula:
/**
* {@code JLightweightFrame} calls this method to notify the client
application that a part of
* the content image, or the whole image has been updated. The
method reports bounds of the
* rectangular dirty region. The {@code dirtyX} and {@code dirtyY}
is the origin of the dirty
* rectangle, which is relative to the origin of the content,
appearing at
* {@code data[(y + dirtyY] * linestride + (x + dirtyX)] in the
pixel buffer
* (see {@link #imageBufferReset}). All indices {@code data[(y +
dirtyY + j] * linestride +
* (x + dirtyX + i)]} where {@code 0 <= i < dirtyWidth} and {@code
0 <= j < dirtyHeight}
* will represent valid pixel data, {@code {i, j}} in the
coordinate space of the dirty rectangle.
*
* @param dirtyX the x coordinate of the dirty rectangle, relative
to the image origin
* @param dirtyY the y coordinate of the dirty rectangle, relative
to the image origin
* @param dirtyWidth the width of the dirty rectangle
* @param dirtyHeight the height of the dirty rectangle
*
* @see #imageBufferReset
* @see #imageReshaped
*/
public void imageUpdated(int dirtyX, int dirtyY, int dirtyWidth,
int dirtyHeight);
In SwingNode:
Why is getContent() not just "return content;"?
Actually, I had some more complicated construction there (wrapped
content ref), in which I needed the local copy, then I reduced it, but
missed the fact that it's even simpler now.
Thanks for noticing, I'll fix it.
Have you discussed the threading issues with anyone in FX? There is a
big discussion right now on the appropriate threads for various
activities...
Not yet. I think we can start this discussion in the context of the
review of the fx part (which actually does the threading stuff).
Thanks,
Anton.
...jim
On 2/13/13 4:57 AM, Anton V. Tarasov wrote:
Hi Sergey,
new webrev: http://cr.openjdk.java.net/~ant/8006406/webrev.7
On 2/12/13 4:57 PM, Sergey Bylokhov wrote:
Hi, Anton.
Notes about implementation:
1 Seems some code was changed for debug simplifications or changes
from previous implementations. It would be good to revert them back.
(Ex /LWComponentPeer.bounds).
Fixed all such occurrences (replaced with public "get" methods where
available). Also, added "protected initializeBase(..)" method for
field
only initialization.
2 Probably it would be good to move grab/ungrab implementation from
LWToolkit/WToolkit to SunToolkit? It looks unclear why we need so
many
grab/ungrab/grabFocus/ungrabFocus methods with the same
implementation
in the different places.
We don't really need so much grabs and I will clean it when (and
if) we
publish the grab API. Please, see my replies to Anthony on this
subject.
3 I suggest make all methods in LightweightFrame final if possible.
Ok, I made toplevel related methods final. I'm not sure we should make
final all the rest... (and by the way, the extender JLF class is
final).
4 JLightweightFrame.rootPane could be final
Did.
5 JLightweightFrame.getGraphics() probably graphics should be
initialized by correct window fonts/background/foreground? Also when
you create backbuffer probably it should be filled by background
color? Note that if transparent images are supported you should be
aware about composite.
Ok, I did:
1) set transparent background for JLightweightFrame
2) set font/background/foreground for the Graphics.
Now I think I shouldn't specially care about the composite (am I
right?).
6 JLightweightFrame.initInterior you shouldn't dispose graphics.
Yes, it seems this adheres to the javadoc:
* Graphics objects which are provided as arguments to the
* <code>paint</code> and <code>update</code> methods
* of components are automatically released by the system when
* those methods return. For efficiency, programmers should
* call <code>dispose</code> when finished using
* a <code>Graphics</code> object only if it was created
* directly from a component or another <code>Graphics</code>
object.
Fixed it.
7 JLightweightFrame.reshape width * height could be changed to
width |
height ?
No =) It rather could be changed to w & h, but in order not to
confuse a
reader, I've changed it to w == 0 || h == 0.
8 JLightweightFrame.reshape you did not flush old backbuffer.
Did.
Also, I had to override LWWindowPeer.updateCursorImmediately() in
LWLFP
to workaround the deadlock I faced on Mac.
The deadlock has the following nature:
- EDT: holding the paintLock (a shared lock b/w JLF and SwingNode),
and
the cursor manager dives to native code and tries to invoke a
method on
Main (FX App) thread.
- FX Renderer: is about to render a SwingNode content, waiting on the
paintLock.
- FX App: (as far as I can guess) waiting for the Renderer to finish.
I can start looking for the solution in parallel with the review,
and if
not yet found, I'd push the first patch with cursor updates disabled.
Thanks!
Anton.
08.02.2013 21:27, Anton V. Tarasov wrote:
Hi All,
Please, review the changes for the CR:
http://bugs.sun.com/view_bug.do?bug_id=8006406
webrev: http://cr.openjdk.java.net/~ant/8006406/webrev.6
It introduces sun.swing.JLightweightFrame class, aimed at
lightweight
embedding of Swing components into java-based toolkits.
The primary target is JavaFX toolkit, however the class is not
limited to this usage and the API it provides is quite generic.
Below I'm giving a link to the jfx side implementation. This
implementation should not be reviewed in this thread (it is in a
pre-review phase),
it should just clarify how the introduced API is supposed to be
used.
Namely, SwingNode.SwingNodeContent which implements
sun.swing.LightweightContent and forwards requests from
sun.swing.JLightweightFrame to NGExternalNode which does the
rendering.
webrev: http://cr.openjdk.java.net/~ant/RT-27887/webrev.1
Some comments on the awt/swing part:
- Only Win and Mac implementation is currently available, X11 will
come lately.
- Win implementation uses a heavyweight window behind the
lightweight
frame, while it is not actually needed for lightweight embedding.
This is due to the architecture of the Win AWT peers which are
strongly tight to the native code, and it's not a trivial task to
separate them.
On Mac the lightweight frame peer is truly lightweight, meaning
that it doesn't create an NSWindow object behind it. The Mac port LW
abstraction
allows to override and substitute CPlatform* classes with their
lightweight stubs.
- LightweightFrame, among others, introduces two new methods -
grabFocus() and ungrabFocus(boolean). Ideally, these methods should
go to
the super Window class where the grab API becomes public (which is
a long-term project...). Current host of the grab API is SunToolkit,
which
now forwards the calls to LightweightFrame. This is necessary to
intercommunicate with the client when grab/ungrab happens on both
sides.
- Unresolved issues exist, like modal dialogs, d&d etc. They are to
be addressed further.
Thanks,
Anton.
