Add custom label formatter support for crosshair by Maoyao233 · Pull Request #1173 · ScottPlot/ScottPlot

Maoyao233 · 2021-08-02T03:13:21Z

New Contributors:
please review CONTRIBUTING.md

Purpose:
#1172

New Functionality:

 var ch = plt.AddCrosshair(20, 0);

// use the custom formatter for crosshair labels
ch.ManualStringFormatterX = customTickFormatter;
ch.ManualStringFormatterY = customTickFormatter;

bclehmann

Looks good, just a few minor things. They're not about your code so much as maintaining a consistent API, as anyone who uses this feature probably is also using the custom formatter on the axis as well. So those features should probably expose a similar API.

Maybe even the crosshair label formatter should use the axis formatter? I don't know how easy that would be though, especially now that we have support for multiple axes. It would also introduce a lot of coupling.

bclehmann · 2021-08-02T22:09:19Z

src/ScottPlot/Plottable/Crosshair.cs

+        /// <summary>
+        /// If defined, this function will be used to generate labels from X position, value of StringFormatX and IsDateTimeX will be ignored
+        /// </summary>
+        public Func<double, string> ManualStringFormatterX = null;


I think TickLabelFormat is ignored if it's a datetime axis, so this introduces an inconsistency.

I think it's probably better that it isn't ignored for datetime axes, but that change would mean changing the axis code as well.

I think TickLabelFormat is ignored if it's a datetime axis

Maybe there should be a NumericFormatter and DateTimeFormatter?

I think it's fine for the user to figure that out themselves, as they should know whether or not it's a DateTime axis.

Although I suppose we could make it easier for them by having DateTimeFormatter take in a DateTime instead of a double.

bclehmann · 2021-08-02T22:09:40Z

src/ScottPlot/Plottable/Crosshair.cs

            gfx.DrawLine(pen, dims.DataOffsetX, yPixel, dims.DataOffsetX + dims.DataWidth, yPixel);

-            string yLabel = IsDateTimeY ? DateTime.FromOADate(Y).ToString(StringFormatY) : Y.ToString(StringFormatY);
+            string yLabel = ManualStringFormatterY != null ? ManualStringFormatterY(Y) : IsDateTimeY ? DateTime.FromOADate(Y).ToString(StringFormatY) : Y.ToString(StringFormatY);


This ternary within a ternary is pretty unwieldly.

Thanks. I'll avoid using it in future.

bclehmann · 2021-08-02T22:11:15Z

src/ScottPlot/ScottPlot.csproj

-    <Version>4.1.16</Version>
-    <AssemblyVersion>4.1.16.0</AssemblyVersion>
-    <FileVersion>4.1.16.0</FileVersion>
+    <Version>4.1.17</Version>


I think Scott will bump the version himself once this is merged and any other PRs have been merged in.

Indeed! This can be left as 4.1.16 for now 👍

I didn't intentionally change the version number, probably because I'm not familiar with ScottPlot's automated build system. I also think this small change should not change the version number.

bclehmann · 2021-08-02T22:13:03Z

src/cookbook/ScottPlot.Cookbook/Recipes/Plottable/Crosshair.cs

+
+            // use the custom formatter for crosshair labels
+            ch.ManualStringFormatterX = customTickFormatter;
+            ch.ManualStringFormatterY = customTickFormatter;


Again on the consistency argument, it's a little weird that for one of them you pass in a formatter delegate to a function and for the other you set a property.

I think that setting a property is probably the better API, but if we want consistency that would mean changing the axis code.

The consistency argument is also present in the original API. I think changing the axis code is a good way.

bclehmann · 2021-08-02T22:19:15Z

Also, it looks like you made this PR from your master branch, instead of a new branch. That's not a problem for us but it could make it more difficult for you to fetch new commits that get merged in.

swharden · 2021-08-03T00:20:44Z

Hi @Maoyao233, thank you for this PR! I really like the idea of exposing custom formatters in more places throughout ScottPlot, and this is a great improvement.

Thinking out loud, an alternative to allowing formatters to be null we could always use these formatters and pre-populate them with generic formatters created in the constructor. This would eliminate the need for those ternary null checks.

I think separate functions would be helpful, but I recognize this is super verbose:

XNumericFormatter
YNumericFormatter
XDateTimeFormatter
YDateTimeFormatter

