[OpenJDK 2D-Dev] <Swing Dev> [11] JDK-8153732: Windows remote printer changes do not reflect in lookupPrintServices()
Philip Race
philip.race at oracle.com
Fri Jun 22 13:40:56 UTC 2018
On 6/22/18, 2:15 AM, Prasanta Sadhukhan wrote:
>
> you are using in java
>
> 409 prevRemotePrinters = GetRemotePrintersNames();
> 431 String[] currentRemotePrinters = GetRemotePrintersNames();
>
I am not sure how this compiles any more (the java code) since the
native declaration changed
> 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.
Not likely to change .. in fact it would make existing code fail to
work, but
good to do it from the point of understanding the code.
-phil.
>
>
> 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 usegetRemotePrintersNames
>>
>>
>> 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) {
>>
>>
>>
>> Inhttp://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/f5ae086b/attachment-0001.html>
More information about the 2d-dev
mailing list