Override data point values functionality

[ewchow]

Hey Ankit,

Been a while! Hope all is well.

Got a fairly hefty update for you here, but I think you'll like the new functionality. This is related to #142. I extended the Adjust Point (S) functionality in a few ways, the main addition is the ability to override point values. I opted to store the override values in each point's metadata field, and since bar charts uses that field for labels, override values currently does not support bar charts.

I wanted to get your input on the metadata field before I make any changes there. What do you think about converting metadata into an object instead of an array? (With backwards compatibility of course.)

But, without further ado, the outline of the changes are follows:

• Ability to override point values
  • New section in the right sidebar when one or more points are selected with the Adjust Point (S) tool
  • Default (no overrides exist in selection):
  • Override indicator (overrides exist in selection):
  • New popup for editing overrides
  • Default view for single points displays calculated values:
  • When one a field of the single point has an override value:
  • Labels and fields are programmatically generated based on axes data
  • Override values are stored as metadata for each point
    • Bar charts currently not supported
  • OK sets the entered values
  • Cancel discards changes
  • Clear reverts the fields (users still have to click OK)
  • Multiple points can be overridden at once
    • Placeholder text Multiple points selected, Multiple override values, and Some values overridden will be displayed instead of calculated values when multiple points are selected
    • Override value will display for each point field if all selected points have the same override value
  • Added shortcut key R to trigger new popup
    • Updated shortcut key informational popup to reflect new shortcut
• Ability to select multiple points when adjusting points by dragging
  • Multiple points can be moved at the same time
  • Relative positions between points are preserved
  • Shortcut keys for all other actions (anything that isn't a directional key or R) will only function on single point selections
  • Single click point selection functionality is preserved
• Miscellaneous
  • Updated wpd.Dataset.hasMetadata infrastructure
  • Now based on count instead, a bit more robust
  • Added getType function to each axes definition
  • Simplifies identification of axes types
  • Updated null metadata values to be null instead of 0 in the data table
  • This does not apply to bar charts
  • Bug fixes
  • Adjust Point (S) shortcut key E to edit labels now only functions when axes type is bar chart
  • Deleting a single point via Adjust Point (S) shortcut Backspace or Delete now updates point count display properly

As always, happy to make any changes you'd like me to. Let me know what you think.

[ankitrohatgi]

Thanks! I'll try to finish the review this weekend :)

[ankitrohatgi]

I wanted to get your input on the metadata field before I make any changes there. What do you think about converting metadata into an object instead of an array? (With backwards compatibility of course.)

I am not opposed to the change. I think I put in metadata as a hack to make bar charts work, but if we need to change it to accommodate new things, then we should change it.

[ankitrohatgi]

So I checked out your branch to try this out myself. Based on the description and UI behavior, I was assuming overriding value would effectively shift the point at that location (more precise shift than what you get with a keyboard/mouse). But looks like that's not the intention here - I'm wondering why you wanted to store the overridden values in metadata vs just shifting the point itself?

[ewchow]

I am not opposed to the change. I think I put in metadata as a hack to make bar charts work, but if we need to change it to accommodate new things, then we should change it.

Great! I will make the change. This will allow for bar chart support as well.

So I checked out your branch to try this out myself. Based on the description and UI behavior, I was assuming overriding value would effectively shift the point at that location (more precise shift than what you get with a keyboard/mouse). But looks like that's not the intention here - I'm wondering why you wanted to store the overridden values in metadata vs just shifting the point itself?

So I originally leaned in the direction of shifting the point, but when I took a closer look at #142, I realized the axes values could be discontinuous or be something other than numbers. In those cases, I thought directly (or indirectly via dataToPixel) modifying the coordinates could lead to misleading placement of points or a loss of information. So I ultimately went in the direction of storing the information as metadata for flexibility and axes integrity.

[ewchow]

Few new changes based on our conversation:

• Updated usages of var to let or const
• Metadata has been converted from arrays to objects
• Value override now supports bar charts
• Metadata can be loaded in the form of an array or object

[billdenney]

Hey, thanks for this PR!

For the idea of shifting the point or not, I'd prefer not to shift it. The rationale is that there are times when I will know what a point value is (like in #74) but if the value were shifted, it would not be in the visually-correct location on the graph.

[ankitrohatgi]

@billdenney, @ewchow - based on your use case, it looks like the override values are more like "labels". Do you think just renaming it to something other than "overrides" is possible?

I'm ok with the general approach - just the name "override" threw me off a bit since I was expecting that to change the point locations.

[ankitrohatgi]

Also, I'll try to do a more detailed code review later tonight. Apologies that reviews have been kind of slow :(

[billdenney]

@ankitrohatgi, I don't have a name preference. If you would prefer something else, perhaps "replace" would fit the action better? Does that work for you?

[ankitrohatgi]

This looks good mostly - the we can change the name for "override" later, it was just a nitpicky comment.

I did give this a shot manually and things work fine - I'll go ahead and merge this.

[ewchow]

Happy to change the name to whatever you prefer later. Thanks for looking through the code changes!

[ankitrohatgi]

I'm seeing that this has broken a lot of things for bar charts :(

For example, I can not change label and see that label in the "view data" window anymore. I also found a number of save/resume bugs that had to be fixed. If I can't fix these bugs soon, then I'm going to have to revert this PR.

[ankitrohatgi]

I think I have a fix for some of the bugs now. Just checked those into master.

[ewchow]

Must have missed some cases with the later commits, sorry about that. I'll run through all the workflows again. Are there any known issues I can look into?

[ewchow]

Ah, I missed the auto extraction functionality. Thanks for fixing them. I haven't seen anything else so far, but please let me know if you find anything else. I'll include qunit tests with the next updates.

[ankitrohatgi]

Yeah, the code coverage of the tests is quite sparse unfortunately :( I never had the bandwidth to write tests. I'm in the process of testing and hardening various things we added in the last few months and officially switch to 4.3 on the website in the next few days.