[OpenJDK 2D-Dev] [PATCH] JDK-8146035: Windows - With LCD antialiasing, some glyphs are not rendered correctly
Hello, I'd like to propose a fix for JDK-8146035. I am not a committer, so I hope someone can sponsor this fix. I work at Jetbrains, which has signed a company-level contributor agreement, so, from a legal perspective, I believe, there are no obstacles. My investigation shows that the issue is caused by incorrect determination of bitmap size, prepared for glyph rendering, so only part of glyph becomes visible due to cropping. This seems to happen because compatible bitmap is not selected into memory device context (DC) before calling GetTextMetrics. Documentation for CreateCompatibleDC call ( https://msdn.microsoft.com/en-us/library/windows/desktop/dd183489%28v=vs.85%29.aspx) says compatible bitmap needs to be selected into DC before any drawing operation. Even though GetTextMetrics is not a drawing operation, it turns out to be affected by selected bitmap's type too (by default a monochrome bitmap is selected in a memory DC). This behaviour was also mentioned in the following MSDN blog post comment: https://blogs.msdn.microsoft.com/oldnewthing/20060614-00/?p=30873#comment-392143 The proposed fix is to select a temporary 1x1 compatible bitmap into memory DC before GetTextMetrics call. Here's webrev link - http://adm-12504.intellij.net/ I didn't create a test case, as it would require a specific font file (I couldn't reproduce the issue for fonts bundled with Windows). Best regards, Dmitry Batrak
[OpenJDK 2D-Dev] JDK-8153732 Change Windows refresh logic to also catch remote printer changes
Hi all, I have various customers that do use remote print servers from the application servers and therefore do not see remote printers added without restarting the JVM of the application server using the current implementation. Because I do not want to tweak with reflection on the Windows PrintServiceLookup implementation in the long run, I would like to help implementing a alternate solution that also handles remote printers as well. I would like to discuss a possible solution for this problem using a the RegistryKeyChangeEvent on printer registry keys instead of the FindFirstPrinterChangeNotification call as described in the following blog: https://blogs.msdn.microsoft.com/hmahrt/2012/04/09/how-to-listen-for-printer-connections/ Cheers Patrick
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jim,
Thanks for your valuable inputs.
Please find the updates inline. I have created new webrev
http://cr.openjdk.java.net/~jdv/7107905/webrev.01/ for review.
Are there any bugs in the database about the fact that many of these ColorModel
subclasses implement equals without hashcode? They really should both be
implemented, but since ColorModel implements the method based on its tests,
they are technically covered by the equals/hashCode contract - it's just not a
very optimal implementation of the contract.
- Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has mentioned
the same thing. Can I create a subtask to address java.awt.image changes?
For now, it would be good to implement hashCode() on ICM now that you are
creating an equals() method for it.
- I am not completely sure of what is the optimal way of adding
hashCode() API so I took help from internet and IDE to make the changes. I am
including super.hashCode() as it adds uniqueness to ICM.
Also, ColorModel.hashCode() is a poor implementation. It doesn't use the
paradigms recommended by Effective Java and looks like it produces poor hashes
as a result (just in the first two elements of the hashCode I see a collision
due to poor choice of numbers).
- I think since we are not using Prime numbers and it might result in
improper hash code. I have made similar changes to hashCode() of ColorModel.
With respect to this particular equals() override I think it looks fine, though
the use of the isValid() method reduces its performance for a couple of reasons:
- it retests the index for the range [0,map size] which we already know is valid
- if the validBits is null, there is no need to even do the test.
This might be faster for the very common case:
if (validBits == null) {
if (cm.validBits != null) return false;
// loop just comparing rgb[] values
} else {
if (cm.validBits == null) return false;
// loop, testing rgb[] values and
// corresponding validBits indices directly
}
Note that in the constructor we set validBits to null if it is "true"
for all valid indices so if one of the cmaps has it null and the other does
not, then that is a very good indicator that their valid indices don't match.
- I have updated the suggested changes.
On a more minor note, I don't like the indentation of the if statement, I'd
move the "{" brace to a line of its own indented the same as its closing "}" to
make the body of the if stand out. It isn't strictly the Java coding
guidelines, but it does match a bunch of the other 2D code - sort of a local
exception to the coding style.
- In the same file itself we are following Java coding guidelines of
having braces after if like "if () {". I am not completely sure of including
"{" in new line.
I'd also add a few test cases that test that 2 separately and identically
constructed ICM instances are equals() == true, tested with one of each of the
constructors...
- I have made changes to test case for verifying all constructors with
same ICM. Also added verification for hashCode value.
...jim
On 4/6/2016 4:47 AM, Jayathirth D V wrote:
> Hi,
>
> _Please review the following fix in JDK9:_
>
> Bug : https://bugs.openjdk.java.net/browse/JDK-7107905
>
> Webrev : http://cr.openjdk.java.net/~jdv/7107905/webrev.00/
>
> Issue : When we compare two IndexColorModel objects with different
> ColorMap(or any other property specific to IndexColorModel) using
> equals() method it returns true.
>
> Root cause : There is no override equals() method specific to
> IndexColorModel and it uses super class equals() which is ColorModel
> and it doesn't verify equality for IndexColorModel specific properties.
>
> Solution : Add override equals() method for IndexColorModel which
> verifies its unique properties for equality.
>
> Thanks,
>
> Jay
>
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Hi Jayathirth,
This looks good.
One thing to note for efficiency, "instanceof" also checks for null. It
only returns true for non-null objects, so you don't need to explicitly
test for null at the top of ICM.equals(). (Though, you should include a
test(s) in the unit test that verifies that no ICM returns true for
equals(null) to be sure.) You can see that CM.equals() is implemented
this way.
Also, for performance, ICM should include a quick "if (this == cm)
return true;" check, like CM.equals(). I'd recommend:
- first instanceof
- then == test
- then super.equals()
- finally, test equality of data fields
More comments inline:
On 4/7/16 6:46 AM, Jayathirth D V wrote:
- Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has mentioned
the same thing. Can I create a subtask to address java.awt.image changes?
That would be good.
For now, it would be good to implement hashCode() on ICM now that you are
creating an equals() method for it.
- I am not completely sure of what is the optimal way of adding
hashCode() API so I took help from internet and IDE to make the changes. I am
including super.hashCode() as it adds uniqueness to ICM.
You did a great job. To save time in the future, you should all have
copies of the latest version of "Effective Java" by Josh Bloch. It has
a whole chapter on equals/hashCode. It's a very handy reference for all
sorts of good programming practice for the Java language.
Also, ColorModel.hashCode() is a poor implementation. It doesn't use the
paradigms recommended by Effective Java and looks like it produces poor hashes
as a result (just in the first two elements of the hashCode I see a collision
due to poor choice of numbers).
- I think since we are not using Prime numbers and it might result in
improper hash code. I have made similar changes to hashCode() of ColorModel.
Looks great.
- In the same file itself we are following Java coding guidelines of having braces after if
like "if () {". I am not completely sure of including "{" in new line.
You are correct, that matching local code is a good consideration when
considering code style. In this case, though, the indentation on these
if statements is an example of what we're trying to avoid so it would be
better to fix these particular statements (don't bother fixing the other
lines in the file at this point, that can wait until we have to update
other parts of the file, but don't propagate a bad style in new code).
In particular:
Never do this:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Quick question - where does the body of the if statement start? Does
your eye see it in a fraction of a second or do you have to search for it?
That is the worst option for indenting an if statement with continuation
lines. Never do that in new code. Do either of the following two versions:
Java Code Style guidelines recommends indenting 8 spaces for conditional
continuations:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Jim's personal extension to the JCS recommends (and the 2D team
historically tended to agree):
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test)
{
the body of the code;
more body of the code;
}
Both of those immediately draw the eye to the separating point between
the conditional and the body of the code.
I'd also add a few test cases that test that 2 separately and identically
constructed ICM instances are equals() == true, tested with one of each of the
constructors...
- I have made changes to test case for verifying all constructors with
same ICM. Also added verification for hashCode value.
Unfortunately, some of your tests for hashCode make an invalid
assumption. It is technically correct for the hash codes of 2 different
objects to be the same regardless of whether they are equals() or not.
In fact, a perfectly valid implementation of hashCode() could return a
constant number and it would be correct from the perspective of the
equals/hashCode contract. (Such code, however, would not be optimal,
but it would be valid/correct-to-spec.) The only condition that is
required is that the hash codes match if the objects are equals(), but
they are allowed to match if the objects are !equals(). In other words:
boolean equals1 = (o1.equals(o2));
boolean equals2 = (o2.equals(o1));
boolean equalsH = (o1.hashCode() == o2.hashCode());
if (equals1 !=
Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: equals() method in IndexColorModel doesnt exist and it relies on ColorModel.equals() which is not strict
Small note that we should not forget to create a ccc.
On 07.04.16 21:58, Jim Graham wrote:
Hi Jayathirth,
This looks good.
One thing to note for efficiency, "instanceof" also checks for null. It
only returns true for non-null objects, so you don't need to explicitly
test for null at the top of ICM.equals(). (Though, you should include a
test(s) in the unit test that verifies that no ICM returns true for
equals(null) to be sure.) You can see that CM.equals() is implemented
this way.
Also, for performance, ICM should include a quick "if (this == cm)
return true;" check, like CM.equals(). I'd recommend:
- first instanceof
- then == test
- then super.equals()
- finally, test equality of data fields
More comments inline:
On 4/7/16 6:46 AM, Jayathirth D V wrote:
- Yes https://bugs.openjdk.java.net/browse/JDK-6588409 has
mentioned the same thing. Can I create a subtask to address
java.awt.image changes?
That would be good.
For now, it would be good to implement hashCode() on ICM now that you
are creating an equals() method for it.
- I am not completely sure of what is the optimal way of adding
hashCode() API so I took help from internet and IDE to make the
changes. I am including super.hashCode() as it adds uniqueness to ICM.
You did a great job. To save time in the future, you should all have
copies of the latest version of "Effective Java" by Josh Bloch. It has
a whole chapter on equals/hashCode. It's a very handy reference for all
sorts of good programming practice for the Java language.
Also, ColorModel.hashCode() is a poor implementation. It doesn't use
the paradigms recommended by Effective Java and looks like it produces
poor hashes as a result (just in the first two elements of the
hashCode I see a collision due to poor choice of numbers).
- I think since we are not using Prime numbers and it might result
in improper hash code. I have made similar changes to hashCode() of
ColorModel.
Looks great.
- In the same file itself we are following Java coding guidelines
of having braces after if like "if () {". I am not completely sure of
including "{" in new line.
You are correct, that matching local code is a good consideration when
considering code style. In this case, though, the indentation on these
if statements is an example of what we're trying to avoid so it would be
better to fix these particular statements (don't bother fixing the other
lines in the file at this point, that can wait until we have to update
other parts of the file, but don't propagate a bad style in new code).
In particular:
Never do this:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Quick question - where does the body of the if statement start? Does
your eye see it in a fraction of a second or do you have to search for it?
That is the worst option for indenting an if statement with continuation
lines. Never do that in new code. Do either of the following two
versions:
Java Code Style guidelines recommends indenting 8 spaces for conditional
continuations:
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test) {
the body of the code;
more body of the code;
}
Jim's personal extension to the JCS recommends (and the 2D team
historically tended to agree):
if (some complex test ||
some additional tests ||
some additional tests ||
some additional tests ||
some continuation of the test)
{
the body of the code;
more body of the code;
}
Both of those immediately draw the eye to the separating point between
the conditional and the body of the code.
I'd also add a few test cases that test that 2 separately and
identically constructed ICM instances are equals() == true, tested
with one of each of the constructors...
- I have made changes to test case for verifying all constructors
with same ICM. Also added verification for hashCode value.
Unfortunately, some of your tests for hashCode make an invalid
assumption. It is technically correct for the hash codes of 2 different
objects to be the same regardless of whether they are equals() or not.
In fact, a perfectly valid implementation of hashCode() could return a
constant number and it would be correct from the perspective of the
equals/hashCode contract. (Such code, however, would not be optimal,
but it would be valid/correct-to-spec.) The only condition that is
required is that the hash codes match if the objects are equals(), but
they are allowed to match if the objects are !equals(). In other words:
boolean equals1 = (o1.equals(o2));
boolean equals2 = (o2.equals(o1));
boolean equalsH = (o1.hashCo
