RangeAnnotations along measure axis fail due to assuming range values are DateTime

[diablodale]

Range annotations along the measure axis fail due to assuming all annotation start/end values are DateTime.

Setup

• Flutter (Channel beta, v0.5.1, on Microsoft Windows [Version 10.0.17134.165], locale en-US)
• charts_flutter 0.3.0
• Flutter create app + code from chart example -> code below
• Google CLA for this contribution on file.

Repo

1. Make a new flutter project
2. Replace main.dart with code below
3. Add to pubspec.yaml the following dependency: charts_flutter: "^0.3.0"
4. Run in emulator

Result

I/flutter ( 7536): type 'int' is not a subtype of type 'DateTime'
...
I/flutter ( 7536): #0      RangeAnnotation._updateViewData.<anonymous closure> (package:charts_common/src/chart/common/behavior/range_annotation.dart:147:62)
I/flutter ( 7536): #1      List.forEach (dart:core/runtime/libgrowable_array.dart:274:8)
I/flutter ( 7536): #2      RangeAnnotation._updateViewData (package:charts_common/src/chart/common/behavior/range_annotation.dart:128:17)
...

Notes

In charts_common/src/chart/common/behavior/range_annotation.dart:147 is a call to _getAnnotationDatum() which due to the generic's type, is assuming that the startValue and endValue are DateTime. However, on the measure axis in this repo, they are int -> leading to the exception.

I don't have an immediate recommendation as type changes to that class will likely cascade.

The only lines which cause the exception are 78-79 in the main.dart code below. I copied the code from this repo's example for annotations... changed the start/end values from DateTime to int values... changed axis type to charts.RangeAnnotationAxisType.measure.

main.dart

import 'dart:math';
import 'package:flutter/material.dart';
import 'package:charts_flutter/flutter.dart' as charts;

void main() => runApp(new MyApp());

class MyApp extends StatelessWidget {
  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return new MaterialApp(
      title: 'Flutter Demo',
      theme: new ThemeData(
        // This is the theme of your application.
        //
        // Try running your application with "flutter run". You'll see the
        // application has a blue toolbar. Then, without quitting the app, try
        // changing the primarySwatch below to Colors.green and then invoke
        // "hot reload" (press "r" in the console where you ran "flutter run",
        // or press Run > Flutter Hot Reload in IntelliJ). Notice that the
        // counter didn't reset back to zero; the application is not restarted.
        primarySwatch: Colors.blue,
      ),
      home: new TimeSeriesRangeAnnotationChart.withSampleData(),
    );
  }
}

class TimeSeriesRangeAnnotationChart extends StatelessWidget {
  final List<charts.Series> seriesList;
  final bool animate;

  TimeSeriesRangeAnnotationChart(this.seriesList, {this.animate});

  /// Creates a [TimeSeriesChart] with sample data and no transition.
  factory TimeSeriesRangeAnnotationChart.withSampleData() {
    return new TimeSeriesRangeAnnotationChart(
      _createSampleData(),
      // Disable animations for image tests.
      animate: false,
    );
  }

  // EXCLUDE_FROM_GALLERY_DOCS_START
  // This section is excluded from being copied to the gallery.
  // It is used for creating random series data to demonstrate animation in
  // the example app only.
  factory TimeSeriesRangeAnnotationChart.withRandomData() {
    return new TimeSeriesRangeAnnotationChart(_createRandomData());
  }

  /// Create random data.
  static List<charts.Series<TimeSeriesSales, DateTime>> _createRandomData() {
    final random = new Random();

    final data = [
      new TimeSeriesSales(new DateTime(2017, 9, 19), random.nextInt(100)),
      new TimeSeriesSales(new DateTime(2017, 9, 26), random.nextInt(100)),
      new TimeSeriesSales(new DateTime(2017, 10, 3), random.nextInt(100)),
      new TimeSeriesSales(new DateTime(2017, 10, 10), random.nextInt(100)),
    ];

    return [
      new charts.Series<TimeSeriesSales, DateTime>(
        id: 'Sales',
        domainFn: (TimeSeriesSales sales, _) => sales.time,
        measureFn: (TimeSeriesSales sales, _) => sales.sales,
        data: data,
      )
    ];
  }
  // EXCLUDE_FROM_GALLERY_DOCS_END

