[OpenJDK 2D-Dev] <Swing Dev> [11] JDK-8153732: Windows remote printer changes do not reflect in lookupPrintServices()
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Fri Jun 22 08:11:51 UTC 2018
looks good. One more thing, you could probably use
if (info4->Attributes & PRINTER_ATTRIBUTE_NETWORK) {
instead of hardcoding
if (info4->Attributes & 0x00000010) {
Also,
getAllPrinterNames() shares more than 80% code with your recently addedGetRemotePrintersNames(). Probably we could optimise getAllPrinterNames
to use yours by having a parameter(all/remote) but it's upto you.
Regards Prasanta
On 6/22/2018 12:53 PM, Shashidhara Veerabhadraiah wrote:
>
> Hi Prasanta, Thank you for the review. Here is the new Webrev:
>
> http://cr.openjdk.java.net/~sveerabhadra/8153732/webrev.05/
> <http://cr.openjdk.java.net/%7Esveerabhadra/8153732/webrev.05/>
>
> For the EnumPrinters case, I think the cReturned is sufficient as it
> is set to zero every time we start.
>
> Thanks and regards,
>
> Shashi
>
> *From:*Prasanta Sadhukhan
> *Sent:* Friday, June 22, 2018 10:41 AM
> *To:* Shashidhara Veerabhadraiah
> <shashidhara.veerabhadraiah at oracle.com>; Philip Race
> <philip.race at oracle.com>; 2d-dev <2d-dev at openjdk.java.net>
> *Subject:* Re: [OpenJDK 2D-Dev] <Swing Dev> [11] JDK-8153732: Windows
> remote printer changes do not reflect in lookupPrintServices()
>
> I guess
>
> if (pollStr.equalsIgnoreCase("true")) {
> 70 pollServices = true;
> 71 } else if (pollStr.equalsIgnoreCase("false")) {
> 72 pollServices = false;
> 73 }
> can be written as
> if (pollStr.equalsIgnoreCase("false"))
> pollServices = false;
> as it is already defaulted to true.
>
> 78 * for polling PrintServices. The default is 120.
>
> I guess default is 240.
>
> also, EnumPrinters returns bool, which we should check like
>
> 263 if (cReturned > 0 && enumprintersret !=0 ) {
> Regards
> Prasanta
>
> On 6/22/2018 10:03 AM, Shashidhara Veerabhadraiah wrote:
>
> Hi Phil, Thanks for your review.
>
> I have made your suggested changes and here is the updated webrev:
>
> http://cr.openjdk.java.net/~sveerabhadra/8153732/webrev.04/
> <http://cr.openjdk.java.net/%7Esveerabhadra/8153732/webrev.04/>
>
> Need one more review to commit this.
>
> Note: A network printer can be added via the ‘Add a printer’ under
> the ‘Devices and Printers’ dialog. A network printer will have the
> property ‘Device description’ set to ‘Network printer connection’.
>
> Thanks and regards,
>
> Shashi
>
> *From:*Phil Race
> *Sent:* Friday, June 22, 2018 2:16 AM
> *To:* Shashidhara Veerabhadraiah
> <shashidhara.veerabhadraiah at oracle.com>
> <mailto:shashidhara.veerabhadraiah at oracle.com>; 2d-dev
> <2d-dev at openjdk.java.net> <mailto:2d-dev at openjdk.java.net>
> *Subject:* Re: <Swing Dev> [11] JDK-8153732: Windows remote
> printer changes do not reflect in lookupPrintServices()
>
> I thought you were going to make the refresh time 4 minutes ?
> I don't see that it has to be the same on Windows as it was on Unix,
> if you say 4 minutes is a sensible value there .. and 4 mins will be
> less CPU wake up, so I'd back that (4 mins) ahead of 2 minutes as
> the default here.
>
> You still have missing white space
>
> 432 while(true) {
>
>
> With these two changes you have my +1 ..
>
> BTW the native diff looks MUCH better now - thanks !
>
> -phil.
>
> On 06/21/2018 01:33 PM, Shashidhara Veerabhadraiah wrote:
>
> Hi Phil, Here is the new Webrev for changes you suggested.
>
> http://cr.openjdk.java.net/~sveerabhadra/8153732/webrev.03/
> <http://cr.openjdk.java.net/%7Esveerabhadra/8153732/webrev.03/>
>
> Thanks and regards,
>
> Shashi
>
> *From:*Phil Race
> *Sent:* Friday, June 22, 2018 1:02 AM
> *To:* Shashidhara Veerabhadraiah
> <shashidhara.veerabhadraiah at oracle.com>
> <mailto:shashidhara.veerabhadraiah at oracle.com>; 2d-dev
> <2d-dev at openjdk.java.net> <mailto:2d-dev at openjdk.java.net>
> *Subject:* Re: <Swing Dev> [11] JDK-8153732: Windows remote
> printer changes do not reflect in lookupPrintServices()
>
>
>
> + private static final long DELAY = 1000 * 60 * 4; // 4
> min pooling
>
> I think we need a System Property that can control this.
>
> I suggest the name "sun.java2d.print.minRefreshTime" which is
> what we use on Unix.
>
> and similarly to there we should have
> "sun.java2d.print.polling" which is a boolean
>
> and controls whether we do this at all ..
>
> See
> src/java.desktop/unix/classes/sun/print/PrintServiceLookupProvider.java
>
> Lots of places where you are missing a space before "("
>
> + if(str1.length != str2.length) {
>
> + for(int i = 0;i < str1.length;i++) {
>
> + for(int j = 0;j < str2.length;j++) {
>
> + if(!str1[i].equals(str2[j])) {
>
>
>
> + while(true) {
>
> + if(doCompare(prevRemotePrinters,
> currentRemotePrinters)) {
>
> There's some of that in native too
>
> 266 if(info4->Attributes & 0x00000010) {
>
> In
> http://cr.openjdk.java.net/~sveerabhadra/8153732/webrev.01/src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp.sdiff.html
> <http://cr.openjdk.java.net/%7Esveerabhadra/8153732/webrev.01/src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp.sdiff.html>
>
> you seem to have moved all the existing and (I think)
> unchanged related functions so the DIFF appears
>
> much bigger than it really is. Please move them back and
> re-generate.
>
> The test really needs to provide an "out" for someone running
> the test who has no way
>
> to add a network printer .. they don't want to have to fail
> the test.
>
> As well as that, arguably this should be an @ignore test, so
> it is not run unless
>
> you are trying to run all the tests.
>
> -phil.
>
> On 06/21/2018 12:08 PM, Shashidhara Veerabhadraiah wrote:
>
> Hi Phil, Here is the new Webrev. I chose 4 mins because I
> think it takes around 2 mins to add a new network printer,
> hence I felt 4 mins is a good time.
>
> The windows **PrinterChangeNotifications** calls are a
> blocking function calls hence I could not add the remote
> printers monitor to the existing thread. Hence there is a
> new thread being added to listen to remote printers status
> changes.
>
> http://cr.openjdk.java.net/~sveerabhadra/8153732/webrev.01/
> <http://cr.openjdk.java.net/%7Esveerabhadra/8153732/webrev.01/>
>
> Thanks and regards,
>
> Shashi
>
> *From:*Philip Race
> *Sent:* Thursday, June 21, 2018 5:38 AM
> *To:* 2d-dev <2d-dev at openjdk.java.net>
> <mailto:2d-dev at openjdk.java.net>; Shashidhara
> Veerabhadraiah <shashidhara.veerabhadraiah at oracle.com>
> <mailto:shashidhara.veerabhadraiah at oracle.com>
> *Subject:* Fwd: Re: <Swing Dev> [11] JDK-8153732: Windows
> remote printer changes do not reflect in lookupPrintServices()
>
> The main concern I have is we now have a busy thread
> burning CPU ..
> bad for laptops .. and if we add a delay we have less
> prompt notification
> of a new local printer.
>
> I think the compromise is that the existing thread maybe
> kept as is,
> and we add a new thread that pools every 10 minutes for a
> remote printer.
>
> If we can make the existing thread wake up from its wait
> and do that, even better.
>
> -phil.
>
> -------- Original Message --------
>
> *Subject: *
>
>
>
> Re: [11] JDK-8153732: Windows remote printer changes do
> not reflect in lookupPrintServices()
>
> *Date: *
>
>
>
> Wed, 20 Jun 2018 17:03:56 -0700
>
> *From: *
>
>
>
> Philip Race <philip.race at oracle.com>
> <mailto:philip.race at oracle.com>
>
> *Organization: *
>
>
>
> Oracle Corporation
>
> *To: *
>
>
>
> Shashidhara Veerabhadraiah
> <shashidhara.veerabhadraiah at oracle.com>
> <mailto:shashidhara.veerabhadraiah at oracle.com>
>
> *CC: *
>
>
>
> awt-dev at openjdk.java.net
> <mailto:awt-dev at openjdk.java.net>,
> swing-dev at openjdk.java.net <mailto:swing-dev at openjdk.java.net>
>
>
>
> This is on the wrong lists. Not Swing. Not AWT. Should be 2d.
> I'll forward it there and continue there. Consider the
> AWT+Swing threads dead.
>
> -phil.
>
> On 6/20/18, 3:12 AM, Shashidhara Veerabhadraiah wrote:
>
> Hi All, Please review this code changes for the below
> enhancement.
>
> Enhancement:
> https://bugs.openjdk.java.net/browse/JDK-8153732
>
> Webrev:
> http://cr.openjdk.java.net/~sveerabhadra/8153732/webrev.00/
> <http://cr.openjdk.java.net/%7Esveerabhadra/8153732/webrev.00/>
>
> Details of the changes: Windows provides
> *PrinterChangeNotification* functions that provides
> information about printer status changes of the local
> printers(subset) but not network printers.
> Alternatively, Windows provides a way thro' which one
> can get the network printer status changes by using
> WMI, RegistryKeyChange combination, which is a
> slightly complex mechanism.
> The Windows WMI offers an async and sync method to
> read thro' registry via the WQL query. The async
> method is considered dangerous as it leaves open a
> channel until we close it. But the async method has
> the advantage of being notified of a change in
> registry by calling callback without polling for
> it. The sync method uses the polling mechanism to notify.
> RegistryValueChange cannot be used in combination with
> WMI to get registry value change notification because
> of an error that may be generated because the scope of
> the query would be too big to handle(at times).
> Hence an alternative mechanism is choosen via the
> EnumPrinters by polling for the count of printer
> status changes(add\remove) and based on it update the
> printers list(both local and remote printers - superset).
>
> Thanks and regards,
>
> Shashi
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20180622/a01623e6/attachment-0001.html>
More information about the 2d-dev
mailing list