Provide threshold elements for charting

[cjolif]

The thresholds elements would draw vertical/horizontal lines at data position and provide optional label.

Optionally this can be a:

• dual threshold (low/high)
• the area can be filled
• on can listen to interaction events on the threshold

[cjolif]

Additionally the Mouse/TouchIndicator actions should be re-written on top of that element.

[cjolif]

draft patch (no doc yet, look at examples), this includes #15589 as well

[cjolif]

updated patch with API doc, animation support & some code factorization on CartesianBase?

[cjolif]

The latest patch is breaking alternate axis & widget markup interpretation, I'll be working on fixing this before committing.

[cjolif]

In [29694]:

fixes #15486, #15589. Implements a plot threshold "indicator" that can be drawn vertically or horizontally on the chart. Make sure that the mouse/touch indicator is leveraging it. Include some small enhancements to mouse indicator (on"over") as well as some CartesianBase? code refactoring. !strict.

[ben hockey]

this breaks existing code that subclasses CartesianBase? and calls this.cleanGroup() without passing the params it now relies upon - creator, dim, offsets, noClip. is there an alternative implementation that remains backward compatible or do i need to update my subclasses?

[ben hockey]

this goes further than i first realized. it also breaks subclasses of action2d/_IndicatorElement subclasses which are indirectly a subclass of CartesianBase?. i hadn't noticed these ones until i started to interact more with some of my custom actions. some of my actions just don't have access to dims and offset when calling cleanGroup because they are used in event handlers that may track mouse movement or touches etc which may call cleanGroup in response to those events - ie render is not the only place that cleanGroup may be called from.

while it's been possible to keep all dojox/charting code working with this change, it is really hurting my custom code. while i know dojox/charting is part of dojox and is technically allowed to break backward compatibility, this change has caught me by surprise.

[cjolif]

Hi Ben, I'll look at CartesianBase incompatibility, I agree we should do our best to keep compatibility here. I just wanted to avoid querying several times those values and so I passed them as arguments. But I can certainly get them directly in the cleanGroup method.

For _IndicatorElement, this was/is a private class. So I except you might have to change some code to get it back running on your side. In particular there is a noClip optional argument that is added to CartesianBase cleanGroup. It is set to true in the new Indicator code (because you don't want the indicator to be clipped to the plot area) which the new !_IndicatorElement inherits from. If you set it to true you should not need the dims & offsets. So cleanGroup should be back working even if called at some point where you don't have the dims/offsets.

[cjolif]

patch to fix API regression on CartesianBase?.cleanGroup

[ben hockey]

i didn't think of _IndicatorElement as a private class because it looks like it provides a base class to build other classes. If I want to build something like MouseIndicator?, it's useful to add a plot that is either an _IndicatorElement (as done by MouseIndicator?) or a subclass of it if i have slightly different requirements. the '_' in a class name often indicates something is a mixin or base class and this was what i thought in this case since it contains code that would be reusable.

anyhow, discussing '_' is just semantics, i could just as easily have said above that i was using plot2d/Indicator (the base class for _IndicatorElement) and i would be having the same problem because my problem doesn't actually come from _IndicatorElement itself. i was just using it so i didn't have to repeat code that's in it.

i tried the noClip option and it doesn't work for me. i have to call cleanGroup from within _trackMove in order for my indicator to work properly and if i use noClip i get some issues. as a side note, i think there's a bug as to why i need to call cleanGroup but i haven't had time to distill it into a test case to figure out how to build a test case. again, that's beside the point now with the patch you've suggested.

the patch should work for me and since it requires no processing to get this.chart.dim and this.chart.offsets i'd say it's not too bad to use those since they should be more definitive than what a user may pass in accidentally (or not even pass in at all). if for some reason you wanted to allow a user to pass in dims and offsets then you could fall back to this.chart.dim and this.chart.offsets if they weren't provided. however, it probably only makes sense to allow for that if you think there's some useful case where the values wouldn't already be the ones from the chart.

[cjolif]

Yes I know your specific issue can happen with more than just _IndicatorElement. However I wanted to emphasize the fact I did not consider this module as a public supported API as of today because you might encounter other issues as I did not tried to keep compatibility on it during my refactoring (note that in addition to the underscore the doc "api" as a "private" tag). Anyway as the patch seems to solve your issue I will commit it. Let me know if you still experience a problem.

[cjolif]

In [29763]:

fixes #15486. Prevents an API regression. !strict.

[cjolif]

In [29957]:

refs #15486. Make sure onChange is called even if text is not displayed in the indicator. !strict.

[cjolif]

In [30266]:

refs #15486. Fix typo + fix for Safari desktop.

[cjolif]

In [30926]:

refs #15486. Fix a regression introduce on which value a Mouse/TouchIndicator? actually displays. !strict