  @override
  Widget build(BuildContext context) {
    return new charts.TimeSeriesChart(seriesList, animate: animate, behaviors: [
      new charts.RangeAnnotation([
        new charts.RangeAnnotationSegment(40,
            50, charts.RangeAnnotationAxisType.measure),
      ]),
    ]);
  }

  /// Create one series with sample hard coded data.
  static List<charts.Series<TimeSeriesSales, DateTime>> _createSampleData() {
    final data = [
      new TimeSeriesSales(new DateTime(2017, 9, 19), 5),
      new TimeSeriesSales(new DateTime(2017, 9, 26), 25),
      new TimeSeriesSales(new DateTime(2017, 10, 3), 100),
      new TimeSeriesSales(new DateTime(2017, 10, 10), 75),
    ];

    return [
      new charts.Series<TimeSeriesSales, DateTime>(
        id: 'Sales',
        domainFn: (TimeSeriesSales sales, _) => sales.time,
        measureFn: (TimeSeriesSales sales, _) => sales.sales,
        data: data,
      )
    ];
  }
}

/// Sample time series data type.
class TimeSeriesSales {
  final DateTime time;
  final int sales;

  TimeSeriesSales(this.time, this.sales);
}

[diablodale]

continues to reproduce on charts version 0.4.0

[jannik3141]

Same problem here, but I got a suggestion:

The problem is in the following code

charts/charts_common/lib/src/chart/common/behavior/range_annotation.dart

Line 152 in 8a4142e

final annotationDatum = _getAnnotationDatum(annotation.startValue,

where _getAnnotationDatum() needs parameters of type D, which is (I strongly guess) is DateTime in the TimeSeriesChart. But at the measure axis the type should be int.

  final annotationDatum = _getAnnotationDatum(annotation.startValue,
          annotation.endValue, axis, annotation.axisType);
           
           ...

// _getAnnotationDatum() definition
  _DatumAnnotation<D> _getAnnotationDatum(D startValue, D endValue,
      ImmutableAxis<D> axis, RangeAnnotationAxisType axisType) {
    final startPosition = axis.getLocation(startValue);
    final endPosition = axis.getLocation(endValue);

    return new _DatumAnnotation<D>(
        startPosition: startPosition,
        endPosition: endPosition,
        axisType: axisType);
  }

My solution

By inlining the _getAnnotationDatum() function, the values are passed directly to axis.getLocation(). That does not cause a problem, because this function expects axis specific types.

      final annotationDatum =  new _DatumAnnotation<D>(
            startPosition: axis.getLocation(annotation.startValue),
            endPosition: axis.getLocation(annotation.endValue),
            axisType: annotation.axisType);

I don't have very good internal knowledge about this library, so I can not ensure that this will fix the bug completely and will not cause side effects.
But this inlining seems pretty safe to me and in my testing the modification works without error or warning, giving the desired output.

[diablodale]

Thanks for the encouragement. By changing the type of the parameters on _getAnnotationDatum(), the immediate exception/bug is resolved. This approach was both less code change and allows for flexibility in axis data types. Now horizontal and vertical axes work with int, float, and datetimes.

I tested the forthcoming pull request fix with: my app, the test code at the top of this issue, and the chart gallery app that comes with this chart package. Those that worked before continue to work the same. Those that failed with an exception before, now work.

With this fixed, it exposes a painting oddity in the test code at the top. The annotation shading extends beyond the right edge of the chart. I consider that a separate issue, so I will not address that in a forthcoming pull request related to this issue.

While investigating this bug, I saw several classes in charts_common\lib\src\chart\common\behavior\range_annotation.dart that I suspect do not need to be generics with <D>. As we can see in this issue, the programmers focused only on DateTime. I suspect they became used to the generic pattern and applied it on classes which are likely inappropriate. Below are the classes which use the generic yet I suspect are unneeded. Since the generic's type is not materially used within these classes, it appears to be functionally harmless today. However, there may be some detriment to compile/run times or memory usage. Only testing can confirm/deny.

class _DatumAnnotation<D>
class _AnimatedAnnotation<D>
class _AnnotationElement<D>

[jannik3141]

I just found out the bug is fixed now with 67709bc, but the new github version is not yet available via the packet manager. To temporarily work with the fixed (and enhanced version) I cloned the project and included charts_flutter locally:

pubspec.yaml:

  charts_flutter:
    path: <path_to_repo>/charts_flutter

Just in case someone needs the fix asap ;)