Hi Phil,
Thanks for the guiding over discussion,
Please take a look at the updated webrev :
http://cr.openjdk.java.net/~arapte/8198000/webrev.05/
Regards,
Ambarish
*From:*Ambarish Rapte
*Sent:* Wednesday, November 07, 2018 10:49 PM
*To:* Philip Race <philip.r...@oracle.com>
*Cc:* awt-dev@openjdk.java.net
*Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 :
java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert
on Windows
Hi Phil,
> I don't think so. I already pointed out that it is apparently
returning -1 in your test
> when HIWORD is 1. I am pretty sure it isn't expecting you to
interpret 0xFFFF stored
> in 16 bits as positive.
The value 0xFFFF is -1 when saved into a signed short and 65535 when
assigned to unsigned short.
I was trying to find any supporting documentation or source code from
MSDN. But cannot be certain of the behavior based on documentation.
So I also think that we can go ahead with the earlier version of fix
as you suggested. : http://cr.openjdk.java.net/~arapte/8198000/webrev.04/
Please take a look at the fix, The webrev has fix only for JDK-8198000.
The fix change for issue #2 i.e. cmath::pow function is removed from
webrev.04
> These are platform UI components. They can behave differently w.r.t
things like
> how they respond to mouse clicks, all dependent on what the platform
norms are.
> I see no reason to go out of our way to "make windows mouse clicks
behave like mac".
This looks good to me as well. With the changes in webrev.04, an item
gets selected only when mouse is clicked on the item.
> Not understanding you. How can you tell if 0x0001 is supposed to be
"1" or "65536" ?
Sorry for not explaining the fix in earlier email with webrev.03.
I have removed this change from the webrev.
I should not have mixed fix of the two issues. If the issue #2 needs
to be addressed, I request if we can work on this issue separately.
> So if the user clicks outside we do nothing. What is wrong with that ?
I too see nothing wrong in this behavior. Mouse click outside the
client area can be neglected under this fix.
Please take a look at the updated webrev.04 which includes fix only
for JDK-8198000
Regards,
Ambarish
*From:*Philip Race
*Sent:* Tuesday, November 06, 2018 10:21 PM
*To:* Ambarish Rapte <ambarish.ra...@oracle.com
<mailto:ambarish.ra...@oracle.com>>
*Cc:* awt-dev@openjdk.java.net <mailto:awt-dev@openjdk.java.net>
*Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 :
java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug assert
on Windows
On 11/5/18 8:51 AM, Ambarish Rapte wrote:
Hi Phil,
The little abstraction or lack of clarity in documentation is
leading us to make some assumptions.
Just trying to connect the pieces,
I referred these two doc pages to learn LOWORD behavior:
1. LOWORD :
https://msdn.microsoft.com/en-us/library/windows/desktop/ms632659(v=vs.85).aspx
<https://msdn.microsoft.com/en-us/library/windows/desktop/ms632659%28v=vs.85%29.aspx>
2. WORD :
https://docs.microsoft.com/en-us/windows/desktop/winprog/windows-data-types
And the documentation of LB_ITEMFROMPOINT does not mention LB_ERR(-1)
Based on these documentation, it looks safe to conclude that
LOWORD would be positive.
I don't think so. I already pointed out that it is apparently
returning -1 in your test
when HIWORD is 1. I am pretty sure it isn't expecting you to interpret
0xFFFF stored
in 16 bits as positive.
Issue #1 :
Yes, the HIWORD is 1, when the mouse is clicked inside the list’s
client area but not on the item.
Once we separate the HIWORD and LOWORD, LOWORD gives correct index
of the last item, and this issue gets solved.
Checking if HIWORD is 1 can be additional check. But as we extract
correct index, this check can be ignored for allowing selection.
Or the other way, we can use HIWORD to avoid selection/deselection
when it is 1.
Additionally, The behavior on Ubuntu and mac is different,
Ubuntu: Last item does not get selected & it also does not select
any other item. User must click on the item to select or deselect.
Mac: Selects the last item
Windows: Focused item gets selected / deselected on multiple
clicks, 3-4
* After this fix[Allowing selection], Mac and Windows will have
same behavior.
These are platform UI components. They can behave differently w.r.t
things like
how they respond to mouse clicks, all dependent on what the platform
norms are.
I see no reason to go out of our way to "make windows mouse clicks
behave like mac".
*
Issue #2 :
This looks a very corner case of list of size more than 65535
size, but the item selection and deselection works fine with keyboard.
Also List behaves correctly on Ubuntu and Mac.
Just for a look, I am including a fix for this issue in webrev.03:
http://cr.openjdk.java.net/~arapte/8198000/webrev.03/
<http://cr.openjdk.java.net/%7Earapte/8198000/webrev.03/>
As you have already mentioned we should not really change a long
running behavior, but I could not hold on from suggestion.
If you think this fix is not needed, I shall just change/remove it.
Not understanding you. How can you tell if 0x0001 is supposed to be
"1" or "65536" ?
I also don't like std::pow .. it is unnecessary here.
Not withstanding the lack of clarity on how you are claiming to know that,
I was expecting a fix to look more like
LONG count = GetCount();
if (count > 0) {
LONG item = static_cast<LONG>(SendListMessage(LB_ITEMFROMPOINT, 0,
msg->lParam));
if (HIWORD(item) == 0) {
item = LOWORD(item);
if (item > 0 && item < GetCount()) {
...
So if the user clicks outside we do nothing. What is wrong with that ?
-phil.
Regards,
Ambarish
*From:*Philip Race
*Sent:* Saturday, November 03, 2018 10:59 PM
*To:* Ambarish Rapte <ambarish.ra...@oracle.com>
<mailto:ambarish.ra...@oracle.com>
*Cc:* awt-dev@openjdk.java.net <mailto:awt-dev@openjdk.java.net>
*Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 :
java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug
assert on Windows
- I am not sure you can assume the LOWORD value is non-negative.
It seems to me that the 0XFFFF we got back is meant to be
interpreted as "-1"
which is what I wrote below.
I do note that it appears that in issue #2 you are seeing that
up to 65534 items
may be allowed and it wraps if you add more than 65535 ..
So *perhaps* we can interpret 0xFFFF as meaning positive 65535
but I think
*only* if HIWORD is "0".
- Issue #1 : I am not sure that a "1" in HIWORD automatically
means it is off the
end of the list ... just "outside the client area". I think if
HIWORD is 1 we just
bail don't we ?
Then you don't have to worry about whether 0xFFFF meant -1 or 65536
- Issue #2 : There is always *some* limit in cases like this.
32767 (2^15-1) or 65535 (2^16-1)
are very typical in these platform APIs. Often the platform
doesn't explicitly document it
and you have to infer it from the data type. I think it was all
very moot when these APIs
were designed because you'd run out of memory before you could get
that many items :-)
I'd be surprised if there were not already some open bug pointing
out that we accept "int"
for index and don't have any feedback when exceeding platform limits.
From a compatibility point of view, I don't think it is worth
doing anything that would
break ancient applications to provide that feedback.
-phil.
On 11/2/18, 10:23 AM, Ambarish Rapte wrote:
Hi Phil,
Thanks for guiding with the documentation.
The fix is modified after a relook at the documentation, and
observed two issues [mentioned below].
webrev: http://cr.openjdk.java.net/~arapte/8198000/webrev.02
<http://cr.openjdk.java.net/%7Earapte/8198000/webrev.02>
Fix Justification:
LB_ITEMFROMPOINT
documentation(https://docs.microsoft.com/en-us/windows/desktop/controls/lb-itemfrompoint):
1. Message LB_ITEMFROMPOINT returns a LONG value.
2. There is no mention of LB_ERR as return value, like it is
mentioned clearly for some of the other messages as
LB_GETCOUNT, LB_SETITEMHEIGHT, LB_GETTEXT, LB_GETCURSEL.
* So the existing comparison against LB_ERR is incorrect.
3. The two parts LOWORD and HIWORD of return value are of
type WORD(unsigned short), so the return value can never
be negative.
* It is another reason to not to compare the returned
index value with LB_ERR which is defined as (-1).
Fix:
Extract the index from LOWORD into a WORD variable and
verify only if the index is smaller than the list size.
(webrev.02)
This fixes both JDK-8198000 & below mentioned Issue 1.
2 other Issues:
Issue 1: NON selection of an item.
1. Currently the returned LONG value is used without
extracting the LOWORD.
2. As far as the HIWORD is 0, the LONG return value would be
same as index of item(LOWORD).
3. But when HIWORD is 1, the LONG return value would be a
large unexpected value. If it is used as an index, then it
would result in NON selection of the item.
4. Test program:
http://cr.openjdk.java.net/~arapte/8198000/ListSize3.java
<http://cr.openjdk.java.net/%7Earapte/8198000/ListSize3.java>
5. Steps:
1. Compile and execute the test program with release
build JDK.
2. Click in the list's client area below the last item,
i.e. do not click on any item.
6. Expected behavior: Last item should get selected.
7. Actual behavior: Last item does not get selected on first
click. But the focused item gets selected after few clicks.
Issue 2: Incorrect selection when list size exceeds
sizeof(WORD) [0xFFFF].
Steps:
1. Compile and execute the program with release build JDK
(with or without the fix) :
http://cr.openjdk.java.net/~arapte/8198000/List65544.java
<http://cr.openjdk.java.net/%7Earapte/8198000/List65544.java>
2. Click the first item in list, press End key.
3. Click the last item 65544.
4. Expected behavior: The item 65544 should get selected.
5. Actual behavior: The item 65544 does not get selected and
instead item 8 gets selected.
6. Verified only windows behavior.
7. I suggest to file a new JBS for this issue.
Regards,
Ambarish
*From:*Phil Race
*Sent:* Thursday, November 01, 2018 1:51 AM
*To:* Ambarish Rapte <ambarish.ra...@oracle.com>
<mailto:ambarish.ra...@oracle.com>; awt-dev@openjdk.java.net
<mailto:awt-dev@openjdk.java.net>
*Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 :
java/awt/List/EmptyListEventTest/EmptyListEventTest.java debug
assert on Windows
That adds what I suggested, but I had also suggested you leave
in what you
had added as it also adds some protection.
Additionally I read the MS docs and they do explain the 131071
return value.
The message this code is sending is LB_ITEMFROMPOINT and the
docs say here
https://docs.microsoft.com/en-us/windows/desktop/controls/lb-itemfrompoint
that
>The return value contains the index of the nearest item in
the *LOWORD*
<https://msdn.microsoft.com/library/windows/desktop/ms632659>.
The *HIWORD*
<https://msdn.microsoft.com/library/windows/desktop/ms632657>
is zero if the > specified point is in the client area of the
list box, or one if it is outside the client area"
You got 131071 which is, in hex 0X1FFFF.
So you got "1" for hi-word, meaning "outside client area" and
"-1" for loword,
meaning the index. And a return index of "-1" doubtless means
an error ..
-phil.
On 10/31/18 12:49 PM, Ambarish Rapte wrote:
Hi Phil & Sergey,
This issue was introduced with the fix for JDK-6806217
<https://bugs.openjdk.java.net/browse/JDK-6806217>, in
7b55, which modified AwtList::HandleEvent(), so It was not
observed with JDK6.
Please review the updated change as discussed offline:
http://cr.openjdk.java.net/~arapte/8198000/webrev.01/
<http://cr.openjdk.java.net/%7Earapte/8198000/webrev.01/>
Regards,
Ambarish
*From:*Phil Race
*Sent:* Tuesday, October 30, 2018 2:09 AM
*To:* Ambarish Rapte <ambarish.ra...@oracle.com>
<mailto:ambarish.ra...@oracle.com>;
awt-dev@openjdk.java.net <mailto:awt-dev@openjdk.java.net>
*Subject:* Re: <AWT Dev> [12] RFR : JDK-8198000 :
java/awt/List/EmptyListEventTest/EmptyListEventTest.java
debug assert on Windows
On 10/29/18 7:31 AM, Ambarish Rapte wrote:
Hi,
Please review this Windows only fix,
JBS:
https://bugs.openjdk.java.net/browse/JDK-8198000
Webrev:
http://cr.openjdk.java.net/~arapte/8198000/webrev.00/
<http://cr.openjdk.java.net/%7Earapte/8198000/webrev.00/>
Issue:
1. Test asserts with debug build jdk, only on windows.
Only the debug build turns on asserts.
But I think JDK 6 always turned on asserts, and this test
was introduced in 6,
so something must have changed else we'd have seen this
test fail a long time ago.
Can you identify what it was ?
Also same comment as the other bug - you need to add the
bug id to the test.
2.
3. Assert at Line no 77, awt_List.h :: IsItemSelected()
4. awt_List.cpp :: AwtList::HandleEvent() calls
IsItemSelected() with an incorrect index value.
Why ?
5.
6. In AwtList::HandleEvent() , the call
SendListMessage(LB_ITEMFROMPOINT, 0, msg->lParam)
returns an arbitrary value 131071, which gets
passed to IsItemSelected().
7. Could not find any relevance to the value 131071,
from LB_ITEMFROMPOINT doc.
That is (128*1024)-1, so it is probably not "arbitrary".
Please add the eval above to the bug report .. once we
have a complete understanding.
-phil.
8.
Fix: Index should be verified before making call to
IsItemSelected() :
http://cr.openjdk.java.net/~arapte/8198000/webrev.00/
<http://cr.openjdk.java.net/%7Earapte/8198000/webrev.00/>
Verification: All list tests pass.
Regards,
Ambarish
