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: 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: 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
