[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8170578: The Pages Range From in print dialog is disabled
From: Philip Race <philip.race () oracle ! com>
Date: 2017-01-31 19:10:08
Message-ID: 5890E110.70502 () oracle ! com
[Download RAW message or body]
I spent some significant amount of time trying to make sure I know the
IPP protocol
that is underpinning this. Prasanta provided me with some additional
logging from
which it seems that there are 3 repeated attributes.
>
IPPPrintService>> readIPPResponse pwg-raster-document-resolution-supported
IPPPrintService>> readIPPResponse pwg-raster-document-sheet-back
IPPPrintService>> readIPPResponse pwg-raster-document-type-supported
Furthermore I have determined that 16.10 ships with CUPS 2.2 - released
only a few weeks before 16.10
Whereas 16.04 has 2.1.3. So what we have here is a major CUPS upgrade
and I am still not 100% sure
if these duplicates are a CUPS bug - but I think it probable.
These duplicated attributes are not important to our implementation, so
all we need to do
is make sure they don't break us. I don't see a reason to care if we
keep the first or replace
it with the second. Any new attributes we've previously ignored will
still be ignored.
What Prasanta has implemented will fix our bug so I am approving.
However our implementation looks odd to me - I do not understand why there
was an expectation a new HashMap ever needed to be created. It only makes
sense if duplicate attributes are normal. But then there is no rhyme nor
reason
to then simply picking the first HashMap and ignoring the rest.
If anything I'd expect that you may want to tie a HashMap to each group tag.
In which case the first one would be operational attributes, not printer
attributes.
And it isn't done like that anyway. It just creates a new one "on
demand" when
it sees the key is already used. This happens in the middle of
processing a particular group.
So it still makes no sense.
Accordingly I think we need a follow-on bug to examine, explain and if
necessary update this in 10.
-phil.
On 1/18/17, 3:11 AM, Prasanta Sadhukhan wrote:
>
> Ok. Updated webrev just in case we are not reading the protocol wrongly.
>
> http://cr.openjdk.java.net/~psadhukhan/8170578/webrev.02/
>
> Regards
> Prasanta
> On 1/12/2017 8:04 AM, Philip Race wrote:
> > I am looking at this and the hold up is I can't remember the protocol
> > readIPPResponse() is
> > trying to follow. Without that understanding it is hard to say much
> > .. so I need to go read
> > up and remember ..
> >
> > Two comments : There is an extra space in
> > if (responseMap.length> 1) {
> > The test update is bogus in referring to this bug. The change is
> > nothing to do with the bug.
> > And I actually prefer printer tests to throw nasty exceptions when
> > there are no printers
> > and they need one ... else SQE just don't configure printers on test
> > systems and the tests all wrongly pass.
> > Would you run Linux UI tests on a blade with no X-server running ?
> >
> > -phil.
> >
> > On 1/10/17, 1:14 AM, Prasanta Sadhukhan wrote:
> > >
> > >
> > >
> > > On 1/10/2017 2:43 PM, Prasanta Sadhukhan wrote:
> > > > Actually, in ubuntu16.10 attribute map did not have
> > > > "page-ranges-supported" attribute because 2 attribute hashmap is
> > > > created in IPPPrintService#readIPPResponse() and most of the
> > > > supported attributes are part of the 2nd hashmap (responseMap[1])
> > > > whereas only the 1st hashmap is utilised through responseMap[0]
> > > > [http://hg.openjdk.java.net/jdk9/client/jdk/file/8be0bb1aa238/src/java.desktop/unix/classes/sun/print/IPPPrintService.java#l1760]
> > > >
> > > > As to why 2 hashmaps are created in ubuntu16.10, it is because
> > > > readIPPResponse() checks if key is already present in existing map,
> > > > then create a new hashmap through this code
> > > > <http://hg.openjdk.java.net/jdk9/client/jdk/file/8be0bb1aa238/src/java.desktop/unix/classes/sun/print/IPPPrintService.java#l1887>
> > > > and in ubuntu16.10, some attributes (like
> > > > pwg-raster-document-type-supported which is not there in ubuntu
> > > > 14.04] are duplicated.
> > > >
> > > > Proposed fix is to check if there are more than 1 hashmaps, if it
> > > > is, get the entries from those maps, remove duplicate entries and
> > > > append to existing hashmap to get a consolidated map having all the
> > > > IPP attributes.
> > > >
> > > Missing webrev link:
> > > http://cr.openjdk.java.net/~psadhukhan/8170578/webrev.01/
> > > > Regards
> > > > Prasanta
> > > > On 12/21/2016 11:28 PM, Phil Race wrote:
> > > > > So now I am very suspicious.
> > > > > First (previous fix I reviewed) Job Sheets weren't supported on
> > > > > 16.10, now its PageRanges.
> > > > >
> > > > > I think this merits investigation of what is going on before we
> > > > > commit this fix.
> > > > > Why are attributes that have always been supported by CUPS
> > > > > suddenly unavailable ?
> > > > >
> > > > > And for the cases we image the pages ourselves, we can implement
> > > > > PageRanges
> > > > > internally, so for a normal PrinterJob it always can be supported
> > > > > .. regardless of
> > > > > what the printer says.
> > > > >
> > > > > -phil.
> > > > >
> > > > >
> > > > > On 12/14/2016 09:50 PM, Prasanta Sadhukhan wrote:
> > > > > >
> > > > > > Hi All,
> > > > > >
> > > > > > Please review a testbug fix for jdk9 for an issue where it it
> > > > > > seen that PageRanges option is disabled in printer dialog in
> > > > > > ubuntu16.10.
> > > > > >
> > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8170578
> > > > > >
> > > > > > webrev: http://cr.openjdk.java.net/~psadhukhan/8170578/webrev.00/
> > > > > >
> > > > > > Issue was, in ubuntu16.10 the attribute map [obtained here
> > > > > > http://hg.openjdk.java.net/jdk9/client/jdk/file/dc658d7dde90/src/java.desktop/unix/classes/sun/print/IPPPrintService.java#l1767]
> > > > > > does not have
> > > > > > "pageranges-supported" attribute returned by CUPS
> > > > > > so this category is not added to supported categories
> > > > > > [http://hg.openjdk.java.net/jdk9/client/jdk/file/7f09d558b044/src/java.desktop/unix/classes/sun/print/IPPPrintService.java#l1030]
> > > > > > so isAttributeCategorySupported() called by ServiceDialog returns
> > > > > > false
> > > > > > [http://hg.openjdk.java.net/jdk9/client/jdk/file/7f09d558b044/src/java.desktop/share/classes/sun/print/ServiceDialog.java#l1141]
> > > > > > resulting in PageRanges option getting disabled.
> > > > > >
> > > > > > Fix is to check if PageRanges category is supported in underlying
> > > > > > platform, only then proceed with the test.
> > > > > >
> > > > > > Regards
> > > > > > Prasanta
> > > > >
> > > >
> > >
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
I spent some significant amount of time trying to make sure I know
the IPP protocol<br>
that is underpinning this. Prasanta provided me with some additional
logging from<br>
which it seems that there are 3 repeated attributes.<br>
:
<br>
IPPPrintService>> readIPPResponse
pwg-raster-document-resolution-supported
<br>
IPPPrintService>> readIPPResponse
pwg-raster-document-sheet-back
<br>
IPPPrintService>> readIPPResponse
pwg-raster-document-type-supported
<br>
<br>
Furthermore I have determined that 16.10 ships with CUPS 2.2 -
released only a few weeks before 16.10<br>
Whereas 16.04 has 2.1.3. So what we have here is a major CUPS
upgrade and I am still not 100% sure<br>
if these duplicates are a CUPS bug - but I think it probable.<br>
<br>
These duplicated attributes are not important to our implementation,
so all we need to do<br>
is make sure they don't break us. I don't see a reason to care if we
keep the first or replace<br>
it with the second. Any new attributes we've previously ignored will
still be ignored.<br>
<br>
What Prasanta has implemented will fix our bug so I am approving.<br>
<br>
However our implementation looks odd to me - I do not understand why
there<br>
was an expectation a new HashMap ever needed to be created. It only
makes<br>
sense if duplicate attributes are normal. But then there is no rhyme
nor reason<br>
to then simply picking the first HashMap and ignoring the rest.<br>
If anything I'd expect that you may want to tie a HashMap to each
group tag.<br>
In which case the first one would be operational attributes, not
printer attributes.<br>
And it isn't done like that anyway. It just creates a new one "on
demand" when<br>
it sees the key is already used. This happens in the middle of
processing a particular group.<br>
So it still makes no sense.<br>
Accordingly I think we need a follow-on bug to examine, explain and
if necessary update this in 10.<br>
<br>
-phil.<br>
<br>
<br>
On 1/18/17, 3:11 AM, Prasanta Sadhukhan wrote:
<blockquote
cite="mid:65c06ba2-0e8e-3219-76bd-220fe15bb973@oracle.com"
type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<p>Ok. Updated webrev just in case we are not reading the protocol
wrongly.<br>
</p>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Epsadhukhan/8170578/webrev.02/">http://cr.openjdk.java.net/~psadhukhan/8170578/webrev.02/</a><br>
<br>
Regards<br>
Prasanta<br>
<div class="moz-cite-prefix">On 1/12/2017 8:04 AM, Philip Race
wrote:<br>
</div>
<blockquote cite="mid:5876EB50.2010305@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
I am looking at this and the hold up is I can't remember the
protocol readIPPResponse() is<br>
trying to follow. Without that understanding it is hard to say
much .. so I need to go read<br>
up and remember .. <br>
<br>
Two comments : There is an extra space in <br>
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<pre><span class="new">if (responseMap.length > 1) {</span></pre>
The test update is bogus in referring to this bug. The change is
nothing to do with the bug.<br>
And I actually prefer printer tests to throw nasty exceptions
when there are no printers<br>
and they need one ... else SQE just don't configure printers on
test systems and the tests all wrongly pass.<br>
Would you run Linux UI tests on a blade with no X-server running
?<br>
<br>
-phil.<br>
<br>
On 1/10/17, 1:14 AM, Prasanta Sadhukhan wrote:
<blockquote
cite="mid:68d7b1d5-40f9-0841-0d6c-328861391f33@oracle.com"
type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<p><br>
</p>
<br>
<div class="moz-cite-prefix">On 1/10/2017 2:43 PM, Prasanta
Sadhukhan wrote:<br>
</div>
<blockquote
cite="mid:224e283e-47c5-f254-bafd-de5beccf1584@oracle.com"
type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Actually, in ubuntu16.10 attribute map did not have
"page-ranges-supported" attribute because 2 attribute
hashmap is created in <span id="l1757">IPPPrintService#readIPPResponse()
and most of the supported attributes are part of the 2nd
hashmap (responseMap[1])<br>
whereas only the 1st hashmap is utilised through
responseMap[0] [<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://hg.openjdk.java.net/jdk9/client/jdk/file/8be0bb1aa238/src/java.desktop/un \
ix/classes/sun/print/IPPPrintService.java#l1760">http://hg.openjdk.java.net/jdk9/clien \
t/jdk/file/8be0bb1aa238/src/java.desktop/unix/classes/sun/print/IPPPrintService.java#l1760</a>]<br>
</span><br>
As to why 2 hashmaps are created in ubuntu16.10, it is
because readIPPResponse() checks if key is already present
in existing map, then create a new hashmap through this <a
moz-do-not-send="true"
href="http://hg.openjdk.java.net/jdk9/client/jdk/file/8be0bb1aa238/src/java.desktop/unix/classes/sun/print/IPPPrintService.java#l1887">code</a><br>
and in ubuntu16.10, some attributes (like
pwg-raster-document-type-supported which is not there in
ubuntu 14.04] are duplicated.<br>
<br>
Proposed fix is to check if there are more than 1 hashmaps,
if it is, get the entries from those maps, remove duplicate
entries and append to existing hashmap to get a consolidated
map having all the IPP attributes.<br>
<br>
</blockquote>
Missing webrev link: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Epsadhukhan/8170578/webrev.01/">http://cr.openjdk.java.net/~psadhukhan/8170578/webrev.01/</a><br>
<blockquote
cite="mid:224e283e-47c5-f254-bafd-de5beccf1584@oracle.com"
type="cite"> Regards<br>
Prasanta <br>
<div class="moz-cite-prefix">On 12/21/2016 11:28 PM, Phil
Race wrote:<br>
</div>
<blockquote
cite="mid:90c7f438-27ee-3334-d91a-5b1aff84e025@oracle.com"
type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
So now I am very suspicious.<br>
First (previous fix I reviewed) Job Sheets weren't
supported on 16.10, now its PageRanges.<br>
<br>
I think this merits investigation of what is going on
before we commit this fix.<br>
Why are attributes that have always been supported by CUPS
suddenly unavailable ?<br>
<br>
And for the cases we image the pages ourselves, we can
implement PageRanges<br>
internally, so for a normal PrinterJob it always can be
supported .. regardless of<br>
what the printer says.<br>
<br>
-phil.<br>
<br>
<br>
<div class="moz-cite-prefix">On 12/14/2016 09:50 PM,
Prasanta Sadhukhan wrote:<br>
</div>
<blockquote
cite="mid:39f21e6d-78fb-6486-0260-620f42a63ccb@oracle.com"
type="cite">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<p>Hi All,</p>
<p>Please review a testbug fix for jdk9 for an issue
where it it seen that PageRanges option is disabled in
printer dialog in ubuntu16.10.</p>
<p>Bug: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8170578">https://bugs.openjdk.java.net/browse/JDK-8170578</a></p>
<p>webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Epsadhukhan/8170578/webrev.00/">http://cr.openjdk.java.net/~psadhukhan/8170578/webrev.00/</a></p>
Issue was, in ubuntu16.10 the attribute map [obtained
here <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://hg.openjdk.java.net/jdk9/client/jdk/file/dc658d7dde90/src/java.desktop/un \
ix/classes/sun/print/IPPPrintService.java#l1767">http://hg.openjdk.java.net/jdk9/clien \
t/jdk/file/dc658d7dde90/src/java.desktop/unix/classes/sun/print/IPPPrintService.java#l1767</a>]<br>
does not have<br>
"pageranges-supported" attribute returned by CUPS<br>
so this category is not added to supported categories<br>
[<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://hg.openjdk.java.net/jdk9/client/jdk/file/7f09d558b044/src/java.desktop/un \
ix/classes/sun/print/IPPPrintService.java#l1030">http://hg.openjdk.java.net/jdk9/clien \
t/jdk/file/7f09d558b044/src/java.desktop/unix/classes/sun/print/IPPPrintService.java#l1030</a>]<br>
so isAttributeCategorySupported() called by
ServiceDialog returns false<br>
[<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://hg.openjdk.java.net/jdk9/client/jdk/file/7f09d558b044/src/java.desktop/sh \
are/classes/sun/print/ServiceDialog.java#l2763">http://hg.openjdk.java.net/jdk9/client \
/jdk/file/7f09d558b044/src/java.desktop/share/classes/sun/print/ServiceDialog.java#l1141</a>]<br>
resulting in PageRanges option getting disabled.<br>
<br>
Fix is to check if PageRanges category is supported in
underlying platform, only then proceed with the test.<br>
<br>
Regards<br>
Prasanta<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic