Bug 332992

Comment 1 John Cortell 2010-12-20 23:23:18 EST

Created attachment 185610 [details]
Fix

I looked at adding a test but I ran into issues exercising the tests. If someone can help me wit that, I'll be happy to add a testcase for this.

Comment 2 Ketan Padegaonkar 2010-12-21 13:47:38 EST

What sort of errors do you see. What OS are you on?

Comment 3 John Cortell 2010-12-21 14:04:54 EST

? The defficiency is that there simply is no SWTBot.scale[Xxxxx](methods), as there is, e.g., SWTBot.slider[Xxxxxx] ones. I wanted to test GUI that uses Scale widgets and was unable to do so until I added the code I've attached.

Comment 4 John Cortell 2010-12-21 14:05:17 EST

I'm on Windows XP

Comment 5 Ketan Padegaonkar 2010-12-26 01:25:50 EST

John,

Your code looks good to me.

Some feedback and the patch is ready to be merged:

* Change the methods get/setSelection to get/setValue -- makes it look more readable
* Add tests, send a note to the newsgroup or mailing list if you need help

Comment 6 Ketan Padegaonkar 2010-12-26 01:27:23 EST

Also I noticed that the copyright header mentions a name that is not yours. Please fix or clarify whatever the case may be.

Comment 7 John Cortell 2011-01-03 10:33:54 EST

OK. Will do.

Comment 8 John Cortell 2011-01-13 18:20:53 EST

(In reply to comment #5)
> John,
> 
> Your code looks good to me.
> 
> Some feedback and the patch is ready to be merged:
> 
> * Change the methods get/setSelection to get/setValue -- makes it look more
> readable
> * Add tests, send a note to the newsgroup or mailing list if you need help

I emailed the list and have gotten no response (on the tests). I'd like to close out on this issue but I need assistance getting the tests to run.

Comment 9 John Cortell 2011-01-28 14:29:30 EST

Created attachment 187870 [details]
Fiix

Updated patch. Added junit test and had the generator plugin make the SWTBot.java changes (previous one was hand-tweaked). I didnt' change the names of get/setSelection() because I think the key is consistency. The Scale and Slider widgets are very similar not only in function but in API. SWTBotSlider has get/setSelection(), and not get/setValue(). It only makes sense for SWTBotScale to have the same. Not to mention, the SWT widget uses 'selection' and not 'value'.

Comment 10 John Cortell 2011-01-28 14:32:43 EST

Created attachment 187871 [details]
Fix

Forgot to update the author name in the boiler plate

Comment 11 Ketan Padegaonkar 2011-01-28 21:31:57 EST

Patch looks good to me. However does this sound rather fishy to you ;-)

	/**
	 * Return the current value of the scale.
	 *
	 * @return the current selection in the scale.
	 */
	public int getSelection();

I'm completely unsure why the SWT team is obsessed with the term 'selection' when this is clearly not a selection. A selection to me implies an act of choosing one of the (many) options available. A scale or a slider merely sets the value, very much like a spinner.

SWTBot is meant to be a human readable API over SWT's (IMO not-so-readable API). As such it tries to keep away from SWT's choice of method signatures. Would you be very much against it if I changed it to get/setValue ?
	
(In reply to comment #9)
> Created attachment 187870 [details]
> Fiix
> 
> Updated patch. Added junit test and had the generator plugin make the
> SWTBot.java changes (previous one was hand-tweaked). I didnt' change the names
> of get/setSelection() because I think the key is consistency. The Scale and
> Slider widgets are very similar not only in function but in API. SWTBotSlider
> has get/setSelection(), and not get/setValue(). It only makes sense for
> SWTBotScale to have the same. Not to mention, the SWT widget uses 'selection'
> and not 'value'.

Comment 12 Ketan Padegaonkar 2011-01-28 22:03:19 EST

Created attachment 187897 [details]
Updated patch file

John: I'm holding onto this patch for your final approval before being checked into trunk.

Patch file containing:

* additional unit tests for missing methods
* changes to use get/setValue instead of get/setSelection

Comment 13 Ketan Padegaonkar 2011-01-28 22:03:21 EST

Created attachment 187898 [details]
mylyn/context/zip

Comment 14 John Cortell 2011-01-28 22:07:35 EST

(In reply to comment #11)
> Patch looks good to me. However does this sound rather fishy to you ;-)
> 
>     /**
>      * Return the current value of the scale.
>      *
>      * @return the current selection in the scale.
>      */
>     public int getSelection();
> 
> I'm completely unsure why the SWT team is obsessed with the term 'selection'
> when this is clearly not a selection. A selection to me implies an act of
> choosing one of the (many) options available. A scale or a slider merely sets
> the value, very much like a spinner.
> 
> SWTBot is meant to be a human readable API over SWT's (IMO not-so-readable
> API). As such it tries to keep away from SWT's choice of method signatures.
> Would you be very much against it if I changed it to get/setValue ?

Yes. They sure weren't consistent in that javadoc. I can't argue that 'selection' is better than 'value'. Again, I was focused only on consistency. Feel free to change it as you see fit.

Comment 15 Ketan Padegaonkar 2011-01-29 00:51:49 EST

Marking fixed in revision 3146430cb (https://github.com/ketan/SWTBot/commit/3146430cb2cdd38e9577e6bee1afd81b72248018). Thanks for your patience. You can grab a build with your changes from hudson.eclipse.org.