bikash-barnwal commented on PR #43180:
URL: https://github.com/apache/superset/pull/43180#issuecomment-5550412990
Good question — this is the intended trade-off, not an oversight, for a
mechanical reason: proactive reservation is required here, not optional. The
whole reason this bug existed is that the post-hoc collision-repair path (walk
`chartLabelsColorMap`, reassign the losing label) can't retroactively fix a
color that was **already returned to the caller** — by the time Playstation's
later resolution detects PC's collision and reassigns PC in the maps, the chart
has already been handed PC's original (colliding) color synchronously from that
earlier `getColor('PC')` call. So the fix has to prevent the collision *before*
handing back a color, not clean it up after.
And at the moment a brand-new label is being resolved, there's no cheap way
to know in advance which *other* shared-map labels will turn out to belong to
this same chart later in the same render — `chartsLabelsMap` for this sliceId
only reflects labels this exact chart has *already* resolved (this render or a
past one), not ones still to come. In the `PC → Playstation → Xbox` test,
Playstation/Xbox aren't yet recorded under chart2's sliceId when PC resolves,
so scoping reservation to "this sliceId's already-known labels" would miss
exactly the case the test pins down. Any signal precise enough to say
"Playstation belongs to chart2 specifically, unlike chart5's Xbox" isn't
available at that point — only the global shared map is.
Given that, treating the whole shared map as reserved is the only proactive
option that actually closes the bug, and it's arguably consistent with what
`map_label_colors`/dashboard-scoped coloring is already for — one label, one
color, dashboard-wide, while the palette lasts. It only changes anything while
the palette still has unused colors; once exhausted it falls back to the
pre-existing least-used-color behavior, same as before this PR for large label
sets. Happy to add a code comment on `getReservedColors` spelling this out if
that helps future readers — let me know if you'd like that, or if you see a
cheaper way to get the finer scoping I might be missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
