You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by di...@apache.org on 2024/10/17 17:24:21 UTC
(superset) branch geido/fix/color-consistency-race-conditions updated: fix(Dashboard): Keep track of charts origin color schemes
This is an automated email from the ASF dual-hosted git repository.
diegopucci pushed a commit to branch geido/fix/color-consistency-race-conditions
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/geido/fix/color-consistency-race-conditions by this push:
new 208a929de9 fix(Dashboard): Keep track of charts origin color schemes
208a929de9 is described below
commit 208a929de9520261856d49d3f9a24461feca8358
Author: Diego Pucci <di...@gmail.com>
AuthorDate: Thu Oct 17 20:24:03 2024 +0300
fix(Dashboard): Keep track of charts origin color schemes
---
.../cypress/e2e/dashboard/editmode.test.ts | 4 +-
.../src/color/LabelsColorMapSingleton.ts | 50 ++++++++-
.../test/color/LabelsColorMapSingleton.test.ts | 41 +++++++
.../src/dashboard/actions/dashboardState.js | 120 +++++++++++----------
.../DashboardBuilder/DashboardContainer.tsx | 16 ++-
superset-frontend/src/utils/colorScheme.ts | 10 +-
6 files changed, 170 insertions(+), 71 deletions(-)
diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts
index 314e0f6048..cec0b6aca5 100644
--- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts
+++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts
@@ -507,7 +507,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
- .should('have.css', 'fill', 'rgb(108, 69, 146)');
+ .should('have.css', 'fill', 'rgb(0, 128, 246)');
// open main tab and nested tab
openTab(0, 0);
@@ -518,7 +518,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(108, 69, 146)');
+ .should('have.css', 'fill', 'rgb(0, 128, 246)');
});
it('should apply the color scheme across main tabs', () => {
diff --git a/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts b/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts
index 59d3f8cc5d..c44304f142 100644
--- a/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts
+++ b/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts
@@ -17,7 +17,9 @@
* under the License.
*/
+import { getCategoricalSchemeRegistry } from '.';
import { makeSingleton } from '../utils';
+import CategoricalColorNamespace from './CategoricalColorNamespace';
export enum LabelsColorMapSource {
Dashboard,
@@ -38,7 +40,16 @@ export class LabelsColorMap {
this.source = LabelsColorMapSource.Dashboard;
}
- updateColorMap(categoricalNamespace: any, colorScheme?: string) {
+ /**
+ * Wipes out the color map and updates it with the new color scheme.
+ *
+ * @param categoricalNamespace - the namespace to use for color mapping
+ * @param colorScheme - color scheme
+ */
+ updateColorMap(
+ categoricalNamespace: CategoricalColorNamespace,
+ colorScheme?: string,
+ ) {
const newColorMap = new Map();
this.colorMap.clear();
this.chartsLabelsMap.forEach((chartConfig, sliceId) => {
@@ -58,6 +69,16 @@ export class LabelsColorMap {
return this.colorMap;
}
+ /**
+ *
+ * Called individually by each plugin via getColor fn.
+ *
+ * @param label - the label name
+ * @param color - the color
+ * @param sliceId - the chart id
+ * @param colorScheme - the color scheme
+ *
+ */
addSlice(
label: string,
color: string,
@@ -75,12 +96,34 @@ export class LabelsColorMap {
labels.push(label);
this.chartsLabelsMap.set(sliceId, {
labels,
- scheme: colorScheme,
+ scheme: this.chartsLabelsMap.get(sliceId)?.scheme || colorScheme,
});
}
this.colorMap.set(label, color);
}
+ /**
+ * Keeps track of original colors of charts in their formData.
+ *
+ * @param sliceId - chart id
+ * @param colorScheme - color scheme
+ */
+ setSliceOriginColorScheme(sliceId: number, colorScheme?: string) {
+ const categoricalSchemes = getCategoricalSchemeRegistry();
+ const fallbackColorScheme =
+ categoricalSchemes.getDefaultKey()?.toString() ?? 'supersetColors';
+
+ this.chartsLabelsMap.set(sliceId, {
+ labels: this.chartsLabelsMap.get(sliceId)?.labels || [],
+ scheme: colorScheme || fallbackColorScheme,
+ });
+ }
+
+ /**
+ * Remove a slice from the color map.
+ *
+ * @param sliceId - the chart
+ */
removeSlice(sliceId: number) {
if (this.source !== LabelsColorMapSource.Dashboard) return;
@@ -96,6 +139,9 @@ export class LabelsColorMap {
this.colorMap = newColorMap;
}
+ /**
+ * Clear the shared labels color map.
+ */
clear() {
this.chartsLabelsMap.clear();
this.colorMap.clear();
diff --git a/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts b/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts
index b93a416e7f..eaed2e4a67 100644
--- a/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts
+++ b/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts
@@ -60,6 +60,29 @@ describe('LabelsColorMap', () => {
expect(getLabelsColorMap()).toBeInstanceOf(LabelsColorMap);
});
+ describe('.setSliceOriginColorScheme(sliceId, scheme)', () => {
+ it('should set slice color scheme', () => {
+ const labelsColorMap = getLabelsColorMap();
+ labelsColorMap.setSliceOriginColorScheme(1, 'testColors');
+ expect(labelsColorMap.chartsLabelsMap.get(1)?.scheme).toEqual(
+ 'testColors',
+ );
+ });
+ it('should fallback to default color scheme if not provided', () => {
+ const labelsColorMap = getLabelsColorMap();
+ labelsColorMap.setSliceOriginColorScheme(1);
+ expect(labelsColorMap.chartsLabelsMap.get(1)?.scheme).toEqual(
+ 'SUPERSET_DEFAULT',
+ );
+ });
+ it('should re-use existing labels when setting color scheme', () => {
+ const labelsColorMap = getLabelsColorMap();
+ labelsColorMap.addSlice('a', 'red', 1);
+ labelsColorMap.setSliceOriginColorScheme(1, 'testColors');
+ expect(labelsColorMap.chartsLabelsMap.get(1)?.labels).toEqual(['a']);
+ });
+ });
+
describe('.addSlice(value, color, sliceId)', () => {
it('should add to sliceLabelColorMap when first adding label', () => {
const labelsColorMap = getLabelsColorMap();
@@ -98,6 +121,24 @@ describe('LabelsColorMap', () => {
labelsColorMap.addSlice('a', 'red', 1);
expect(Object.fromEntries(labelsColorMap.chartsLabelsMap)).toEqual({});
});
+
+ it('should use set slice color scheme via setSliceOriginColorScheme', () => {
+ const labelsColorMap = getLabelsColorMap();
+ labelsColorMap.setSliceOriginColorScheme(1, 'testColors2');
+ labelsColorMap.addSlice('a', 'red', 1);
+ expect(labelsColorMap.chartsLabelsMap.get(1)?.scheme).toEqual(
+ 'testColors2',
+ );
+ });
+
+ it('should use given color scheme when not set via setSliceOriginColorScheme', () => {
+ const labelsColorMap = getLabelsColorMap();
+ labelsColorMap.setSliceOriginColorScheme(2, 'testColors2');
+ labelsColorMap.addSlice('a', 'red', 1, 'testColors');
+ expect(labelsColorMap.chartsLabelsMap.get(1)?.scheme).toEqual(
+ 'testColors',
+ );
+ });
});
describe('.remove(sliceId)', () => {
diff --git a/superset-frontend/src/dashboard/actions/dashboardState.js b/superset-frontend/src/dashboard/actions/dashboardState.js
index 10fcf518cb..1b70fe73f2 100644
--- a/superset-frontend/src/dashboard/actions/dashboardState.js
+++ b/superset-frontend/src/dashboard/actions/dashboardState.js
@@ -595,6 +595,7 @@ export function addSliceToDashboard(id) {
dispatch(fetchDatasourceMetadata(form_data.datasource)),
]).then(() => {
dispatch(addSlice(selectedSlice));
+ getLabelsColorMap().setSliceOriginColorScheme(id, form_data.color_scheme);
});
};
}
@@ -686,62 +687,73 @@ const updateDashboardMetadata = async (id, metadata, dispatch) => {
dispatch(dashboardInfoChanged({ metadata }));
};
-export const updateDashboardLabelsColor = () => async (dispatch, getState) => {
- const {
- dashboardInfo: { id, metadata },
- } = getState();
- const categoricalSchemes = getCategoricalSchemeRegistry();
- const colorScheme = metadata?.color_scheme;
- const colorSchemeRegistry = categoricalSchemes.get(
- metadata?.color_scheme,
- true,
- );
- const defaultScheme = categoricalSchemes.defaultKey;
- const fallbackScheme = defaultScheme?.toString() || 'supersetColors';
- const colorSchemeDomain = metadata?.color_scheme_domain || [];
-
- try {
- const updatedMetadata = { ...metadata };
- let updatedScheme = metadata?.color_scheme;
-
- // Color scheme does not exist anymore, fallback to default
- if (colorScheme && !colorSchemeRegistry) {
- updatedScheme = fallbackScheme;
- updatedMetadata.color_scheme = updatedScheme;
- updatedMetadata.color_scheme_domain = getColorSchemeDomain(colorScheme);
-
- dispatch(setColorScheme(updatedScheme));
- // must re-apply colors from fresh labels color map
- applyColors(updatedMetadata, true);
- }
+export const updateDashboardLabelsColor =
+ charts => async (dispatch, getState) => {
+ const {
+ dashboardInfo: { id, metadata },
+ } = getState();
+ const categoricalSchemes = getCategoricalSchemeRegistry();
+ const sharedColorMapInstance = getLabelsColorMap();
+ const colorScheme = metadata?.color_scheme;
+ const colorSchemeRegistry = categoricalSchemes.get(
+ metadata?.color_scheme,
+ true,
+ );
+ const defaultScheme = categoricalSchemes.defaultKey;
+ const fallbackScheme = defaultScheme?.toString() || 'supersetColors';
+ const colorSchemeDomain = metadata?.color_scheme_domain || [];
+
+ try {
+ const updatedMetadata = { ...metadata };
+ let updatedScheme = metadata?.color_scheme;
+
+ // track original color scheme for each chart
+ charts.forEach(chart =>
+ sharedColorMapInstance.setSliceOriginColorScheme(
+ chart.id,
+ chart.form_data?.color_scheme,
+ ),
+ );
- // stored labels color map and applied might differ
- // new data may appear in the map (data changes)
- // or new slices may appear while changing tabs
- const isMapSynced = isLabelsColorMapSynced(metadata);
- if (!isMapSynced) {
- // re-apply a fresh labels color map
- applyColors(updatedMetadata, false, true);
- // pull and store the just applied labels color map
- updatedMetadata.shared_label_colors = getLabelsColorMapEntries();
- }
+ // Color scheme does not exist anymore, fallback to default
+ if (colorScheme && !colorSchemeRegistry) {
+ updatedScheme = fallbackScheme;
+ updatedMetadata.color_scheme = updatedScheme;
+ updatedMetadata.color_scheme_domain = getColorSchemeDomain(colorScheme);
- // the stored color domain registry and fresh might differ at this point
- const freshColorSchemeDomain = getColorSchemeDomain(colorScheme);
- const isRegistrySynced =
- colorSchemeDomain.toString() === freshColorSchemeDomain.toString();
+ dispatch(setColorScheme(updatedScheme));
+ // must re-apply colors from fresh labels color map
+ applyColors(updatedMetadata, true);
+ }
- if (colorScheme && !isRegistrySynced) {
- updatedMetadata.color_scheme_domain = freshColorSchemeDomain;
- }
+ // stored labels color map and applied might differ
+ // new data may appear in the map (data changes)
+ // or new slices may appear while changing tabs
+ // requires merging the stored and applied maps
+ const isMapSynced = isLabelsColorMapSynced(metadata);
+ if (!isMapSynced) {
+ // re-apply a fresh labels color map
+ applyColors(updatedMetadata, false, true);
+ // pull and store the just applied labels color map
+ updatedMetadata.shared_label_colors = getLabelsColorMapEntries();
+ }
- if (
- (colorScheme && (!colorSchemeRegistry || !isRegistrySynced)) ||
- !isMapSynced
- ) {
- await updateDashboardMetadata(id, updatedMetadata, dispatch);
+ // the stored color domain registry and fresh might differ at this point
+ const freshColorSchemeDomain = getColorSchemeDomain(colorScheme);
+ const isRegistrySynced =
+ colorSchemeDomain.toString() === freshColorSchemeDomain.toString();
+
+ if (colorScheme && !isRegistrySynced) {
+ updatedMetadata.color_scheme_domain = freshColorSchemeDomain;
+ }
+
+ if (
+ (colorScheme && (!colorSchemeRegistry || !isRegistrySynced)) ||
+ !isMapSynced
+ ) {
+ await updateDashboardMetadata(id, updatedMetadata, dispatch);
+ }
+ } catch (error) {
+ console.error('Failed to update dashboard color settings:', error);
}
- } catch (error) {
- console.error('Failed to update dashboard color settings:', error);
- }
-};
+ };
diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx
index 7c27a9f1f7..53be05eecd 100644
--- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx
+++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx
@@ -31,6 +31,7 @@ import { pick } from 'lodash';
import Tabs from 'src/components/Tabs';
import DashboardGrid from 'src/dashboard/containers/DashboardGrid';
import {
+ Chart,
DashboardInfo,
DashboardLayout,
LayoutItem,
@@ -85,8 +86,13 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
const directPathToChild = useSelector<RootState, string[]>(
state => state.dashboardState.directPathToChild,
);
- const chartIds = useSelector<RootState, number[]>(state =>
- Object.values(state.charts).map(chart => chart.id),
+ const charts = useSelector<RootState, Chart[]>(state =>
+ Object.values(state.charts),
+ );
+
+ const chartIds: number[] = useMemo(
+ () => charts.map(chart => chart.id),
+ [charts],
);
const prevTabIndexRef = useRef();
@@ -144,9 +150,11 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
// verify freshness of color map on tab change
// and when loading for first time
setTimeout(() => {
- dispatch(updateDashboardLabelsColor());
+ if (charts.length) {
+ dispatch(updateDashboardLabelsColor(charts));
+ }
}, 500);
- }, [directPathToChild, dispatch]);
+ }, [directPathToChild, dispatch, charts]);
useEffect(() => {
const labelsColorMap = getLabelsColorMap();
diff --git a/superset-frontend/src/utils/colorScheme.ts b/superset-frontend/src/utils/colorScheme.ts
index a3da1427c7..03cd03d266 100644
--- a/superset-frontend/src/utils/colorScheme.ts
+++ b/superset-frontend/src/utils/colorScheme.ts
@@ -115,18 +115,10 @@ export const applyColors = (
const colorNameSpace = getColorNamespace(metadata?.color_namespace);
const categoricalNamespace =
CategoricalColorNamespace.getNamespace(colorNameSpace);
- let colorScheme = metadata?.color_scheme;
+ const colorScheme = metadata?.color_scheme;
const customLabelColors = metadata?.label_colors || {};
- // when scheme unset, update only custom label colors
const labelsColorMap = metadata?.shared_label_colors || {};
- if (!colorScheme) {
- // a fallback color scheme must be set to generate shared label colors
- const categoricalSchemes = getCategoricalSchemeRegistry();
- colorScheme =
- categoricalSchemes.getDefaultKey()?.toString() ?? 'supersetColors';
- }
-
if (fresh) {
// reset forced colors (custom labels + labels color map)
categoricalNamespace.resetColors();