Maybe this is an indication this class should be refactored as two smaller objects (because this isn't the only duplication of X and Y properties). Alternatively I wonder if these advanced label features should be added to AxisLine, then Crosshair could contain two of those. We can consider that type of refactoring in a future PR. For now this PR is going in a good direction, and I'm happy to merge it in (with its cookbook recipe) after @Maoyao233 has a chance to review some of the code comments. Thanks @bclehmann for your input!

bclehmann · 2021-08-03T01:03:52Z

I think separate functions would be helpful, but I recognize this is super verbose:
.
XNumericFormatter
YNumericFormatter
XDateTimeFormatter
YDateTimeFormatter

One could allow the user to pass in one function (or class) which would override one or several of these? It could be done with the decorator pattern. There is a way to do this where they pass in a function, where they're passed an object, and setting fields of that object sets the formatter. The formatter should probably be allowed to be null just so it's easy for them to reset it.

Both of these are departures from the rest of the ScottPlot API however.

swharden · 2021-08-03T12:24:18Z

@Maoyao233 thanks again for this PR and your feedback! When you're done making changes to this PR let me know and I'll make a few small changes of my own then merge this in 👍

One could allow the user to pass in one function (or class) which would override one or several of these? It could be done with the decorator pattern.

Both of these are departures from the rest of the ScottPlot API however.

@bclehmann this is a good observation! This trend could be taken to its logical conclusion by having a fully-abstracted "configuration object" or "styling object" that could be instantiated and modified by the user and passed into plots or plottables. This would help separate code for configuration, styling, and rendering. If ScottPlot were a private library that only I used, I would prefer this. Since it's a public library and I strive to make it quick and easy to learn for new users, I find myself preferring the simplicity of having users assign public fields.

Maoyao233 · 2021-08-04T09:27:09Z

@swharden I have finished my work on this PR.

I think TickLabelFormat is ignored if it's a datetime axis, so this introduces an inconsistency.

Again on the consistency argument, it's a little weird that for one of them you pass in a formatter delegate to a function and for the other you set a property.

I ended up using different formatter for numeric and datetime to avoid the first problem, and I think changing the axis code to keep consistency is valuable.

Thanks again to @swharden and @bclehmann. I've learnt a lot.

bclehmann · 2021-08-04T14:41:59Z

I wonder if we should have a demo as well, for the datetime axes? Perhaps that should wait for when axes get custom datetime formatters.

swharden · 2021-08-06T23:29:15Z

Thanks @Maoyao233 this looks fantastic! I also appreciate your input along the way @bclehmann

I'll be refactoring the Crosshair module shortly. I intend to add support for position labels to axis lines, and when I do that I'll be sure to support this type of custom formatter support as well. I suspect the Crosshair will get refactored to contain one HorizontalLine and one VerticalLine, but we'll see. Progress there will be tracked in #1122

#1122, #1173

#1172, #1173, #1122

Add custom label formatter support for crosshair

16fddf5

bclehmann reviewed Aug 2, 2021

View reviewed changes

Maoyao233 added 2 commits August 4, 2021 16:51

Modified the custom formatter API and comments accroding to the review

be59c3b

run autoformat

fcc6db2

swharden added 3 commits August 6, 2021 19:22

cookbook: refine crosshair formatter recpie

a72c743

Update changelog.md

945456d

Merge branch 'master' into master

16fc188

swharden merged commit a7c20bc into ScottPlot:master Aug 6, 2021

swharden mentioned this pull request Aug 7, 2021

Line Plot: Show position label on axis #1122

Closed

swharden added a commit that referenced this pull request Aug 7, 2021

AxisLine: support position labels

0df6573

#1122, #1173

swharden added a commit that referenced this pull request Aug 8, 2021

crosshair: refactor to use two AxisLines

329d7b5

#1172, #1173, #1122

Conversation

Maoyao233 commented Aug 2, 2021

Uh oh!

bclehmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maoyao233 Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bclehmann commented Aug 2, 2021

Uh oh!

swharden commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bclehmann commented Aug 3, 2021

Uh oh!

swharden commented Aug 3, 2021

Uh oh!

Maoyao233 commented Aug 4, 2021

Uh oh!

bclehmann commented Aug 4, 2021

Uh oh!

swharden commented Aug 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Maoyao233 Aug 3, 2021 •

edited

Loading

swharden commented Aug 3, 2021 •

edited

Loading