Hi Alexandr,
On 7/21/15 7:41 AM, Alexander Scherbatiy wrote:
Hello Jim,
Thank your for the review.
Could you review the updated fix there the suggested comments are
updated:
http://cr.openjdk.java.net/~alexsch/8029339/webrev.09/
Minor comments embedded below.
On 7/14/2015 2:36 AM, Jim Graham wrote:
AbstractMRI - getGraphics() should include "getGraphics() not
supported on Multi-Resolution Images" as the exception description text.
AbstractMRI - the getGraphics() description string contains an embedded
'\n' and an embedded '\"', are those typos?
BaseMRI - class comment - "The implementation will return the first
... satisfy the rendering request." Add another sentence right there
"The last image in the array will be returned if no suitable image is
found that is larger than the rendering request."
BaseMRI - whoops, my bad. "that is larger than" should probably be
"that is as large as" to match the <= comparison
SG2D.getResolutionVariant - using destRegionWH to calculate
destImageWH can involve a lot of "do some math and then later have
additional code undo it". Would it be faster to just compute
destImageWH directly, as in:
float destImageWidth, destImageHeight;
if (BASE) {
destImageWH = srcWH;
} else if (DPI) {
destImageWH = (float) abs(srcWH * devScale);
} else {
double destRegionWidth, destRegionHeight;
if (type) {
...
}
destImageWH = (float) abs(srcWH * destRegionWH / swh);
}
Image rv = img.getRV(destImageWH);
For the BASE and DPI_FIT cases shouldn't you use srcWidth,srcHeight
instead of sw,sh? The sw,sh are affected by sub-image parameters, but
srcWidth,srcHeight are literally the size of the original image, which
is what you want to search the MRI for. The sw,sh should only be used
to scale the srcWidth in the "else" clause - as in "srcWidth *
destRegionWidth / sw".
Is there intent to have the default case be mapped to DPI_FIT for
Windows?
I think yes.
I didn't see where this was being done - is that a TBD follow-on fix or
did I miss a default somewhere?
MRRHTest - why not render to a BufferedImage and avoid Robot? Could it
tap into internal APIs to create a BImg/compatibleImage with a given
devScale?
The DPI_FIT test includes case there a graphics transform is
identity but the device configuration transform has scale 2.
There should be a way to set scale for the device configuration
transform;
Ah yes, DPI_FIT needs a default transform. Also, without a way to
manually create a device with a transform, that means that DPI_FIT is
only successfully tested if the tests are run on a HiDPI machine, right?
MRRHTest - why allow up to delta=50 on the comparison?
It is just for monitors that are calibrated on Mac OS X and they
colors are different from that which was set. The test uses colors which
have at least one color component differ by 255.
Another issue that might go away if we could use BImg instead of robot.
MRRHTest - since the scale factor comes from a dialog, we depend on
running this on a HiDPI display to fully test the hints? Could using
"scaled compatible images" allow testing this more definitively?
Could you explain more what does it mean "scaled compatible images"?
I seem to recall that there is a mechanism in there to create backbuffer
images for swing that record the fact that they are scaled. I forget
how this is done, but I'm wondering if it could be used to run all of
this test code in a simulated scale environment rather than using the
actual configurations that AWT creates for the current system.
...jim
