fix(Dashboard): Color inconsistency on refreshes and conflicts

[geido]

SUMMARY

After the changes in this PR https://github.com/apache/superset/pull/23762 color inconsistency on refreshes was reintroduced.

Fixes #16970 Fixes #26456 Fixes #28104 Fixes #25384 (specifically for metrics changing colors on refreshes. An additional fix is required for charts emitting only COUNT(*) as label and not specific ones)

This PR does the following:

• Fixes color inconsistency on refreshes
• Fixes color inconsistency from chart in hidden tab to Explore
• It helps avoiding color collisions by adding a usage tracker of the colors at the chart level
• Deprecates AVOID_COLORS_COLLISION feature flag
• Renames the shared labels color map to LabelColorsMap
• Centralizes the logics of generating the colors map and color metadata

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N.A.

TESTING INSTRUCTIONS

1. Set a color scheme for a dashboard
2. Refresh the dashboard
3. Colors should not change
4. Going from any chart in any tab to Explore, should carry the same colors forward

ADDITIONAL INFORMATION

• [ ] Has associated issue:
• [ ] Required feature flags:
• [ ] Changes UI
• [ ] Includes DB Migration (follow approval process in SIP-59)
  • [ ] Migration is atomic, supports rollback & is backwards-compatible
  • [ ] Confirm DB migration upgrade and downgrade tested
  • [ ] Runtime estimates and downtime expectations provided
• [ ] Introduces new feature or API
• [ ] Removes existing feature or API

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 78.66667% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 67.34%. Comparing base (f453d5d) to head (36795f2). Report is 22 commits behind head on master.

Files	Patch %	Lines
...d/src/dashboard/components/gridComponents/Tabs.jsx	9.09%	10 Missing :warning:
...rc/explore/components/ExploreChartHeader/index.jsx	60.00%	3 Missing and 1 partial :warning:
...frontend/src/dashboard/components/Header/index.jsx	50.00%	0 Missing and 1 partial :warning:
...src/dashboard/components/PropertiesModal/index.tsx	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27439      +/-   ##
==========================================
- Coverage   69.61%   67.34%   -2.28%     
==========================================
  Files        1908     1909       +1     
  Lines       74543    74649     +106     
  Branches     8316     8330      +14     
==========================================
- Hits        51895    50272    -1623     
- Misses      20595    22321    +1726     
- Partials     2053     2056       +3     

Flag	Coverage Δ
javascript	57.20% <78.66%> (-0.06%)	:arrow_down:
mysql	78.00% <ø> (-0.01%)	:arrow_down:
postgres	78.10% <ø> (-0.01%)	:arrow_down:
python	78.23% <ø> (-4.65%)	:arrow_down:
sqlite	77.61% <ø> (-0.01%)	:arrow_down:
unit	?

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[villebro]

@geido would it be possible to add a brief description of the mechanics of the changes in this PR? This would help grasp the chosen approach without having to dive directly into the diff.

[geido]

@geido would it be possible to add a brief description of the mechanics of the changes in this PR? This would help grasp the chosen approach without having to dive directly into the diff.

@villebro this is meant to fix the regression introduced in https://github.com/apache/superset/pull/23762 so there is not much more context to add. However, I added inline/GitHub comments to the parts of the code that I think introduce new behaviours or that might require clarifications.

[geido]

A few thoughts about the current state of the implementation and the way forward:

• SharedLabelColorSingleton was meant to store only those labels that are shared across multiple charts. However, that is not the case since this is used to store all label colors independently if shared or not. This should get a better name, probably LabelColorsMap and change its references everywhere.
• There are multiple places that are storing the color map in the dashboard metadata, including saving the properties and clicking on tabs (as of the changes in this PR), on verifyUpdateColorScheme and others. A larger refactoring work is required to coordinate and centralize changes to the metadata. For example, we might centralize updating the metadata when individual tabs load.
• There are instances in which the underlying data of a chart might change and new labels show up or get removed. Currently, the implementation does not store those in the metadata as the saving process only happens when editing properties or when switching tabs. This requires a fix but it makes sense to apply it in the lenses of a larger refactor.
• CategoricalColorScale and CategoricalColorNamespace, as well as SharedLabelColorSingleton should be refactored to improve the hierarchy and lower interdependencies. This PR attempts to improve on that but more refactoring work is required.
• While this PR improves issues with color collisions by introducing a color count at the CategoricalColorScale level, specific logics should be implemented at the individual viz types as order and shapes affect collisions and there is hardly a solution that can account for all possible shapes.

CC @eschutho

[geido]

/testenv up

[github-actions[bot]]

@geido Ephemeral environment spinning up at http://35.86.251.170:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[mistercrunch]

Rebasing should fix the issues around required checks

[geido]

Thanks everyone for the reviews. After some more thinking, I will continue with the refactoring work on this branch so that we can merge a stable/longer term solution to this problem.

[mistercrunch]

I'd love to see a user-oriented FAQ entry "How do colors get assigned to my dashboard?"

[geido]

/testenv up

[github-actions[bot]]

@geido Ephemeral environment spinning up at http://34.219.194.7:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[geido]

/testenv up

[github-actions[bot]]

@geido Ephemeral environment spinning up at http://54.202.10.57:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[sadpandajoe]

I think I may have broken a chart if giving too many data points

Repro steps:

• Create a pie chart
• Dimensions: name
• Metric: sum(rank)
• row limit: 100
• color scheme: red to yellow
• This will create a chart
• bump the row limit to 10000
• This will show white as a color which is not part of the red to yellow palette and if you hover over the white slice, there are actually multiple items in there

[sadpandajoe]

Color collision still happens sometimes on stacked bar charts

Steps to repro:

• Create a bar chart with video games sales dataset
• x-axis: platform
• metric: sum(rank)
• dimensions: name
• row limit: 100
• color theme: blue to green
• stacked style: stack

notice that the GC and PS4 bar have the same color on top of each other

[geido]

Color collision still happens sometimes on stacked bar charts

Steps to repro:

• Create a bar chart with video games sales dataset
• x-axis: platform
• metric: sum(rank)
• dimensions: name
• row limit: 100
• color theme: blue to green
• stacked style: stack

notice that the GC and PS4 bar have the same color on top of each other

After a deeper investigation, it appears that this issue is due to how data is grouped. This is one of those issues that require a dedicated color algorithm as the default one can only check the color usage in the order as the data is received, not in the way it is ultimately grouped and visualized.

[geido]

I think I may have broken a chart if giving too many data points

Repro steps:

• Create a pie chart
• Dimensions: name
• Metric: sum(rank)
• row limit: 100
• color scheme: red to yellow
• This will create a chart
• bump the row limit to 10000
• This will show white as a color which is not part of the red to yellow palette and if you hover over the white slice, there are actually multiple items in there

This seems to be a problem with the Pie chart in general as I can reproduce it on master too. I'll track it for a separate investigation.

[github-actions[bot]]

Ephemeral environment shutdown and build artifacts deleted.