On Thu, 7 Jul 2022 22:23:55 GMT, Phil Race <[email protected]> wrote:
> This fixes the issue that the default printer or set of available printers is
> fixed from the first call to it in the lifetime of the application.
> Now subsequent calls will check to see if there are changes.
> A manual test is provided for verifying this - it requires you to add/remove
> printers at the system level to verify anything related to this fix.
I tested it on Windows by adding and removing a printer, and it does what I
would expect. I left a couple questions on the implementation, along with a
minor formatting issue. You'll also need to fix the whitespace error that
`jcheck` reported.
modules/javafx.graphics/src/main/java/com/sun/prism/j2d/PrismPrintPipeline.java
line 100:
> 98: // The map is useful when updating
> 99: private static HashMap<PrintService, Printer> pMap = new HashMap<>();
> 100: //private static TreeSet<Printer> printers = null;
Since this is unused, I recommend to remove it.
modules/javafx.graphics/src/main/java/com/sun/prism/j2d/PrismPrintPipeline.java
line 138:
> 136: }
> 137: }
> 138: lastTime = System.currentTimeMillis();
Is `lastTime` intended to be the last time you updated the printers or the last
time the `getAllPrinters` method was called? If the former, then you would want
to set `lastTime` right after `updatePrinters`. If the latter, then it's fine
where it is.
modules/javafx.graphics/src/main/java/com/sun/prism/j2d/PrismPrintPipeline.java
line 192:
> 190: if (oldDefaultService != null && newDefaultService != null) {
> 191: if (!oldDefaultService.equals(newDefaultService)) {
> 192: defaultPrinter = findDefaultPrinter(printerSet,
> newDefaultService);
Don't you also want to find the default printer in the case where the old
`default` printer was null and the `newDefaultService` is not null?
modules/javafx.graphics/src/main/java/javafx/print/Printer.java line 95:
> 93: } else {
> 94: defaultPrinter.setValue(p);
> 95: }
Minor: indentation is off for these two lines
-------------
PR: https://git.openjdk.org/jfx/pull/817
