[OpenJDK 2D-Dev] <Swing Dev> [11] JDK-8153732: Windows remote printer changes do not reflect in lookupPrintServices()
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Mon Jun 25 05:22:20 UTC 2018
looks good.
Regards
Prasanta
On 6/25/2018 10:48 AM, Shashidhara Veerabhadraiah wrote:
>
> My bad Prashanta. Here is the updated Webrev:
>
> http://cr.openjdk.java.net/~sveerabhadra/8153732/webrev.07/
> <http://cr.openjdk.java.net/%7Esveerabhadra/8153732/webrev.07/>
>
> Thanks and regards,
> Shashi
>
> *From:*Prasanta Sadhukhan
> *Sent:* Friday, June 22, 2018 2:46 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()
>
> you are using in java
>
> 409 prevRemotePrinters = GetRemotePrintersNames();
> 431 String[] currentRemotePrinters =
> GetRemotePrintersNames();
>
>
> while you are at it, please change
>
> 269 if (info4->Attributes & 0x00000010) {
> to
> if (info4->Attributes & PRINTER_ATTRIBUTE_NETWORK) {
> as the hardcoded value might change in future.
> Regards
> Prasanta
>
> On 6/22/2018 2:32 PM, Shashidhara Veerabhadraiah wrote:
>
> Hi Prasanta, Here is the new webrev for that change:
>
> http://cr.openjdk.java.net/~sveerabhadra/8153732/webrev.06/
> <http://cr.openjdk.java.net/%7Esveerabhadra/8153732/webrev.06/>
>
> Thanks and regards,
>
> Shashi
>
> *From:*Prasanta Sadhukhan
> *Sent:* Friday, June 22, 2018 2:00 PM
> *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()
>
> 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>
> <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()
>
> 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/20180625/9c7dbc85/attachment-0001.html>
More information about the 2d-dev
mailing list