jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1516145 - /jmeter/trunk/src/core/org/apache/jmeter/testbeans/gui/ComboStringEditor.java
Date Wed, 21 Aug 2013 19:14:00 GMT
On 21 August 2013 19:11, sebb <sebbaz@gmail.com> wrote:
> On 21 August 2013 16:10, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
>> On Wed, Aug 21, 2013 at 3:43 PM, sebb <sebbaz@gmail.com> wrote:
>>
>>> On 21 August 2013 13:29,  <pmouawad@apache.org> wrote:
>>> > Author: pmouawad
>>> > Date: Wed Aug 21 12:29:52 2013
>>> > New Revision: 1516145
>>> >
>>> > URL: http://svn.apache.org/r1516145
>>> > Log:
>>> > Bug 55459 - Elements using ComboStringEditor lose the input value if
>>> user selects another Test Element
>>> > Rollback change using Editor to access value
>>> > Replace requestFocus() by requestFocusInWindow()
>>> > Bugzilla Id: 55459
>>> >
>>> > Modified:
>>> >
>>> jmeter/trunk/src/core/org/apache/jmeter/testbeans/gui/ComboStringEditor.java
>>> >
>>> > Modified:
>>> jmeter/trunk/src/core/org/apache/jmeter/testbeans/gui/ComboStringEditor.java
>>> > URL:
>>> http://svn.apache.org/viewvc/jmeter/trunk/src/core/org/apache/jmeter/testbeans/gui/ComboStringEditor.java?rev=1516145&r1=1516144&r2=1516145&view=diff
>>> >
>>> ==============================================================================
>>> > ---
>>> jmeter/trunk/src/core/org/apache/jmeter/testbeans/gui/ComboStringEditor.java
>>> (original)
>>> > +++
>>> jmeter/trunk/src/core/org/apache/jmeter/testbeans/gui/ComboStringEditor.java
>>> Wed Aug 21 12:29:52 2013
>>> > @@ -183,11 +183,7 @@ class ComboStringEditor extends Property
>>> >              return tags[item-minTagIndex];
>>> >          }
>>> >          // Not a tag entry, return the original value
>>> > -        // combo.getSelectedItem() javadocs says:
>>> > -        // If the combo box is editable,  then this value may not have
>>> been added to the combo box
>>> > -        // with addItem, insertItemAt or the data constructors.
>>> > -        JTextComponent textField = (JTextComponent)
>>> combo.getEditor().getEditorComponent();
>>> > -        return textField.getText();
>>> > +        return (String) value;
>>> >      }
>>> >
>>> >      /**
>>> > @@ -241,7 +237,7 @@ class ComboStringEditor extends Property
>>> >
>>> >          combo.setEditable(true);
>>> >
>>> > -        textField.requestFocus();
>>> > +        textField.requestFocusInWindow();
>>>
>>> I see that requestFocus() is discouraged as it is platform dependent.
>>> Code should call requestFocusInWindow() instead.
>>>
>> I had already done the changes locally, will commit this evening.
>>
>>> We should probably change all the other occurrences; I'll raise a bug.
>>>
>>> Agree
>>
>>> This does not fix the issue; but adding requestFocusInWindow() to
>>> JMeterTreeListener seems to have done the trick.
>>>
>>
>> Well frankly, I reviewed the changes made by the bug we think introduced
>> the regression. I think it requires a double check.
>> I don't see what could have introduced this. I wonder if it was not working
>> fine by some sort of chance.
>
> I suppose it could have been a side effect of how the code was organised.
> I suspect it was coded correctly, but just not documented, so a subtle
> change to the code caused it to break.
>
>> But as to root explanation of why focusLost was not called before
>> valueChanged, as we say in french "je donne ma langue au chat " :-).
>
> We have "Has the cat got your tongue?" but that has a different
> meaning ("why are you (unexpectedly) silent?")
>
>> With current commit, we force the focus on tree, and by consequence
>> focusLost on all components of TestElement GUI, so it seems clean to me.
>
> Yes, I think it is a much better solution, and I think we can proceed
> on that basis.
>
> However it would still be useful to know why the behaviour changed.
> Just in case there is some other subtle bug lurking.
>
> I hope to try and find out which commit broke it, and see if that
> helps our understanding.

It was definitely the commit associated with Bug 55103

However, that is not an easy change to analyse.

>>
>>>
>>> >          String text = translate(initialEditValue);
>>> >          if (text == null) {
>>> >              text = ""; // will revert to last valid value if invalid
>>> >
>>> >
>>>
>>
>>
>>
>> --
>> Cordialement.
>> Philippe Mouawad.

Mime
View raw message