Invalidate TileSource cache on property changes #8594
Conversation
philippjfr
added
type: bug
status: ready
tag: component: bokehjs
and removed
tag: component: bokehjs
type: bug
labels
This is true AFAIK @mattpap can comment if I am mistaken |
| @@ -52,6 +52,7 @@ export class TileRendererView extends RendererView { | |||
| connect_signals(): void { | |||
| super.connect_signals() | |||
| this.connect(this.model.change, () => this.request_render()) | |||
| this.connect(this.model.tile_source.change, () => this.request_render()) | |||
There was a problem hiding this comment.
So, what happens here if the tile source itself is swapped out?
There was a problem hiding this comment.
Ah, in that case I guess it should unsubscribe from this event, and subscribe to the new tile source. Is that handled on the GlyphRenderer when swapping out the Glyph? I can't spot where that's done.
There was a problem hiding this comment.
Hrm I am not actually sure it is handled currently though I thought it was in the past. I would be OK with merging this and making a follow on issue to investigate more thorough signal disconnect/reconnect (in general, for glyphs and this and anything else)
There was a problem hiding this comment.
+1
Would be neat if the property could detect a model was being replaced and then dis- and reconnect any signals that were attached to the model being replaced, instead of having to handle this reconnecting logic all over the place. I don't know enough to determine whether that's feasible though.
| x_origin_offset: p.Property<number> | ||
| y_origin_offset: p.Property<number> | ||
| initial_resolution: p.Property<number> | ||
| } |
There was a problem hiding this comment.
Let me know if my assumption that attributes need to be properties to trigger model change events is wrong.
Yes and no. If you want automation, then yes. You may try to use setv() directly, but that will fail, if "properties" weren't defined. You can, however, emit change signal manually if you want (e.g. from a setter). I'm writing this in the context of the above change, because it isn't necessary in this PR (though useful in general). From runtime perspective only this.define(...) matters. Also note that define() is untyped currently, so you can put whatever you want there (which obviously I don't recommend).
There was a problem hiding this comment.
Thanks for the clarification, I think in this case it is needed because we want the model to be responsive to any change events, no matter how they were triggered.
There was a problem hiding this comment.
Sure, the default is to use properties, especially as the part of the API. However, I just want to make sure that it's understood that the code above doesn't have any run time implications. Any interfaces or types (or anything else that is part of TypeScript and not JavaScript) are fully erased at compilation time. The only thing that matters at runtime is define(), and that was fully specified already. But as I said, it doesn't harm to add this code (in fact I have a WIP PR that fills all those in), just I want to make sure that be benefits and consequences of such change are fully understood.
There was a problem hiding this comment.
Ah okay, I did misunderstand you initially then. Thanks for clarifying, the only thing I'm missing is why attributes and properties seem to be duplicating the same type information. Wouldn't it be sufficient to declare them once as either attributes or as properties?
There was a problem hiding this comment.
Wouldn't it be sufficient to declare them once as either attributes or as properties?
Yes (*), however, limitations of the language when we started the transition didn't allow for this. Currently I can use mapped and conditional types, to generate attributes from properties (this direction, because properties carry more information than attributes). I have a WIP PR that fixes that (after layout). However, (*) attributes and properties may, and in fact do in case of specs, carry different types, or even input (constructor) and output (model interface) types may differ. Altogether this can get pretty tricky, but with some type-level magic it can be handled without duplication.
There was a problem hiding this comment.
Wouldn't it be sufficient to declare them once as either attributes or as properties?
Just to add more color: the duplication is entirely because we developed a properties system in coffee script, if we had had the current version of typescript at the time we probably could have done else. Now we have the task of unifying our old sort-of-types with TypieScript's actual language capabilities, and that is an onerous task.
There was a problem hiding this comment.
Thanks for the background, I really appreciate it.
|
@bryevdv Should we merge this or do you want me to take some action on the tile source replacement issue? |
|
No I will merge now, please make a follow up issue about the other. |
Upgrades
TileSourceattributes to properties and add events to clear cache and rerender when a property changes. Let me know if my assumption that attributes need to be properties to trigger model change events is wrong.