emit event on appended element on dom

[Carreau]

Start working on #4747,

I'm wondering if we shouldn't have the this['append_'+type](json[type], md, element) return (a reference to) the inner element it happends to
pass it to the event.

I'm also wondering if I should pass the mimetype as a parameter of the
event, or dynamically create the event-name based on the mime-type.

Last possibility is that each of the append_xxx(xxx, md, element)
(except JS) seem to end with element.append(toinsert) we could hve
them return the toinsert and move element.append(toinsert) to append mimetype.

Thoughts ?

[Carreau]

(haha this pull request is 4774 and is suppose to fixed 4747)

[minrk]

just a note that this should probably wait for #4533 since it touches some of the same code, and it would be easier to rebase this on that rather than the other way around, in case there is a conflict.

[ellisonbg]

This is a minor change, but I don't think we should add events unless there is a clear, broad need for them. I would like to better understand the actual usage case first. @gibiansky

[minrk]

@ellisonbg is there a significant cost to events? I think we should definitely air on the side of more events rather than fewer, unless there is a clear cost.

[minrk]

For instance, this event would be useful in the test suite. I have found while writing js tests that I want many more events in our js than we have.

[Carreau]

Rebased, but apparently travis is not running the tests.

[minrk]

semicolons!

[ellisonbg]

Comments:

• Let's move all of the OutputArea class dictionaries (such as append_map) to the same place in the code to make it easier to see all of this stuff.
• append_javascript should return element not container to be consistent with other methods. We could rename container->element and element->toinsert in append_javascript to be fully consistent with other append_* methods, but users are still calling container.show() in older JS code. The container.show() is no longer needed.

[Carreau]

updated to take comment into account.
Only other change is in append_javascript, that does not return outer, but inner div to conform to others methods.

The container object still exist but is bound to a phone .show() method that print a warning in the js console.

[Carreau]

Yes I know I will uncommit this change.

[Carreau]

Fixed, rebase, whats new and put all class properties in one section just after constructor.

[minrk]

failures look real, can you check?

[ellisonbg]

I agree that the tests failures look real.

[Carreau]

one of the error was that

 OutputArea.append_map = {
        "text/plain" : OutputArea.prototype.append_text,
        ...
    };

Was at the beginning hence the map was registering undefined.
BTW, written like that append_map does a copy so you cannot redefine prototypes on the fly.
Will open an issue.

[minrk]

These probably shouldn't be prototype methods at all. Overriding append behavior should be done with OutputArea.append_map, not through prototype override. So there's no problem that prototype overrides don't work.

[takluyver]

Tests are passing now - does anyone want to review this any further?

[ellisonbg]

I think this looks good, but @minrk mentioned wanting the append_map to be non prototypes? Why is that? I think we do want them to be prototypes so they can be easily bound to this using apply. Other than that I am +1 on merge.

[minrk]

The prototype conversation is unrelated to this PR. Merging.

[minrk]

append_map is a class attribute, mapping mime-type keys to functions. To change the append behavior, you register new functions in this mapping, not by overriding prototype methods. That's all I meant by it.

So I wasn't saying that these shouldn't be prototype methods, only that it shouldn't matter whether they are or not, since the general behavior of prototyping is irrelevant.