Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]
I have not looked at entire file but since this function was modified, I made that suggestion. I am not that particular if you wish to ignore that. Regards Prasanta Get Outlook for Android<https://aka.ms/AAb9ysg> From: 2d-dev <2d-dev-r...@openjdk.java.net> on behalf of Alexey Ivanov Sent: Wednesday, April 7, 2021 4:22:29 PM To: 2d-dev@openjdk.java.net <2d-dev@openjdk.java.net> Subject: Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3] On Wed, 7 Apr 2021 04:12:55 GMT, Prasanta Sadhukhan wrote: >> [Code Conventions for >> Java](https://www.oracle.com/java/technologies/javase/codeconventions-indentation.html#248) >> say, “Line wrapping for `if` statements should generally use the 8-space >> rule, since conventional (4 space) indentation makes seeing the body >> difficult.” (It's the second to last block on the page.) > > If we are adding a new line, then I think we should need to add at l129, l138 > otherwise it will look odd doing it at one place only. I haven't touched that code at all. Not that odd because it's isolated to the new function now. What is your suggestion? Refactor all if statements in these two functions? Submit a separate bug for refactor all if statements the entire file? Revert back to no new line, leaving this particular if untouched as well? - PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]
On Wed, 7 Apr 2021 04:12:55 GMT, Prasanta Sadhukhan wrote: >> [Code Conventions for >> Java](https://www.oracle.com/java/technologies/javase/codeconventions-indentation.html#248) >> say, “Line wrapping for `if` statements should generally use the 8-space >> rule, since conventional (4 space) indentation makes seeing the body >> difficult.” (It's the second to last block on the page.) > > If we are adding a new line, then I think we should need to add at l129, l138 > otherwise it will look odd doing it at one place only. I haven't touched that code at all. Not that odd because it's isolated to the new function now. What is your suggestion? Refactor all if statements in these two functions? Submit a separate bug for refactor all if statements the entire file? Revert back to no new line, leaving this particular if untouched as well? - PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]
On Tue, 6 Apr 2021 20:39:04 GMT, Alexey Ivanov wrote: >> I added a blank line between the condition and the body. >> Does it look good? @mrserb, @jayathirthrao > > [Code Conventions for > Java](https://www.oracle.com/java/technologies/javase/codeconventions-indentation.html#248) > say, “Line wrapping for `if` statements should generally use the 8-space > rule, since conventional (4 space) indentation makes seeing the body > difficult.” (It's the second to last block on the page.) If we are adding a new line, then I think we should need to add at l129, l138 otherwise it will look odd doing it at one place only. - PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]
On Tue, 6 Apr 2021 20:38:57 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: > > Add a blank line to separate condition from body 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 [v3]
On Tue, 6 Apr 2021 20:38:57 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: > > Add a blank line to separate condition from body Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]
On Tue, 6 Apr 2021 20:35:09 GMT, Alexey Ivanov wrote:
>> BTW another solution is to move { to the next line if the previous statement
>> was split across few lines.
>
> I added a blank line between the condition and the body.
> Does it look good? @mrserb, @jayathirthrao
[Code Conventions for
Java](https://www.oracle.com/java/technologies/javase/codeconventions-indentation.html#248)
say, “Line wrapping for `if` statements should generally use the 8-space rule,
since conventional (4 space) indentation makes seeing the body difficult.”
(It's the second to last block on the page.)
-
PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]
On Fri, 2 Apr 2021 01:56:37 GMT, Sergey Bylokhov wrote:
>>> 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.
I added a blank line between the condition and the body.
Does it look good? @mrserb, @jayathirthrao
-
PR: https://git.openjdk.java.net/jdk/pull/3151
Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]
> 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: Add a blank line to separate condition from body - Changes: - all: https://git.openjdk.java.net/jdk/pull/3151/files - new: https://git.openjdk.java.net/jdk/pull/3151/files/edc8ab06..774e4074 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3151=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3151=01-02 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 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
