[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:29:35 UTC 2018
btw,
GetRemotePrintersNames violates camelcase style. I suggest to use getRemotePrintersNames
Regards
Prasanta
On 6/22/2018 1:56 PM, Shashidhara Veerabhadraiah wrote:
>
> Thank you Prasanta for the review.
>
> Thanks and regards,
>
> Shashi
>
> *From:*Prasanta Sadhukhan
> *Sent:* Friday, June 22, 2018 1:42 PM
> *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()
>
> 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>
> <mailto:shashidhara.veerabhadraiah at oracle.com>; Philip Race
> <philip.race at oracle.com> <mailto:philip.race at oracle.com>; 2d-dev
> <2d-dev at openjdk.java.net> <mailto: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/638fd2a5/attachment-0001.html>
More information about the 2d-dev
mailing list