Allow a RenderContext to be passed directly to the Renderer constructor #1161
Conversation
|
Is this suggestion related to your PR? Maybe someday there can be a |
|
Yeah, the suggestion in that issue is similar but this PR bypasses the need for any kind of registration by simply allowing you to pass in any object that fulfills the |
|
Awesome. After your PR is merged we can close that issue. The commenter referred to D3. It would be fun to have a D3 renderer, haha. One could make a fancy zooming visualization of multiple scores or rearrange the measures of a score or something. :-) |
|
This PR also fixes #724 It's good to see multiple devs asking for the same thing. Thanks for finding the solution! |
|
Looks great Tom. Can you fix the conflicts please? Also, update the wiki (and FAQ), since this is particularly useful. |
src/factory.ts
Outdated
| const { elementId, width, height, background } = this.options.renderer; | ||
| if (elementId == null || elementId === '') { | ||
| if (elementId === null) { |
There was a problem hiding this comment.
Does this need to handle undefined? The previous solution did, although not explicitly.
There was a problem hiding this comment.
The change history around these lines is somewhat complicated but I think the elementId === null change came from a different PR that I merged in. Regardless, I think you're correct and the more lax check is preferred.
| @@ -9,29 +9,16 @@ import { RuntimeError } from './util'; | |||
| // A ContextBuilder is either Renderer.getSVGContext or Renderer.getCanvasContext. | |||
| export type ContextBuilder = typeof Renderer.getSVGContext | typeof Renderer.getCanvasContext; | |||
|
|
|||
| // eslint-disable-next-line | |||
| function isRenderContext(obj: any): obj is RenderContext { | |||
There was a problem hiding this comment.
Is this our type guard? Seems like a funny way to check if it's a render context, but if it works, it works.
There was a problem hiding this comment.
It is our type guard unfortunately. By design, there's a big overlap between the interfaces in RenderContext and `CanvasRenderingContext2D', so this is the most robust test I can think of.
I think a better fix would be to make RenderContext an abstract base class, but that's a fairly big change that I'd like to make in isolation.
| constructor(arg0: string | HTMLCanvasElement | HTMLDivElement | RenderContext, arg1?: number) { | ||
| if (isRenderContext(arg0)) { | ||
| // The user has provided what looks like a RenderContext, let's just use it. | ||
| // TODO(tommadams): RenderContext is an interface, can we introduce a context base class |
There was a problem hiding this comment.
I wouldn't mind having an abstract base RenderContext class that people are required to extend.
Alternatively, we go the isCategory route (like our type guard file) where if you have a static property CATEGORY that returns 'RenderContext', we will trust that you know what you're doing.
@0xfe any input on this? I'd prefer an abstract class that both SVGContext and CanvasContext extend from.
src/renderer.ts
Outdated
| } | ||
| this.ctx = new CanvasContext(context); | ||
| } else if (backend === Renderer.Backends.SVG) { | ||
| if (!(element instanceof HTMLDivElement)) { |
There was a problem hiding this comment.
How come we use window.HTMLCanvasElement above, but here we use HTMLDivElement?
Should we drop the window. prefix, for situations where window is not defined, but those types are somehow available in the globalThis scope?
There was a problem hiding this comment.
(I saw your commit comment: "window.HTMLCanvasElement so code works under node.js").
Does jsdom / node-canvas have window.HTMLCanvasElement, but not HTMLCanvasElement? How come HTMLDivElement works?
There was a problem hiding this comment.
I played around with jsdom. We can assume that HTMLCanvasElement and HTMLDivElement are globally available IF the developer does this in node:
const dom = new JSDOM(`<!DOCTYPE html><body></body>`);
global.window = dom.window;
global.document = dom.window.document;
global.HTMLCanvasElement = dom.window.HTMLCanvasElement;
global.HTMLDivElement = dom.window.HTMLDivElement;
Then these will log true:
const d = document.createElement('div');
console.log(d instanceof HTMLDivElement);
const c = document.createElement('canvas');
console.log(c instanceof HTMLCanvasElement);
So I guess it's safest if we add the window. prefix to all our instanceof checks, e.g., instanceof window.HTMLDivElement.
Or perhaps we should expect the developer to make these two types globally available? We could document this in our examples and on the wiki.
There was a problem hiding this comment.
(I saw your commit comment: "window.HTMLCanvasElement so code works under node.js").
Does jsdom / node-canvas have
window.HTMLCanvasElement, but notHTMLCanvasElement? How comeHTMLDivElementworks?
Thanks for the catch, I missed updating HTMLDivElement, it doesn't actually work :)
I only caught the HTMLCanvasElement because it failed when I was ran the visual regression test and that (of course) doesn't exercise any of the SVG code paths.
|
I believe this all works, but could we get an example file that is a hello world demonstration of this? You can drop it in vexflow/demos/ and it can be a stub RenderContext that just console.logs the method name and params. It can illustrate to others that it is possible to implement your own RenderContext. |
|
@tommadams if you write "fixes #827" and "fixes #724" in the first comment of this PR like in #1170 , the issues and the PR will be connected and when the PR is merged, the issues will be closed automatically. |
|
Thanks for all the great comments folks, and sorry for the delayed response. I'm going get cracking on all the suggestions today. |
|
I've added a simple example of a custom |
|
It's my bedtime now, I'll update the wiki tomorrow :) |
|
Looks good, Tom. Agreed with creating a base abstraction here, and doing it separately. |
fixes #827
fixes #724
This enables users to create their own
RenderContextimplementations.The required a couple of changes:
CanvasContextresize logic out ofRendererand intoCanvasContext.Rendererconstructor to accept aRenderContext.This PR also fixes a bug in the
Factorywhere it would always create anSVGContexteven if you passed it the element ID for aHTMLCanvasElement. Consequently, there are a few image diffs because theFactory.Drawtest now actually renders something for the canvas test :)