xxxWasSelectedAtRecordIndex should fire on UP events, not down

[GoogleCodeExporter]

Most touch/mouse events occur on the Up event, instead of the down (short of 
dragging). However, all of the delegate methods for the various graphs seem to 
be using the Down event.

This causes problems with graphs that are embedded in scrollable views, as you 
try to scroll and the events are unexpectedly fired.

Would you consider changing this behavior in the next version?

Thanks,
Graham

Original issue reported on code.google.com by graham.m...@gmail.com on 4 Nov 2013 at 10:00

[GoogleCodeExporter]

This will additionally cause issue when "accidentally clicking" (IE user 
accidentally grabs one bar, and tries to drag away to prevent actually 
selecting it). The expected behavior in this case is that the event would not 
be fired at all (or at least not for that bar), but currently it would be fired 
immediately on the accidental tap-down.

Original comment by graham.m...@gmail.com on 4 Nov 2013 at 10:04

[GoogleCodeExporter]

Should we change the behavior of the existing selection methods or add new ones 
with the new behavior to give the user a choice? If we change the existing 
behavior, that will need to go into the release 2.0 branch along with the other 
major changes in progress over there.

Original comment by eskr...@mac.com on 14 Nov 2013 at 11:58

• Changed state: Accepted

[GoogleCodeExporter]

I think that allowing the user to choose would make sense, but the default 
should be (in my opinion) on Up.

Based on that, I believe this will need to go into the 2.0 Branch. Is that  
nearing release, or is that likely not going to happen yet this year?

Original comment by graham.m...@gmail.com on 15 Nov 2013 at 12:05

[GoogleCodeExporter]

We don't have a timeline for the 2.0 release yet. There are still some major 
architecture and performance changes that need to happen that are not ready to 
check in. The 2.0 branch is functional—I know some people are already using 
it.

Original comment by eskr...@mac.com on 16 Nov 2013 at 12:28

[GoogleCodeExporter]

Original comment by eskr...@mac.com on 16 Nov 2013 at 1:28

• Added labels: Milestone-Release2.0

[GoogleCodeExporter]

Hi Eric,

Here are two patches that should resolve this issue for the 2.0 release.  I 
took two different approaches, let me know if either would work for you.

The first was using a boolean to determine when to fire the event. It didn't 
change the API at all, but adds a property to all of the items that handle 
events on the pointingDeviceUp/Down. If the boolean is set to false, it only 
fires on the down event, if set to true (the default), it will fire on the up 
event. This seems a little kludgy, so I came up with a second change, that 
fully preserves (though deprecating) backwards compatibility.

The second approach is to have all of those items that handle the 
pointingDeviceUp/Down events fire both events, depending on what the delegate 
wants. These methods all have a new API looks like, for 
example,(void)axis:(CPTAxis *)axis labelTouchDown:(CPTAxisLabel *)label 
withEvent:(CPTNativeEvent *)event;

This is nice because it allows the user to choose which events they want (it 
also enables them to respond to both.

Anyway, let me know what you think.

Graham

Original comment by graham.m...@gmail.com on 12 Dec 2013 at 10:59

Attachments:

• SelectedEventUpOrDownOnBool.diff
• TouchUpOrDownDelegateChange.diff

[GoogleCodeExporter]

Thanks. I will take a look at the patches this weekend.

Original comment by eskr...@mac.com on 13 Dec 2013 at 2:38

[GoogleCodeExporter]

The boolean property makes for an awkward API. I prefer the solution that adds 
additional delegate methods. As you say, this gives the application a choice of 
which one to use.

Rather than deprecating and renaming all of the xxxWasSelected: methods, what 
if we instead add the "up" methods and leave the original methods as they are. 
We could call the "up" methods something like xxxWasReleased:. Since adding new 
APIs doesn't break old apps, this change wouldn't need to wait for the 2.0 
release.

Original comment by eskr...@mac.com on 13 Dec 2013 at 9:33

[GoogleCodeExporter]

If that's what you would prefer, it could certainly be done. However, to me 
"wasSelected" and "wasReleased" are not corresponding events.

Selection is really up/down agnostic, and seems like more of a "this item is 
now in focus" event.

The wasPressed/Released method would better indicate what is actually happen -- 
mouse/finger down/Up.

I realize you're suggesting this in favor of getting it into 1.x, but to me the 
breaking API change seems like a better fit, which would make it a better 
candidate for 2.0. That's just my opinion, though, so I can certainly modify my 
patch to make it fit your preference.

Original comment by graham.m...@gmail.com on 13 Dec 2013 at 9:38

[GoogleCodeExporter]

I agree with the approach, I'm just looking for the right name for the event. 
Core Plot is platform agnostic; the names should make sense on both Mac and 
iOS. That's why we used "pointingDeviceXXX" for the CPTResponder events and 
"xxxWasSelected:" for the selection events.

However, none of this addresses the issue you raised in the initial issue 
report or the first comment. Rather than deprecate "xxxWasSelected:", why not 
use those to indicate when the down and up events occur on the same plot point? 
For example, if you touch a bar on a bar plot, slide your finger to a different 
bar, and then lift the finger, you'll get the down event for the first bar and 
the up event for the second bar, but not the selected event. If the up event 
occurs on the same bar as the down event, you'll get the selected event, too.

I'm not sure if this change in behavior will fix the issue when embedded in a 
scroll view or if more needs to be done to handle that case, too. The solution 
might be different on Mac and iOS.

Original comment by eskr...@mac.com on 14 Dec 2013 at 4:13

[GoogleCodeExporter]

I know what you mean about choosing the right name. I recognize the cross 
platform aspect of the framework, and had actually looked at a couple of other 
libs that are as well when I decided on the touch name.

Check out LibGDX's (a gaming lib) InputProcessor class at 
http://libgdx.badlogicgames.com/nightlies/docs/api/ (sorry, I can't directly 
link to the class, JavaDocs kind of suck :/)

What you mention with touchUp/Down on different, non-matched points is actually 
an issue I had ran into when I was writing a Lua UI framework. It does actually 
get kind of tricky; what should the behavior be? If you started (down) on one, 
and ended (up) on another, are you actually trying to 'select' the latter, or 
are you canceling your selection of the first?

In that sense, rather than exposing either of those new events (up/down), maybe 
a change in the behavior of wasSelected would make sense -- it could only be 
fired when both up and down are triggered on the same point.

Regarding the scroll behavior, I believe this will work, though I guess I'm not 
100% sure -- we put a separate work around in place to deal with it for this 
release, and I didn't try pulling it out to test. I'll give it a shot once 
we've decided on a strategy here, since presumably that's going to require some 
rework depending on the direction it goes.

Original comment by graham.m...@gmail.com on 14 Dec 2013 at 6:05

[GoogleCodeExporter]

> It does actually get kind of tricky; what should the behavior be? If you 
started (down) on one, and ended (up) 
> on another, are you actually trying to 'select' the latter, or are you 
canceling your selection of the first?

That's why having both the up/down events and xxxWasSelected: would be useful. 
The application could choose which one is appropriate for what it is doing. 
xxxWasSelected: would only fire if the down and up events occur on the same 
data point.

I like your suggestion in comment #9 of using xxxWasPressed: for the down 
events and xxxWasReleased: for the up events.

Original comment by eskr...@mac.com on 15 Dec 2013 at 5:25

[GoogleCodeExporter]

Moved to GitHub issue #27.

https://github.com/core-plot/core-plot/issues/27

Original comment by eskr...@mac.com on 30 Dec 2013 at 1:30

• Changed state: Done