<AWT Dev> 7171412: awt Choice doesn't fire ItemStateChange when selecting item after select() call

[Artem Ananiev]

Hi, Tim,

see my comments below.

On 7/25/2012 1:31 AM, Tim English wrote:
> newb here.. Trying to follow procedure and run this by awt-dev before I
> dive into this.
> I ran into 7171412, produced a similar test case to the one in the bug
> report, and then found the existing bug report.
> I consider this High priority since any GUI that uses an awt Choice
> could be showing the end user the wrong information
> if the GUI is counting on the ItemListener for correct state.

I don't say counting on ItemEvents is wrong, but application developers 
should be careful here. And the problem is with missing events on 
Choice.select() calls.

> The code in AwtChoice::WmNotify looks like it is trying to prevent
> notifications if the user keeps selecting the same item over and over.

Correct.

> I came across a bug/feature request along those lines when searching for
> 7171412.
> Possible Fix 1 update m_selectedItem when select(int) called
> void AwtChoice::_Select(void *param)
> {
> JNIEnv *env = (JNIEnv *)JNU_GetEnv(jvm, JNI_VERSION_1_2);
> SelectStruct *ss = (SelectStruct *)param;
> jobject choice = ss->choice;
> jint index = ss->index;
> AwtChoice *c = NULL;
> PDATA pData;
> JNI_CHECK_PEER_GOTO(choice, done);
> c = (AwtChoice *)pData;
> if (::IsWindow(c->GetHWnd()))
> {
> c->SendMessage(CB_SETCURSEL, index);
> + m_selectedItem = index;
> // c->VerifyState();
> }
> done:
> env->DeleteGlobalRef(choice);
> delete ss;
> }

This fix #1 looks fine.

> Possible Fix 2 Eliminate m_selectedItem and replace it with java object
> value
> Only notify java if java's selected index does not match the Windows
> control selected index
> This eliminates the m_selectedItem so we only have 2 copies of the state
> instead of 3.
> It is probably more efficient to hit the java field, directly, but I
> have not researched jfieldID.
> This is just a first stab.
> MsgRouting AwtChoice::WmNotify(UINT notifyCode)
> {
> if (notifyCode == CBN_SELCHANGE) {
> int selectedItem = (int)SendMessage(CB_GETCURSEL);
> + // tim_english: not sure if it is legal to call from here
> + JNIEnv *env = (JNIEnv *)JNU_GetEnv(jvm, JNI_VERSION_1_2);
> + jobject target = GetTarget(env);
> + int javaIndex = JNU_CallMethodByName(env, NULL, target,
> "getSelectedIndex", "()I").i;
> + env->DeleteLocalRef(target);
>
> + if (selectedItem != CB_ERR && javaIndex != selectedItem){
> - if (selectedItem != CB_ERR && m_selectedItem != selectedItem){
> - m_selectedItem = selectedItem;
> DoCallback("handleAction", "(I)V", selectedItem);
> }
> } else if (notifyCode == CBN_DROPDOWN) {

Reading the field value directly in WM_NOTIFY handler is also possible, 
but #1 seems more simple to me.

> http://hg.openjdk.java.net/jdk8/awt/jdk/rev/3a2355dcef13
>
> http://hg.openjdk.java.net/jdk7u/jdk7u6/jdk/rev/3a2355dcef13
>
> Any advice is appreciated..
> Tim

Since you're not OpenJDK Committer, you'll need to find a sponsor to 
push your fix to the workspace. Oleg or I can do this, if you don't mind.

The fix for JDK8 don't require any additional approvals, but 7u8 process 
is more complex: explicit request to include a fix to 7u must be sent to 
jdk7u-dev alias [1]. But first of all, the fix should be pushed to JDK8 
(see Rule 1 at [2]), so let's start with JDK8 and then proceed to 7u8.

[1] http://openjdk.java.net/projects/jdk7u/approval-template.html
[2] http://openjdk.java.net/projects/jdk7u/groundrules.html

Thanks,

Artem