Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v2]
On Fri, 2 Apr 2021 00:12:58 GMT, Sergey Bylokhov wrote:
>> I do not suggest that the change should be moved forth and back, but I think
>> that the second conditions should always be shifted, and if this causes 80
>> chars overflow then some other line split/rename/etc should be done to
>> prevent that.
>>
>> Recent example of such style...:
>> if ((getColorSpaceType (p) == ColorSpace.TYPE_RGB) &&
>> (getData (p, icSigMediaWhitePointTag) != null) &&
>> (getData (p, icSigRedColorantTag) != null) &&
>> (getData (p, icSigGreenColorantTag) != null) &&
>> (getData (p, icSigBlueColorantTag) != null) &&
>> (getData (p, icSigRedTRCTag) != null) &&
>> (getData (p, icSigGreenTRCTag) != null) &&
>> (getData (p, icSigBlueTRCTag) != null)) {
>> thisProfile = new ICC_ProfileRGB (p);
>>
>> Is that really looks fine?
>
>> To make it clear, you're for keeping the indentation as it was in the
>> original PR to visually separate condition from the statement in the if
>> block. Do I get it right, @mrserb?
>
> I think it looks better.
BTW another solution is to move { to the next line if the previous statement
was split across few lines.
-
PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v2]
On Thu, 1 Apr 2021 17:29:08 GMT, Sergey Bylokhov wrote:
>>> I would prefer if you revert this line or if we want to put emphasis on
>>> readability moving '{' to new line also seems fine.
>>
>> Done.
>
> I do not suggest that the change should be moved forth and back, but I think
> that the second conditions should always be shifted, and if this causes 80
> chars overflow then some other line split/rename/etc should be done to
> prevent that.
>
> Recent example of such style...:
> if ((getColorSpaceType (p) == ColorSpace.TYPE_RGB) &&
> (getData (p, icSigMediaWhitePointTag) != null) &&
> (getData (p, icSigRedColorantTag) != null) &&
> (getData (p, icSigGreenColorantTag) != null) &&
> (getData (p, icSigBlueColorantTag) != null) &&
> (getData (p, icSigRedTRCTag) != null) &&
> (getData (p, icSigGreenTRCTag) != null) &&
> (getData (p, icSigBlueTRCTag) != null)) {
> thisProfile = new ICC_ProfileRGB (p);
>
> Is that really looks fine?
> To make it clear, you're for keeping the indentation as it was in the
> original PR to visually separate condition from the statement in the if
> block. Do I get it right, @mrserb?
I think it looks better.
-
PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8255800: Raster creation methods need some specification clean up [v2]
On Thu, 1 Apr 2021 00:56:41 GMT, Sergey Bylokhov wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8255800: Raster creation methods need some specification clean up > > Marked as reviewed by serb (Reviewer). I've updated the PR with one very minor typo correction and a removal of a duplicate throws clause. Also I've prepared a draft CSR which needs to be reviewed https://bugs.openjdk.java.net/browse/JDK-8264625 - PR: https://git.openjdk.java.net/jdk/pull/3223
Re: [OpenJDK 2D-Dev] RFR: 8255800: Raster creation methods need some specification clean up [v2]
> https://bugs.openjdk.java.net/browse/JDK-8255800 could have been a one line > spec clean up but > it didn't take a lot of looking to realize there were many more > inconsistencies between spec and implementation. > I've spent a lot of time on what is just small number of factory methods in > Raster because there are so > many possible exceptions and in some cases they rely on other API they call > to generate exceptions and > these may have not been documented or documented acc. to some long lost > behavior. > I've mostly tried to ONLY change spec. But I couldn't help myself when some > checks were missed that > ended up with bizarre and dubious behavior - throwing > NegativeArrayIndexException which just about > always has to be an internal bug ! > > The supplied test passes on JDK 16 as well as this code, because the > (relatively) small number of > cases where JDK 16 threw NegativeArrayIndexException are caught and allowed > only for releases < 17 > So where you see those in the test it corresponds to the behavioral changes > from NegativeArrayIndexException > to IllegalArgumentException. > JCK conformance tests still pass so they must not test those conditions. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8255800: Raster creation methods need some specification clean up - Changes: - all: https://git.openjdk.java.net/jdk/pull/3223/files - new: https://git.openjdk.java.net/jdk/pull/3223/files/07424a32..85148fd5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3223=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3223=00-01 Stats: 5 lines in 2 files changed: 1 ins; 3 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3223.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3223/head:pull/3223 PR: https://git.openjdk.java.net/jdk/pull/3223
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v2]
On Thu, 1 Apr 2021 17:29:08 GMT, Sergey Bylokhov wrote:
>>> I would prefer if you revert this line or if we want to put emphasis on
>>> readability moving '{' to new line also seems fine.
>>
>> Done.
>
> I do not suggest that the change should be moved forth and back, but I think
> that the second conditions should always be shifted, and if this causes 80
> chars overflow then some other line split/rename/etc should be done to
> prevent that.
>
> Recent example of such style...:
> if ((getColorSpaceType (p) == ColorSpace.TYPE_RGB) &&
> (getData (p, icSigMediaWhitePointTag) != null) &&
> (getData (p, icSigRedColorantTag) != null) &&
> (getData (p, icSigGreenColorantTag) != null) &&
> (getData (p, icSigBlueColorantTag) != null) &&
> (getData (p, icSigRedTRCTag) != null) &&
> (getData (p, icSigGreenTRCTag) != null) &&
> (getData (p, icSigBlueTRCTag) != null)) {
> thisProfile = new ICC_ProfileRGB (p);
>
> Is that really looks fine?
This looks even worse than the case in this PR which has only two conditions.
If it'd been the new code, I wouldn't have agreed to revert indentation.
Let's wait for another opinion…
To make it clear, you're for keeping the indentation as it was in the original
PR to visually separate condition from the statement in the if block. Do I get
it right, @mrserb?
-
PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v2]
On Thu, 1 Apr 2021 13:28:50 GMT, Alexey Ivanov wrote:
>>> By mapping i mean same indentation for all conditions in if statement…
>>
>> Okay, it's the first time I've come across to such a use of `map`.
>>
>>> I have not come across code in java.desktop where we add indentation at
>>> each continuation line of 'if' condition.
>>
>> Take a look at
>> [DefaultTableCellRenderer.getTableCellRendererComponent](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/table/DefaultTableCellRenderer.java#L201).
>>
>>> I understand difficulty to scan without indentation but then in cases where
>>> we have multiple lines on continuation line in if statement we will easily
>>> hit 80 characters limit.
>>
>> What is more important code readability or the strict limit of 80 columns?
>>
>> All in all, all these styles are used throughout `java.desktop` module.
>> I chose to opt for readability in this particular case and the line fits
>> into 80 column limit.
>>
>> Do I revert the change to this line?
>> Any other suggestions? What is your preference?
>
>> I would prefer if you revert this line or if we want to put emphasis on
>> readability moving '{' to new line also seems fine.
>
> Done.
I do not suggest that the change should be moved forth and back, but I think
that the second conditions should always be shifted, and if this causes 80
chars overflow then some other line split/rename/etc should be done to prevent
that.
Recent example of such style...:
if ((getColorSpaceType (p) == ColorSpace.TYPE_RGB) &&
(getData (p, icSigMediaWhitePointTag) != null) &&
(getData (p, icSigRedColorantTag) != null) &&
(getData (p, icSigGreenColorantTag) != null) &&
(getData (p, icSigBlueColorantTag) != null) &&
(getData (p, icSigRedTRCTag) != null) &&
(getData (p, icSigGreenTRCTag) != null) &&
(getData (p, icSigBlueTRCTag) != null)) {
thisProfile = new ICC_ProfileRGB (p);
Is that really looks fine?
-
PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline [v4]
On Thu, 1 Apr 2021 14:15:37 GMT, Jayathirth D V wrote: >> In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state >> information because of which we ignore clip parameters set in rect clip and >> shape clip. We need to query and use encoders from EncoderManager to honour >> clip states in copyArea. > > Jayathirth D V has updated the pull request incrementally with one additional > commit since the last revision: > > Refactor code to use drawTex2Tex Marked as reviewed by psadhukhan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3283
Re: [OpenJDK 2D-Dev] RFR: 8258788: incorrect response to change in window insets [lanai]
On Thu, 1 Apr 2021 14:38:52 GMT, Alexey Ushakov wrote: > Dynamically change MTLLayer insets depending on FULL_WINDOW_CONTENT property. > MTLLayer.h header cleanup. Is it possible to automatically test it? src/java.desktop/macosx/native/libawt_lwawt/awt/AWTWindow.m line 1171: > 1169: layer.leftInset = > (jint)(screenContentRect.origin.x - frame.origin.x); > 1170: } > 1171: } Can you check that it will work in the "new" tabbed mode on big sur as well? Probably it is possible to swap coordinates during rendering in the layer and get rid of this field? - PR: https://git.openjdk.java.net/jdk/pull/3308
Re: [OpenJDK 2D-Dev] RFR: 8264428: Cleanup usages of StringBuffer in java.desktop
On Thu, 1 Apr 2021 12:48:52 GMT, Andrey Turbanov wrote: >> Looks like this change goes beyond of simple replacement of StringBuffer >> with StringBuilder. Please update the description of the bug and PR >> description. > > I've updated PR description, but I don't have rights to update JIRA. I updated the bug synopsis in JIRA. - PR: https://git.openjdk.java.net/jdk/pull/3251
[OpenJDK 2D-Dev] RFR: 8258788: incorrect response to change in window insets [lanai]
Dynamically change MTLLayer insets depending on FULL_WINDOW_CONTENT property. MTLLayer.h header cleanup. - Commit messages: - 8258788: incorrect response to change in window insets [lanai] Changes: https://git.openjdk.java.net/jdk/pull/3308/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3308=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8258788 Stats: 31 lines in 7 files changed: 16 ins; 13 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3308.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3308/head:pull/3308 PR: https://git.openjdk.java.net/jdk/pull/3308
Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline [v4]
> In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state > information because of which we ignore clip parameters set in rect clip and > shape clip. We need to query and use encoders from EncoderManager to honour > clip states in copyArea. Jayathirth D V has updated the pull request incrementally with one additional commit since the last revision: Refactor code to use drawTex2Tex - Changes: - all: https://git.openjdk.java.net/jdk/pull/3283/files - new: https://git.openjdk.java.net/jdk/pull/3283/files/9b4a90ab..91e068b9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3283=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3283=02-03 Stats: 35 lines in 1 file changed: 0 ins; 22 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/3283.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3283/head:pull/3283 PR: https://git.openjdk.java.net/jdk/pull/3283
Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline [v3]
On Thu, 1 Apr 2021 06:58:27 GMT, Prasanta Sadhukhan wrote: >> Jayathirth D V has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Comment update > > src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBlitLoops.m line > 826: > >> 824: id interEncoder = >> 825: [mtlc.encoderManager getTextureEncoder:interTexture >> 826:isSrcOpaque:dstOps->isOpaque > > Should it not be srcOps->isOpaque? Both source and destination are same in copyArea. So isOpaque property will be same. But we use 32 bit non-opaque intermediate texture, so for intermediate texture we should pass JNI_FALSE for isOpaque property. > src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBlitLoops.m line > 854: > >> 852: atIndex:MeshVertexBuffer]; >> 853: [finalEncoder setFragmentTexture:interTexture atIndex: 0]; >> 854: [finalEncoder drawPrimitives:MTLPrimitiveTypeTriangle >> vertexStart:0 > > Can't we reuse drawTex2Tex() for this snippet? Thanks for the suggestion Prasanta, yes we should refactor code to use drawTex2Tex. I have updated the PR. - PR: https://git.openjdk.java.net/jdk/pull/3283
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v2]
On Thu, 1 Apr 2021 13:19:07 GMT, Alexey Ivanov wrote:
>> I would prefer if you revert this line or if we want to put emphasis on
>> readability moving '{' to new line also seems fine.
>
>> By mapping i mean same indentation for all conditions in if statement…
>
> Okay, it's the first time I've come across to such a use of `map`.
>
>> I have not come across code in java.desktop where we add indentation at each
>> continuation line of 'if' condition.
>
> Take a look at
> [DefaultTableCellRenderer.getTableCellRendererComponent](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/table/DefaultTableCellRenderer.java#L201).
>
>> I understand difficulty to scan without indentation but then in cases where
>> we have multiple lines on continuation line in if statement we will easily
>> hit 80 characters limit.
>
> What is more important code readability or the strict limit of 80 columns?
>
> All in all, all these styles are used throughout `java.desktop` module.
> I chose to opt for readability in this particular case and the line fits into
> 80 column limit.
>
> Do I revert the change to this line?
> Any other suggestions? What is your preference?
> I would prefer if you revert this line or if we want to put emphasis on
> readability moving '{' to new line also seems fine.
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v2]
On Thu, 1 Apr 2021 13:28:48 GMT, Alexey Ivanov wrote: >> When `getAllPrinterNames()` returns null, the list of `printServices` is >> assigned a new empty array without invalidating old services which were in >> the array before. >> >> The old print services should be invalidated. > > Alexey Ivanov has updated the pull request incrementally with one additional > commit since the last revision: > > Revert indentation of if condition continuation line Marked as reviewed by jdv (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v2]
> When `getAllPrinterNames()` returns null, the list of `printServices` is > assigned a new empty array without invalidating old services which were in > the array before. > > The old print services should be invalidated. Alexey Ivanov has updated the pull request incrementally with one additional commit since the last revision: Revert indentation of if condition continuation line - Changes: - all: https://git.openjdk.java.net/jdk/pull/3151/files - new: https://git.openjdk.java.net/jdk/pull/3151/files/98129416..edc8ab06 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3151=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3151=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3151.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3151/head:pull/3151 PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers
On Thu, 1 Apr 2021 13:10:40 GMT, Jayathirth D V wrote:
>> By mapping i mean same indentation for all conditions in if statement
>> without adding additional indentation for each continuation line
>> like(Basically line without your change of indentation)
>> if ((condition1) &&
>> (condition2)) {
>> }
>>
>> or
>>
>> if ((condition1)
>> &&(condition2)) {
>> }
>>
>> I have not come across code in java.desktop where we add indentation at each
>> continuation line of 'if' condition.
>> I understand difficulty to scan without indentation but then in cases where
>> we have multiple lines on continuation line in if statement we will easily
>> hit 80 characters limit.
>>
>> If we want to differentiate between if conditions and actual statement
>> execution to improve readability, we can move the statement block to new
>> line like
>> if ((condition1) &&
>> (condition2))
>> {
>> }
>
> I would prefer if you revert this line or if we want to put emphasis on
> readability moving '{' to new line also seems fine.
> By mapping i mean same indentation for all conditions in if statement…
Okay, it's the first time I've come across to such a use of `map`.
> I have not come across code in java.desktop where we add indentation at each
> continuation line of 'if' condition.
Take a look at
[DefaultTableCellRenderer.getTableCellRendererComponent](https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/javax/swing/table/DefaultTableCellRenderer.java#L201).
> I understand difficulty to scan without indentation but then in cases where
> we have multiple lines on continuation line in if statement we will easily
> hit 80 characters limit.
What is more important code readability or the strict limit of 80 columns?
All in all, all these styles are used throughout `java.desktop` module.
I chose to opt for readability in this particular case and the line fits into
80 column limit.
Do I revert the change to this line?
Any other suggestions? What is your preference?
-
PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers
On Thu, 1 Apr 2021 12:57:44 GMT, Jayathirth D V wrote:
>> Is it not right?
>> I admit I don't understand what you mean by _map_ here.
>>
>> When the code is written like it was:
>> if ((printServices[j] instanceof Win32PrintService) &&
>> (!printServices[j].equals(defaultPrintService))) {
>>
>> ((Win32PrintService)printServices[j]).invalidateService();
>> }
>> it's hard to scan: it's not clear what is part of the condition and what is
>> the statement inside the if block.
>>
>> I'd prefer to write it like this:
>> if ((printServices[j] instanceof Win32PrintService)
>> && (!printServices[j].equals(defaultPrintService))) {
>>
>>
>> ((Win32PrintService)printServices[j]).invalidateService();
>> }
>> That is moving the operator to the continuation line which makes it obvious
>> it is a continuation line of the condition and adding a blank line before
>> the statement in the code. It's still not perfect, however; and it changes
>> the author in `blame` output.
>>
>> I indented the continuation line by additional 8 spaces, which is also a
>> common practice in Java, to visually separate the condition and the
>> statement. In fact, it's IDE that updated the formatting, I decided to keep
>> it because it makes the code clearer.
>>
>> I can revert the change to this line if you like.
>> Or I can just add a blank line between the condition and the statement.
>>
>> What is your preference?
>
> By mapping i mean same indentation for all conditions in if statement without
> adding additional indentation for each continuation line like(Basically line
> without your change of indentation)
> if ((condition1) &&
> (condition2)) {
> }
>
> or
>
> if ((condition1)
> &&(condition2)) {
> }
>
> I have not come across code in java.desktop where we add indentation at each
> continuation line of 'if' condition.
> I understand difficulty to scan without indentation but then in cases where
> we have multiple lines on continuation line in if statement we will easily
> hit 80 characters limit.
>
> If we want to differentiate between if conditions and actual statement
> execution to improve readability, we can move the statement block to new line
> like
> if ((condition1) &&
> (condition2))
> {
> }
I would prefer if you revert this line or if we want to put emphasis on
readability moving '{' to new line also seems fine.
-
PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers
On Thu, 1 Apr 2021 12:05:30 GMT, Alexey Ivanov wrote:
>> src/java.desktop/windows/classes/sun/print/PrintServiceLookupProvider.java
>> line 159:
>>
>>> 157: for (int j=0; j < printServices.length; j++) {
>>> 158: if ((printServices[j] instanceof Win32PrintService) &&
>>> 159:
>>> (!printServices[j].equals(defaultPrintService))) {
>>
>> Is this indentation right? I thought we always map newline additional
>> conditions and not add extra indentation.
>
> Is it not right?
> I admit I don't understand what you mean by _map_ here.
>
> When the code is written like it was:
> if ((printServices[j] instanceof Win32PrintService) &&
> (!printServices[j].equals(defaultPrintService))) {
> ((Win32PrintService)printServices[j]).invalidateService();
> }
> it's hard to scan: it's not clear what is part of the condition and what is
> the statement inside the if block.
>
> I'd prefer to write it like this:
> if ((printServices[j] instanceof Win32PrintService)
> && (!printServices[j].equals(defaultPrintService))) {
>
> ((Win32PrintService)printServices[j]).invalidateService();
> }
> That is moving the operator to the continuation line which makes it obvious
> it is a continuation line of the condition and adding a blank line before the
> statement in the code. It's still not perfect, however; and it changes the
> author in `blame` output.
>
> I indented the continuation line by additional 8 spaces, which is also a
> common practice in Java, to visually separate the condition and the
> statement. In fact, it's IDE that updated the formatting, I decided to keep
> it because it makes the code clearer.
>
> I can revert the change to this line if you like.
> Or I can just add a blank line between the condition and the statement.
>
> What is your preference?
By mapping i mean same indentation for all conditions in if statement without
adding additional indentation for each continuation line like(Basically line
without your change of indentation)
if ((condition1) &&
(condition2)) {
}
or
if ((condition1)
&&(condition2)) {
}
I have not come across code in java.desktop where we add indentation at each
continuation line of 'if' condition.
I understand difficulty to scan without indentation but then in cases where we
have multiple lines on continuation line in if statement we will easily hit 80
characters limit.
If we want to differentiate between if conditions and actual statement
execution to improve readability, we can move the statement block to new line
like
if ((condition1) &&
(condition2))
{
}
-
PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8264428: Cleanup usages of StringBuffer in java.desktop
On Wed, 31 Mar 2021 21:12:37 GMT, Sergey Bylokhov wrote: >> Submitted: https://bugs.openjdk.java.net/browse/JDK-8264428, please rename >> PR to "8264428: Replace uses of StringBuffer with StringBuilder in >> java.desktop". > > Looks like this change goes beyond of simple replacement of StringBuffer > with StringBuilder. Please update the description of the bug and PR > description. I've updated PR description, but I don't have rights to update JIRA. - PR: https://git.openjdk.java.net/jdk/pull/3251
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers
On Thu, 1 Apr 2021 06:04:32 GMT, Jayathirth D V wrote:
>> When `getAllPrinterNames()` returns null, the list of `printServices` is
>> assigned a new empty array without invalidating old services which were in
>> the array before.
>>
>> The old print services should be invalidated.
>
> src/java.desktop/windows/classes/sun/print/PrintServiceLookupProvider.java
> line 159:
>
>> 157: for (int j=0; j < printServices.length; j++) {
>> 158: if ((printServices[j] instanceof Win32PrintService) &&
>> 159:
>> (!printServices[j].equals(defaultPrintService))) {
>
> Is this indentation right? I thought we always map newline additional
> conditions and not add extra indentation.
Is it not right?
I admit I don't understand what you mean by _map_ here.
When the code is written like it was:
if ((printServices[j] instanceof Win32PrintService) &&
(!printServices[j].equals(defaultPrintService))) {
((Win32PrintService)printServices[j]).invalidateService();
}
it's hard to scan: it's not clear what is part of the condition and what is the
statement inside the if block.
I'd prefer to write it like this:
if ((printServices[j] instanceof Win32PrintService)
&& (!printServices[j].equals(defaultPrintService))) {
((Win32PrintService)printServices[j]).invalidateService();
}
That is moving the operator to the continuation line which makes it obvious it
is a continuation line of the condition and adding a blank line before the
statement in the code. It's still not perfect, however; and it changes the
author in `blame` output.
I indented the continuation line by additional 8 spaces, which is also a common
practice in Java, to visually separate the condition and the statement. In
fact, it's IDE that updated the formatting, I decided to keep it because it
makes the code clearer.
I can revert the change to this line if you like.
Or I can just add a blank line between the condition and the statement.
What is your preference?
-
PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline [v3]
On Thu, 1 Apr 2021 05:49:51 GMT, Jayathirth D V wrote: >> In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state >> information because of which we ignore clip parameters set in rect clip and >> shape clip. We need to query and use encoders from EncoderManager to honour >> clip states in copyArea. > > Jayathirth D V has updated the pull request incrementally with one additional > commit since the last revision: > > Comment update src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBlitLoops.m line 826: > 824: id interEncoder = > 825: [mtlc.encoderManager getTextureEncoder:interTexture > 826:isSrcOpaque:dstOps->isOpaque Should it not be srcOps->isOpaque? src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBlitLoops.m line 854: > 852: atIndex:MeshVertexBuffer]; > 853: [finalEncoder setFragmentTexture:interTexture atIndex: 0]; > 854: [finalEncoder drawPrimitives:MTLPrimitiveTypeTriangle > vertexStart:0 Can't we reuse drawTex2Tex() for this snippet? - PR: https://git.openjdk.java.net/jdk/pull/3283
[OpenJDK 2D-Dev] Integrated: 8262470: Printed GlyphVector outline with low DPI has bad quality on Windows
On Fri, 26 Feb 2021 19:40:22 GMT, Alexander Scherbatiy wrote: > Printing text using GlyphVector outline has bad quality on printers with low > DPI on Windows. > The GDI system used for text printing on Windows accepts only integer path > coordinates. > Rounding GlyphVector outline coordinates leads to distorted printed text. > > The issue had been reported as JDK-8256264 but was reverted because of the > regression JDK-8259007 "This test printed a blank page". > > The fix JDK-8256264 scaled coordinates in wPrinterJob.moveTo()/lineTo() > methods up and scaled transforms in wPrinterJob.beginPath()/endPath() down. > > The regression was in the WPathGraphics.deviceDrawLine() method which uses > wPrinterJob.moveTo()/lineTo() methods without surrounding them with > wPrinterJob.beginPath()/endPath() so the line coordinates were only scaled up. > > I tried to put wPrinterJob.beginPath()/endPath() methods around > wPrinterJob.moveTo()/lineTo() in the method WPathGraphics.deviceDrawLine() > but the line was not drawn at all even without scaling coordinates up and > transform down (without JDK-8256264 fix). It looks like GDI treats this case > as an empty shape. > > The proposed fix applies path coordinates and transform scaling only in > WPathGraphics.convertToWPath() method. > The one more PathPrecisionScaleFactorShapeTest.java manual test is added > which checks that all methods that draw paths in WPathGraphics are used: line > in WPathGraphics.deviceDrawLine() and SEG_LINETO/SEG_QUADTO/SEG_CUBICTO in > WPathGraphics.convertToWPath() . > > The `java/awt/print` and `java/awt/PrintJob` automatic and manual tests were > run on Windows 10 Pro with the fix. > > There are two failed automated tests which fail without the fix as well: > java/awt/print/PrinterJob/GlyphPositions.java > java/awt/print/PrinterJob/PSQuestionMark.java > > The following manual tests have issues on my system: > - `java/awt/print/Dialog/PrintDlgPageable.java` > java.lang.IllegalAccessException: class > com.sun.javatest.regtest.agent.MainWrapper$MainThread cannot access a member > of class PrintDlgPageable with modifiers "public static" > > - `java/awt/print/PrinterJob/PrintAttributeUpdateTest.java` I select pages > radio button, press the print button but the test does not finish and I do > not see any other dialogs with pass/fail buttons. > > - `java/awt/PrintJob/PrintCheckboxTest/PrintCheckboxManualTest.java` Tests > that there is no ClassCastException thrown in printing checkbox and scrollbar > with XAWT. Error. Can't find HTML file: > test\jdk\java\awt\PrintJob\PrintCheckboxTest\PrintCheckboxManualTest.html > > > - `java/awt/print/PrinterJob/SecurityDialogTest.java` A windows with > instructions is shown but it does not contain print/pass/fail buttons and it > is not possible to close the window. > > - The tests below fail with "Error. Parse Exception: Arguments to `manual' > option not supported: yesno" message: > java/awt/print/Dialog/DialogOrient.java > java/awt/print/Dialog/DialogType.java > java/awt/print/PrinterJob/ImagePrinting/ClippedImages.java > java/awt/print/PrinterJob/ImagePrinting/ImageTypes.java > java/awt/print/PrinterJob/ImagePrinting/PrintARGBImage.java > java/awt/print/PrinterJob/PageDialogTest.java > java/awt/print/PrinterJob/PageRanges.java > java/awt/print/PrinterJob/PageRangesDlgTest.java > java/awt/print/PrinterJob/PrintGlyphVectorTest.java > java/awt/print/PrinterJob/PrintLatinCJKTest.java > java/awt/print/PrinterJob/PrintTextTest.java > java/awt/print/PrinterJob/SwingUIText.java > java/awt/PrintJob/ConstrainedPrintingTest/ConstrainedPrintingTest.java > java/awt/PrintJob/PageSetupDlgBlockingTest/PageSetupDlgBlockingTest.java > java/awt/PrintJob/SaveDialogTitleTest.java This pull request has now been integrated. Changeset: 02287349 Author:Alexander Scherbatiy URL: https://git.openjdk.java.net/jdk/commit/02287349 Stats: 731 lines in 5 files changed: 728 ins; 0 del; 3 mod 8262470: Printed GlyphVector outline with low DPI has bad quality on Windows Reviewed-by: serb, psadhukhan - PR: https://git.openjdk.java.net/jdk/pull/2756
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers
On Tue, 23 Mar 2021 13:45:33 GMT, Alexey Ivanov wrote:
> When `getAllPrinterNames()` returns null, the list of `printServices` is
> assigned a new empty array without invalidating old services which were in
> the array before.
>
> The old print services should be invalidated.
src/java.desktop/windows/classes/sun/print/PrintServiceLookupProvider.java line
159:
> 157: for (int j=0; j < printServices.length; j++) {
> 158: if ((printServices[j] instanceof Win32PrintService) &&
> 159:
> (!printServices[j].equals(defaultPrintService))) {
Is this indentation right? I thought we always map newline additional
conditions and not add extra indentation.
-
PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers
On Wed, 24 Mar 2021 20:44:26 GMT, Alexey Ivanov wrote: >> Since this is windows specific code, I am not sure if system will not have >> any printers. I guess by default, Microsoft XPS Document Writer, Microsoft >> Print-to-PDF, Fax are present in printers list. That is the reason we do not >> have getPrintService() return 0 printer although there may not be any real >> printer present in windows system...reason for some failure in jtreg test >> which caused us to use `@key printer `tag in those tests to make real >> printers are configured. > >> Since this is windows specific code, I am not sure if system will not have >> any printers. I guess by default, Microsoft XPS Document Writer, Microsoft >> Print-to-PDF, Fax are present in printers list. > > Yes, that's correct. But you can remove them. You can also configure the > image so that these are not installed by default. > >> That is the reason we do not have getPrintService() return 0 printer >> although there may not be any real printer present in windows >> system...reason for some failure in jtreg test which caused us to use `@key >> printer `tag in those tests to make real printers are configured. > > I agree that getting zero printers in Windows is unlikely. But it's still > possible. > > Consider the following scenario: Let's assume there are 10 printers in the > system. The user removes 5 of them. In this case, `invalidateService()` is > called on the instances of `Win32PrintService` which were removed from the > system. > > Then the user removes the remaining 5 printers. In this case, > `invalidateService()` is not called at all. If an application has references > to any of these instances, they will continue to appear operational. However, > the flag `isInvalid` in `Win32PrintService` is used in two methods only: > `getPrinterState` and `getPrinterStateReasons`. > > This fix is minor, probably this situation never occurs in the real life. > > The difference in handling the deleted services caught my attention. If > everyone agrees it's not problem, I'll withdraw the PR and close the bug as > _Not an Issue_. Change looks like good practice and needed, eventhough it may occur rarely. - PR: https://git.openjdk.java.net/jdk/pull/3151
