[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 09:15:35 UTC 2018
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>; 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()
>
> 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/20180622/29ca6ce4/attachment-0001.html>
More information about the 2d-dev
mailing list