RFR: 8263984: Invalidate printServices when there are no printers

classic Classic list List threaded Threaded
30 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 8263984: Invalidate printServices when there are no printers

Alexey Ivanov-2
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.

-------------

Commit messages:
 - 8263984: Invalidate printServices when there are no printers

Changes: https://git.openjdk.java.net/jdk/pull/3151/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3151&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263984
  Stats: 8 lines in 1 file changed: 6 ins; 1 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Alexey Ivanov-2
On Tue, 23 Mar 2021 13:45:33 GMT, Alexey Ivanov <[hidden email]> 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 116:

> 114:     private synchronized void refreshServices() {
> 115:         String[] printers = getAllPrinterNames();
> 116:         if (printers == null) {

`getAllPrinterNames` returns null when an error occurs or when there are no printers in the system.

Up for discussion:
If an error occurs, we may leave the old services without invalidating them. In this case, it is necessary to distinguish between the error and the lack of printers in the system. Does this make any sense?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3151
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Alexey Ivanov-2
In reply to this post by Alexey Ivanov-2
On Tue, 23 Mar 2021 13:45:33 GMT, Alexey Ivanov <[hidden email]> 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 119:

> 117:             // In Windows it is safe to assume no default if printers == null so we
> 118:             // don't get the default.
> 119:             invalidateServices();

The default printer service, `defaultPrintService`, is usually in `printServices` array. It's possible that it's not in the array; in this case getDefaultPrintService() will reset it to `null` when accessed.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3151
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Prasanta Sadhukhan-2
In reply to this post by Alexey Ivanov-2
On Tue, 23 Mar 2021 13:45:33 GMT, Alexey Ivanov <[hidden email]> 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 120:

> 118:             // don't get the default.
> 119:             invalidateServices();
> 120:             printServices = new PrintService[0];

I am not sure this call to invalidateServices() is needed as we are creating a new `printServices` object of 0 length so making the old printServices invalid is redundant as it is same global variable.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3151
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Alexey Ivanov-2
On Wed, 24 Mar 2021 09:16:33 GMT, Prasanta Sadhukhan <[hidden email]> 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 120:
>
>> 118:             // don't get the default.
>> 119:             invalidateServices();
>> 120:             printServices = new PrintService[0];
>
> I am not sure this call to invalidateServices() is needed as we are creating a new `printServices` object of 0 length so making the old printServices invalid is redundant as it is same global variable.

That's exactly the problem I am addressing.

In the regular case, the existing services left in `printServices` are invalidated before `newServices` is assigned to it. Yet no existing services are invalidated if all the printers were removed from the system.

Why is it needed to invalidate deleted services if the list of printers is updated and if at least one printer is left in the system? Why is it not needed to invalidate deleted services if all the printers are removed from the system?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3151
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Prasanta Sadhukhan-2
On Wed, 24 Mar 2021 09:25:56 GMT, Alexey Ivanov <[hidden email]> wrote:

>> src/java.desktop/windows/classes/sun/print/PrintServiceLookupProvider.java line 120:
>>
>>> 118:             // don't get the default.
>>> 119:             invalidateServices();
>>> 120:             printServices = new PrintService[0];
>>
>> I am not sure this call to invalidateServices() is needed as we are creating a new `printServices` object of 0 length so making the old printServices invalid is redundant as it is same global variable.
>
> That's exactly the problem I am addressing.
>
> In the regular case, the existing services left in `printServices` are invalidated before `newServices` is assigned to it. Yet no existing services are invalidated if all the printers were removed from the system.
>
> Why is it needed to invalidate deleted services if the list of printers is updated and if at least one printer is left in the system? Why is it not needed to invalidate deleted services if all the printers are removed from the system?

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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3151
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Alexey Ivanov-2
On Wed, 24 Mar 2021 13:07:21 GMT, Prasanta Sadhukhan <[hidden email]> 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.

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_.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3151
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Sergey Bylokhov-2
In reply to this post by Alexey Ivanov-2
On Tue, 23 Mar 2021 13:45:33 GMT, Alexey Ivanov <[hidden email]> 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.

Marked as reviewed by serb (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3151
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Jayathirth D V-2
In reply to this post by Alexey Ivanov-2
On Wed, 24 Mar 2021 20:44:26 GMT, Alexey Ivanov <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Jayathirth D V-2
In reply to this post by Alexey Ivanov-2
On Tue, 23 Mar 2021 13:45:33 GMT, Alexey Ivanov <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Alexey Ivanov-2
On Thu, 1 Apr 2021 06:04:32 GMT, Jayathirth D V <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Jayathirth D V-2
On Thu, 1 Apr 2021 12:05:30 GMT, Alexey Ivanov <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Jayathirth D V-2
On Thu, 1 Apr 2021 12:57:44 GMT, Jayathirth D V <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers

Alexey Ivanov-2
On Thu, 1 Apr 2021 13:10:40 GMT, Jayathirth D V <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers [v2]

Alexey Ivanov-2
In reply to this post by Alexey Ivanov-2
> 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&pr=3151&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3151&range=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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers [v2]

Alexey Ivanov-2
In reply to this post by Alexey Ivanov-2
On Thu, 1 Apr 2021 13:19:07 GMT, Alexey Ivanov <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers [v2]

Jayathirth D V-2
In reply to this post by Alexey Ivanov-2
On Thu, 1 Apr 2021 13:28:48 GMT, Alexey Ivanov <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers [v2]

Sergey Bylokhov-2
In reply to this post by Alexey Ivanov-2
On Thu, 1 Apr 2021 13:28:50 GMT, Alexey Ivanov <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers [v2]

Alexey Ivanov-2
On Thu, 1 Apr 2021 17:29:08 GMT, Sergey Bylokhov <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8263984: Invalidate printServices when there are no printers [v2]

Sergey Bylokhov-2
In reply to this post by Sergey Bylokhov-2
On Thu, 1 Apr 2021 17:29:08 GMT, Sergey Bylokhov <[hidden email]> 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
12