Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Removing Kapi.prototype.redraw #25

Closed
jeremyckahn opened this Issue · 8 comments

2 participants

@jeremyckahn
Owner

@sork: I think we can remove Kapi.prototype.redraw in the Canvas extension. Because it only applies to CanvasActors, I think that it's a leaky abstraction that may confuse users. I recently made a change to Kapi.prototype.update to accept no arguments, allowing a recalculation and render. It's slightly slower than redraw, but I think it will simplify the API. Thoughts?

@sork
  1. I think it's good to consolidate and simplify the API in this way. I have no issue with the removal of Kapi.prototype.redraw

  2. I agree that it's kind of a leaky abstraction, but that's also the case for other methods such as: moveActorToLayer, setOrderFunction, canvasHeight, canvasWidth etc.

    If we want a generic Rekapi object with the same API whatever extensions are currently loaded, I think we would need to prevent extensions from augmenting of the Kapi prototype with additional methods, and provide them somewhere else. To be clear, I'm not really advocating for this, just trying to highlight some inconsistencies. For example, I'm not sure why the presence of setOrderFunction (which only applies to CanvasActors too) doesn't seem to bother you whereas redraw does.

@jeremyckahn
Owner

You raise a good point about methods such as setOrderFunction. I wonder if it makes sense to subclass the Kapi Object like we do for Kapi.Actor. So, we would have Obejcts like CanvasKapi and DOMKapi. My initial thought is that it makes the library overly object-oriented and would make it too complex, but I'm open to discussing it.

Another option is to prepend canvas to the Kapi.prototype methods that are being added to it (making methods like canvasSetOrderFunction), but I think that's even worse than my first idea.

A third idea is to adopt a sort of mixin pattern for Kapi instances that require it. For example, we already inject functionality for when Kapi.CanvasActors are added, so we could also attach the appropriate Canvas Kapi.prototype extensions to the Kapi instance at that time. That approach makes the most sense of ones I've listed, but it seems a little hacky, and still leaky. @sork, what do you think? Are there any other approaches you think we could take to minimize unnecessary Kapi.prototype extensions?

@jeremyckahn
Owner

Okay, I removed redraw in 818a438.

I'd like to keep this discussion open for the moment, until we figure out what to do with the Canvas methods.

@sork

Sub-classing is not a good idea because as I mentioned in previous discussions, it's very useful to be able to animate different kinds of actors with the same Rekapi instance. Let's not remove that feature, since web apps often need to mix different rendering techniques (e.g. canvas + DOM elements). It would be cumbersome to have to instantiate multiple Rekapi objects.

Your second suggestion of prefixing methods with the extension name makes more sense in my opinion, as it's already the case with canvasClear, canvasHeight, canvasWidth, etc. However I agree with you that's it not ideal and it somehow doesn't feel right to me.

I don't really like your third suggestion (unless I'm misunderstanding you) because the Kapi instance would have an inconsistent APi depending on whether it's used before or after attaching CanvasActors. I'm also not sure how it solves the problem of having extension-specific methods in an ideally generic object.

To be honest, I don't really have a good suggestion to make at this time. If we take jQuery as an example, plugins are added in a similar way as current Rekapi extension methods are, i.e. by augmenting the $.fn prototype.

@jeremyckahn
Owner

I spent some time thinking about it, and I have a new idea. I think it would makes sense from a user's perspective if canvas-specific methods were accessed via a kapiInstance.canvas namespace. So, setOrderFunction would be accessed as kapi.canvas.setOrderFunction.

To do achieve this, we'd have to create a new Object (let's call it CanvasContext) and attach a new instance of it to the Kapi instance with the _contextInitHook.

We could repeat this pattern for all of the renderers. It'll be a little weird to implement, but I think it makes the API a little clearer and more scalable. What do you think of this approach, @sork?

@sork

Sorry for the slow reply.

I really like your suggestion. I think it makes a lot of sense to have these methods in a separate namespace. I had a similar idea but I wasn't sure how it could fit within the current code so I didn't go further with it. Your implementation proposal sounds good to me!

@jeremyckahn
Owner

Cool! I will close this issue for now, and hopefully get started on the change this weekend. Thanks for your input!

@jeremyckahn
Owner

Ok, the API change has been implemented in da6a764. Specific differences are documented in the upgrade guide, as usual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.