Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#15486 closed feature (fixed)

Provide threshold elements for charting

Reported by: cjolif Owned by: cjolif
Priority: undecided Milestone: 1.9
Component: Charting Version: 1.7.2
Keywords: Cc: Mathevet julien
Blocked By: Blocking:

Description (last modified by 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

Attachments (3)

15486.patch (46.6 KB) - added by cjolif 7 years ago.
draft patch (no doc yet, look at examples), this includes #15589 as well
15486_2.patch (66.6 KB) - added by cjolif 7 years ago.
updated patch with API doc, animation support & some code factorization on CartesianBase?
fixes#15486.patch (6.0 KB) - added by cjolif 7 years ago.
patch to fix API regression on CartesianBase?.cleanGroup

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by cjolif

Owner: changed from Eugene Lazutkin to cjolif
Status: newassigned

comment:2 Changed 7 years ago by cjolif

Description: modified (diff)
Type: defectfeature

comment:3 Changed 7 years ago by cjolif

Description: modified (diff)

comment:4 Changed 7 years ago by cjolif

Milestone: tbd1.9

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

Last edited 7 years ago by cjolif (previous) (diff)

Changed 7 years ago by cjolif

Attachment: 15486.patch added

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

comment:5 Changed 7 years ago by cjolif

Cc: Mathevet julien added

Changed 7 years ago by cjolif

Attachment: 15486_2.patch added

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

comment:6 Changed 7 years ago by cjolif

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

comment:7 Changed 7 years ago by cjolif

Resolution: fixed
Status: assignedclosed

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.

comment:8 Changed 7 years ago by ben hockey

Resolution: fixed
Status: closedreopened

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?

comment:9 Changed 7 years ago by 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.

comment:10 Changed 7 years ago by 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.

Last edited 7 years ago by cjolif (previous) (diff)

Changed 7 years ago by cjolif

Attachment: fixes#15486.patch added

patch to fix API regression on CartesianBase?.cleanGroup

comment:11 Changed 7 years ago by 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.

comment:12 Changed 7 years ago by 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.

comment:13 Changed 7 years ago by cjolif

Resolution: fixed
Status: reopenedclosed

In [29763]:

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

comment:14 Changed 6 years ago by cjolif

In [29957]:

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

comment:15 Changed 6 years ago by cjolif

In [30266]:

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

comment:16 Changed 6 years ago by cjolif

In [30926]:

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

Note: See TracTickets for help on using tickets.