change osmEntity.key() from a static method to a instance method#12212
change osmEntity.key() from a static method to a instance method#12212
osmEntity.key() from a static method to a instance method#12212Conversation
| // Avoid exit/enter if we're just moving stuff around. | ||
| // The node will get a new version but we only need to run the update selection. | ||
| function fastEntityKey(d) { | ||
| if (typeof d === 'number') return d; |
There was a problem hiding this comment.
i had to add this line, because there was an minor "bug": this function was being passed a number sometimes, which caused osmEntity.key() to return "undefinedv0".
same reason for adding the instanceof guard added in the file above.
There was a problem hiding this comment.
this function was being passed a number sometimes
I tried to find out how/when this would be the case, but could not find any reason why it would ever be anything else than an osm entity. Any hints about how to trigger this "bug"?
There was a problem hiding this comment.
it matches every other class method
I believe that the method was implemented as a static method on purpose to make using it with d3's .data easier: .data(…, osmEntity.key). I'm actually not sure if it is better suited as a instance method or for as a static function somewhere. Speaking of it: I see that this method is currently also present in the new osmIdManager, but never used. Maybe it can also live there (either by renaming the manager to osmEntityManager to reflect the more generic usage, or factoring it out to a simple exported function getOsmEntityD3Key that only does that)? Anyway, one of the two methods should be removed.
Another thing: The naming of this method as key was IMHO quite unfortunate, as the association with the OSM term "tag key" is potentially confusing. Here it is not as quite as bad, but there are some other modules (e.g. ui/fields/combo.js) where it is super perplexing that the key property is assigned to a tag value: when in half of that line, key is meant to be an index for d3, and in the other half on is talking about key-values pairs of a tag. 🤷
As we're touching all of the occurrences of this method here anyway: what do you think about renaming this to something a little less confusing. Maybe index could work? Or d3_key??
| // Avoid exit/enter if we're just moving stuff around. | ||
| // The node will get a new version but we only need to run the update selection. | ||
| function fastEntityKey(d) { | ||
| if (typeof d === 'number') return d; |
There was a problem hiding this comment.
this function was being passed a number sometimes
I tried to find out how/when this would be the case, but could not find any reason why it would ever be anything else than an osm entity. Any hints about how to trigger this "bug"?
|
small ping @k-yle ⬆️ 😊 |
|
sorry, i missed this notification amongst the dependabot spam 😆 I've simplified this to use the identical method from |
2 participants
part 7 of #10909.
this is the last PR for the preëmptive work ! after that is should be easy to convert
osmEntityit to an ES6 Class. i might try this tonight.in this PR: we replace a static function
osmEntity.key(...)with a class method, so that:osmEntityis as many files