<AWT Dev> [9] Review Request: 6815345 java.awt.Component.createImage(int width, int height) should remove behavioral optionality
Semyon Sadetsky
semyon.sadetsky at oracle.com
Fri Sep 25 17:13:44 UTC 2015
On 9/25/2015 7:06 PM, Sergey Bylokhov wrote:
> On 25.09.15 18:38, Semyon Sadetsky wrote:
>>
>>
>> On 9/25/2015 4:51 PM, Sergey Bylokhov wrote:
>>> On 25.09.15 15:44, Semyon Sadetsky wrote:
>>>>
>>>>
>>>> On 9/25/2015 2:46 PM, Sergey Bylokhov wrote:
>>>>> On 25.09.15 10:13, Semyon Sadetsky wrote:
>>>>>> Native container based components are disabled. But other native
>>>>>> resources can be used if available.
>>>>>
>>>>> no hw components -> no backbuffers for them.
>>>> In my understanding it depends on the component. If component
>>>> capable to
>>>> work in the headless mode it can continue to use its peer, can't it?
>>>>>
>>>>>> All **Peer interfaces are public and external. Peer is not a
>>>>>> hidden or
>>>>>> implementation specific term. It isn't allowed to application
>>>>>> developers
>>>>>> for implementation, but it is used for porting client libs to other
>>>>>> platforms. Java specification has wider vision than one particular
>>>>>> implementation. And for porting the headless mode is very sensitive
>>>>>> topic because specific platforms are headless often.
>>>>>
>>>>> The java.awt.peer package is private and should be used by the awt
>>>>> developers only. If the new port will be implemented then it should
>>>>> follow the same specification as our implementation.
>>>
>>>> That's correct. And our specification should be reasonable in its
>>>> turn.
>>>> It must not be simply back-filled from our code otherwise it would
>>>> unlikely have more than one implementation.
>>>> JCK team likes direct specs in "if-then" style. But I see that the
>>>> initial doc was soft enough because it just clarify that exceptions
>>>> from
>>>> the general rule are possible depending on the specific implementation
>>>> concept to leave more possibilities for porting.
>>>
>>> But it was not strict enough, we always return null for a long time,
>>> all other ports do the same because jck require this. So this is not a
>>> problem for possible ports and this removes possible misleading
>>> assumption for application developers. This fix is a spec
>>> clarification that an application should not try to create a buffer
>>> for the hw component in such situations.
>>>
>>> The article I send you
>>>> also contains a lot of soft phrases.
>>>>>
>>>>>>>
>>>>>>> Maybe
>>>>>>>> Component#getImage() implementation should simply check for
>>>>>>>> isHeadless()
>>>>>>>> and return null?
>>>>>> But what will happen if Unix DISPLAY variable is not set so
>>>>>> isHeadlessInstance()=true? Will peer be allowed? Should component
>>>>>> buffering work smoothly?
>>>>>
>>>>> The peers are not allowed in the headless mode. The DISPLAY var is
>>>>> not
>>>>> specified in the javadoc, this means that some implementation can
>>>>> work
>>>>> when DISPLAY is empty/unset, but in this case they must return false
>>>>> from the isHeadlessXX();
>>>> isHeadlessInstance() just tests DISPLAY on Linux.
>>>
>>> This is our implementation, other can check other things. There are
>>> some implementation that can work without display, but in this case
>>> this environment is not headless.
>>>
>>> Can you specify what sentence you complain?
>>> This cannot be removed since it was there already
>>> "This will always happen if
>>> * <code>GraphicsEnvironment.isHeadless()</code> returns
>>> * <code>true</code>."
>>
>> "The return value may be null if the component is not displayable. This
>> will always happen if GraphicsEnvironment.isHeadless() returns true."
>>
>> your new version:
>>
>> "Returns null value if the component is not displayable or
>> GraphicsEnvironment.isHeadless() returns true"
>>
>> which invited me as a reviewer to find an "if" in the method body or
>> deeper, because it sounds "It shall return null if ..." .
>> But I didn't find it. Imagine if you get this spec to be implemented
>> from scratch. Why 'if' is not added as the first line? That would
>> eliminate all questions.
>
> Is this check in the code is not clear?
> (peer != null) ? peer.createImage(width, height) : null
It is not obvious. It should check if(GraphicsEnvironment.isHeadless())
{return null;}. Why to execute anything else?
Look on the similar spec and its implementation:
* @exception HeadlessException if GraphicsEnvironment.isHeadless()
* returns true
* @see java.awt.GraphicsEnvironment#isHeadless
*/
public Button(String label) throws HeadlessException {
GraphicsEnvironment.checkHeadless();
this.label = label;
}
>
>>
>> The previous version doesn't demand "if" because it is indirect and as
>> developer I would never rely on the null value promised by such spec.
>> But with the new version of the spec the null should be guaranteed for
>> the specified conditions otherwise we receive a bug.
>
> And if an application, which will tested on your code, will start on
> our jdk it will fail with npe.
>
No, its reversed. I meant "relay on null value". That means any logic
that expects null value only at this conditions may fail.
>>
>> And I cannot see the reason why non-null value cannot be allowed because
>> it doesn't brake anything.
>> Explanation "we define this and developers should follow" is a bit
>> frustrating because I see scenarios when component buffer could be
>> useful in the headless mode.
>
> This is not about a headless.
Why? I provided you reference to the article in which states opposite.
Peer#createVolatileImage() may be used in headless.
For example, a network server without GUI can render images and send
them to client via network or to printer. Why it cannot use its
available hardware for image rendering?
Maybe there are some obstacles for that which I don't know. I'd
appreciate it if you provide them.
>
>>
>> I suspect the original spec didn't imply strict binding to the values,
>> and this fix may simplify testing but distorts the original approach.
>
> It is always good to specify strict and clear null behavior to
> suppress possible surprises.
>
>> I just unsure that this issue should be resolved. Maybe other reviewers
>> could add their opinion.
>>>
>>>
>>>>>
>>>>>> Note this is a common network server scenario which runs Java app
>>>>>> from
>>>>>> the text console. I cannot get from the spec is it completely
>>>>>> different
>>>>>> headless mode or it is the same as the global headless?
>>>>>>>
>>>>>>>>
>>>>>>>> And more global question: Why should we disable the creating of
>>>>>>>> component image buffer for the headless mode?It could be used for
>>>>>>>> the
>>>>>>>> same performance reason as in non-headless.
>>>>>>>
>>>>>>> in the headless mode we cannot create most of the hw components,
>>>>>>> actually non of top level window can be created. And for lw
>>>>>>> components
>>>>>>> like in swing the one global buffer is used in the RepaintManager.
>>>>>> One global buffer means one global repaint
>>>>>
>>>>> No this is not true. If one component is changed then only one
>>>>> component is repainted.
>>>>>
>>>>> off all components for any
>>>>>> single component change. Why can't we be isomorphic here?
>>>>>>>
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> The headless case is covered, heavyweight components cannot be
>>>>>>>>>>> created
>>>>>>>>>>> in such mode, so only lightweight buttons are checked.
>>>>>>>>>>>
>>>>>>>>>>> On 24.09.15 15:58, Semyon Sadetsky wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 9/24/2015 3:25 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>>> On 24.09.15 11:36, Semyon Sadetsky wrote:
>>>>>>>>>>>>>> Hi Sergey,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> isHeadless()=true must return null. If so please add the
>>>>>>>>>>>>>> corresponding
>>>>>>>>>>>>>> test case. It is not not obvious from the code.
>>>>>>>>>>>>>
>>>>>>>>>>>>> isHeadless()=true is headless mode where the frames are
>>>>>>>>>>>>> always
>>>>>>>>>>>>> not
>>>>>>>>>>>>> displayable, so everything is similar to the current test
>>>>>>>>>>>>> except
>>>>>>>>>>>>> that
>>>>>>>>>>>>> in headless the pack() will be throw an exception and second
>>>>>>>>>>>>> part of
>>>>>>>>>>>>> the test in this mode is unnecessary.
>>>>>>>>>>>> Then maybe simply do not call pack() for the headless test?
>>>>>>>>>>>> Okay, let me rephrase what I meant. Since isHeadless()=true
>>>>>>>>>>>> case is
>>>>>>>>>>>> mentioned in those 3 specs so explicitly it must be guarantied
>>>>>>>>>>>> that
>>>>>>>>>>>> the
>>>>>>>>>>>> specified behavior works for the case as described. I cannot
>>>>>>>>>>>> trace
>>>>>>>>>>>> the
>>>>>>>>>>>> result by reading the code of the createImage(), isHeadless()
>>>>>>>>>>>> method is
>>>>>>>>>>>> not even called there. So the test case should be added.
>>>>>>>>>>>> Or you
>>>>>>>>>>>> could
>>>>>>>>>>>> remove isHeadless() references from the specs. Or write
>>>>>>>>>>>> something
>>>>>>>>>>>> like
>>>>>>>>>>>> "the result is non-deterministic if isHeadless() is not
>>>>>>>>>>>> false..."
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --Semyon
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 9/23/2015 9:14 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>>>>> Hello.
>>>>>>>>>>>>>>> Please review the fix for jdk9.
>>>>>>>>>>>>>>> The specification is updated as suggested in JDK-6186530
>>>>>>>>>>>>>>> and
>>>>>>>>>>>>>>> JDK-6815345. The test is added to prove that we always
>>>>>>>>>>>>>>> return
>>>>>>>>>>>>>>> null
>>>>>>>>>>>>>>> when the component is not displayable.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> ccc request will be created after the technical review. One
>>>>>>>>>>>>>>> additional
>>>>>>>>>>>>>>> bug filed https://bugs.openjdk.java.net/browse/JDK-8137047
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-6815345
>>>>>>>>>>>>>>> Webrev can be found at:
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~serb/6815345/webrev.04
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20150925/c4ec55bd/attachment-0001.html>
More information about the awt-dev
mailing list