visualizations emit 'renderComplete' event when done rendering #8914

ppisljar · 2016-11-01T14:15:29Z

Vis emits 'renderComplete' event once its done rendering. This allows reporting to know when to make screenshots.

inside your visualization you should $scope.vis.emit('renderComplete') when rendering is done
in VisType definition you should set implementRenderComplete property to true
You can check if visualization implements this event with vis.implementsRenderComplete()
You can listen for renderComplete event with vis.onRenderComplete(callback)

the event will also be emitted in case:

esRequest failed
error occurred while rendering visualization (like container too small or no data)

ppisljar · 2016-11-01T17:59:19Z

src/ui/public/template_vis_type/template_vis_type.js

@@ -15,8 +15,8 @@ export default function TemplateVisTypeFactory(Private) {
    }
  }

-  TemplateVisType.prototype.createRenderbot = function (vis, $el, uiState) {
-    return new TemplateRenderbot(vis, $el, uiState);
+  TemplateVisType.prototype.createRenderbot = function (vis, $el, uiState, doneCallback) {


or should we go for promises ?

would that work? createRenderBot is only called at setup time.

you are right

thomasneirynck

I like this change overall. It's pretty clear at first glance, and user-friendly.

I haven't tested this yet with the Reporting-module (so we should do that ;)).
I used this API in the Tagcloud-proposal for now Tagcloud #8104).
The rendered events are not fired when there is a change in the vis.params (and not the data). I don't think that is a problem in light of its purpose (supporting Reported), but this event (or something similar) should fire for any change in the visualization. This is also just a problem with the current implementations of e.g. MetricVis, that doesn't handle options changes explicitly but lets angular handle them transparently.
Generally speaking, I think we should try and divorce this event from angular. But i guess that's long term. A specific issue right now with this approach is that we use the scope both to propagate model-changes (data, options), but also for the view to publish changes. That feels a little 'crooked', as scope should really only be the model-state. I think we can justify this as long as it is well documented an its use is only internal. But an event like this belongs more in the "view" (e.g. as an event of TemplateVisType).
I am not sure if 'rendered' should be an event to begin with. Essentially, we want a hook for a client to know when a visualization is ready. But the ready-state of a visualization also depends on the model-state and any other changes to the view. So we want a hook for when a visualization is ready for a particular configuration. From a client-perspective, when using events, you get into the complexity where you have to unhook your existing listeners if there are any state changes (since you might respond to events from older states). Another approach may be giving the visualizations a
whenReady method that returns a promise. When this promise resolves, the visualization is ready at the moment it resolves and there are no outstanding model/config changes that are still being resolved.

I did found it easy to use in Tagcloud, but I think this API probably won't work long-term. So I guess we need to work out if this goes in as a sort of temporary improvement.

thomasneirynck · 2016-11-02T04:34:58Z

src/ui/public/template_vis_type/template_vis_type.js

@@ -15,8 +15,8 @@ export default function TemplateVisTypeFactory(Private) {
    }
  }

-  TemplateVisType.prototype.createRenderbot = function (vis, $el, uiState) {
-    return new TemplateRenderbot(vis, $el, uiState);
+  TemplateVisType.prototype.createRenderbot = function (vis, $el, uiState, doneCallback) {


would that work? createRenderBot is only called at setup time.

ppisljar · 2016-11-02T08:07:37Z

for reporting it wont yet work, there is still an open PR for this, which needs to be updated to address this new way (the old way was really bad anyway :) )
i agree, the event should be fired every time the visualization updates. and i also agree that its just a problem with implementations of current visualizations. (should probably be quickly fixed in this pr by just watching for changes to vis.params as well.)
i dont think i really understand your point here so lets catch up on zoom. i agree we should go away from angular dependency for this, so instead of using scope we could have another object which would do pretty much the same. allow us to listen for updates to data and config and allow us to emit events like 'rendered'
dont understand the last point completely either. in this implementation we already use a hook (callback ... or do you mean something else with hook?) with which renderbot notifies us that rendering was done. however renderbot uses scope to get event from the plugin ... you mean we should just pass in the callback to plugins ? lets catch up on zoom and discuss this

ppisljar · 2016-11-02T10:56:05Z

i updated it to use SimpleEmitter (its using this one in vislib) in the TemplateVisType (instead of relying on angular $scope)

ppisljar · 2016-11-02T13:26:43Z

jenkins, test this

trevan · 2016-11-03T14:20:53Z

There are quite a few plugins that have visualization addons. It would be nice if this functionality did not require the plugin authors to add it into the plugin.

thomasneirynck · 2016-11-03T17:02:51Z

src/ui/public/template_vis_type/template_renderbot.js

@@ -11,6 +11,10 @@ export default function TemplateRenderbotFactory(Private, $compile, $rootScope)
    this.$scope.vis = vis;
    this.$scope.uiState = uiState;

+    if (this.vis.listeners.renderComplete) {


I don't like these instance checks. We should give all visualizations the renderComplete API.

thomasneirynck

I like the simple publishing of an event on the scope (although perhaps we could leave the hook in vis/vis and have Reporting hook into it directly).

I would propose using simple pub/sub with emit/on iso. having clients conditionally overwrite a method on .listeners.

thomasneirynck · 2016-11-03T17:05:42Z

src/ui/public/visualize/visualize.js

+        if (vis) {
+          vis.listeners.renderComplete = function () {
+            $scope.$emit('renderComplete');
+          };


We shouldn't have an external component be responsible for implementing functionality of the vis-type. We should try and decouple pub/sub.

its not necessary to define this function, if component using Vis needs it it can define it ...

thomasneirynck · 2016-11-03T17:33:37Z

src/ui/public/vis/vis_type.js

+  VisType.prototype.createRenderbot = function (vis, $el, uiState) {
+    throw new Error('not implemented');
+  };
+
  return VisType;
 };


I am not a 100% sure if we need to broker this with the renderbot at all, but maybe I miss something major.

Since every externally accessible visualization (instance of vis) needs to publish a 'renderComplete' event, I would propose modeling on the Vis-level.

E.g.:

We make ui/vis/vis support pub/sub (perhaps do the same for ui/vis/vis_type in fact)

function Vis(indexPattern, state, uiState) { EventEmitter.call(this); ... } Vis.prototype = Object.create(EventEmitter.prototype);

visualization implementations publish their readiness (templated vis, vislib vis)

e.g. in metric_vis_controller, (but also table_vis_controller, `vislib, ...)

$scope.processTableGroups(tabifyAggResponse($scope.vis, resp)); $scope.vis.emit('renderComplete'); }

listeners can just subscribe to this events
e.g. in ui/visualize/visualize, (but also Reporting...)

if (oldVis) $scope.renderbot = null; if (vis) { vis.on('renderComplete', () => { //...here we could propage on the correct scope with $scope.$emit('renderComplete'); // ...but perhaps Reporting could just listen to it directly }); }

I think this will have several advantages:

the implementation remains a little cleaner

we don't have clients conditionally implement a method. They can just use on.

publishers can just use emit() to publish, and don't have to worry if there is a registered listener or not

Thinking longer ahead, this will be easier to document in JSDoc as well since @events are a core-type.

regarding VisType.prototype.createRenderbot ... VisType must implement this method, thats why i added an implementation to the base type which will throw an error if this method is not implemented on vis subtype.

Regarding Vis extending EventEmitter ... i am generally for that. But i don't feel that having every new functionalty achieve things in different way is a good way forward. The listeners object is currently used to hook to events in vis ... so i followed the same pattern. If we would change that i would be for changing that for all the events.

i agree implementation would be cleaner (template_renderbot doesnt need to do anything)

but nothing much would change for publishers (they can emit now as well, and even in this case with no registered listener nothing happens ...) so nothing changes for them

clients (like visualize) would still need to define a listener (setting an object property or calling on doesnt make that much of a difference)

i think the biggest benefit of this would be not having to rely on angular (which i hear we are moving away from)

@spalger do you maybe know why current implementation is using listeners object with functions which are then attached to events instead of just having vis extend EventEmitter ?

another thing i would like to understand is the use of all the different event objects (utils/simple_emitter, public/events) ...

do you maybe know why current implementation is using listeners object with functions which are then attached to events instead of just having vis extend EventEmitter ?

I'm 99% sure that is was just the way that the original implementor wanted to do it. The listeners object though was intended to be a shortcut for situations like this but I totally agree that it would be best to have one way of doing that (and I prefer vis.on(...))

i would like to understand is the use of all the different event objects

The SimpleEmitter class is a standard event emitter, very similar to the event emitter in node.js. The Events class in ui/events is an angular compatible version that wraps handlers in promises so that they properly trigger angular digest cycles and bind to the correct angular scope when listeners are added. If the vislib uses the SimpleEmitter then any angular consumer has to wrap their event handlers in $scope.apply() or something similar to ensure that an angular digest cycle is triggered after running, but if it uses the ui/events Events class then angular consumers will just be able to consume them like normal.

thomasneirynck · 2016-11-03T17:44:51Z

@trevan this change will not require external add-ons to implement this. We need this event to support screenshot functionality (e.g. for testing, reporting). But this is not planned to be a breaking change.

Over time, I do see a use for visualizations in Kibana having to advertise when they are done rendering (since there is no guarantee visualizations must render synchronously), but we're not at the point yet of making this an API requirement.

trevan · 2016-11-03T17:54:53Z

@thomasneirynck, but doesn't an external add-on that wants to work with reporting not need this change?

ppisljar · 2016-11-03T18:03:17Z

i agree with @trevan, 3rd party plugins will need to implement this in order to work with reporting.

thomasneirynck · 2016-11-03T18:08:28Z

@trevan yup, they would have to, but they don't necessarily work with Reporting right now either (all depends if they render async or not).

FWIW, Kibana needs a public API for Visualizations, one that is documented and stable across releases. One of the requirements of that API is that it must support visualizations that render async (e.g. they might have animations, render on a requestAnimationFrame-loop, etc....). But that's an effort down the road. This PR is addressing a specific issue right now, but we don't want to enforce it yet for external add-ons.

w33ble · 2016-11-03T20:08:27Z

src/core_plugins/markdown_vis/public/markdown_vis_controller.js

@@ -11,7 +11,11 @@ marked.setOptions({
 const module = uiModules.get('kibana/markdown_vis', ['kibana', 'ngSanitize']);
 module.controller('KbnMarkdownVisController', function ($scope) {
  $scope.$watch('vis.params.markdown', function (html) {
-    if (!html) return;
+    if (!html) {


Something like this would be cleaner:

if (html) { $scope.html = marked(html); } $scope.$emit('renderComplete');

Plus, then there's no potential of forgetting to emit if other conditions are added.

ppisljar · 2016-11-04T09:35:21Z

@thomasneirynck i think this is ready for another review.

ppisljar · 2016-11-04T09:38:11Z

src/ui/public/vislib/vis.js

+            this.render(this.data, this.uiState);
+          }
+        };
+        uiState.on('change', this._uiStateChangeHandler);


for some reason uiStateChangeHandler gets called 2-3 times AFTER vis.destroy() was called (which removes the event handler) when we open a new dashboard for example. multiple renderComplete would be thrown (for each render call) when opening a new dashboard. i was not able to identify why this is happening so this is a short term workaround (check if the element is still in DOM).

ppisljar · 2016-11-04T09:39:08Z

src/ui/public/vislib_vis_type/vislib_renderbot.js


-    self.vislibParams = self._getVislibParams();
-    self.vislibVis = new vislib.Vis(self.$el[0], self.vislibParams);
+    _.each(this.vis.listeners, (listener, event) => {


in vislib we still depend on listeners object. as this would require many changes i suggest to do it in a follow up PR.

ppisljar · 2016-11-04T09:40:26Z

src/ui/public/visualize/visualize.js

+        if (vis) {
+          vis.on('renderComplete', () => {
+            $scope.$emit('renderComplete');
+          });


@w33ble are you ok with removing emiting renderComplete on scope (so you would need to subscribe to $visualizeScope.vis.on instead of $visualizeScope.on ?

Yeah, that's fine with me. I just need something to listen to, I don't care what that something is.

w33ble · 2016-11-16T18:46:50Z

Latest changes, with the implementsRenderComplete() helper, look good.

However, I'm running into another problem. The visualization is pretty quick to render the vis on my machine, and the listener is attached after it's already done. I think wrapping up the listener in a helper as well, and firing it immediately if the event has already emitted, is the way to go. Something like visScope.vis.onRenderComplete(cbFunction), which could call cbFunction every time the event happens, and also immediately if it's happened in the past. What do you think of that?

w33ble · 2016-11-16T18:57:47Z

onRenderComplete works a treat! Below you can see resolveOnComplete resolve immediately when the listener is registered, because the visualization has already emitted renderComplete.

And here's an example using a Tilemap, where the tiles take a few seconds to download it delays the resolveOnComplete call.

Successfully generated a Dashboard with every vis type, and even removed the event from Timelion to test without the listener. Functionally LGTM!

w33ble · 2016-11-16T22:53:44Z

@thomasneirynck there's been a few small commits since you last looked at this I think. Once you've taken a look through them, and assuming you approve, let's get this merged.

thomasneirynck

LGTM

thomasneirynck · 2016-11-16T23:37:48Z

@w33ble Looked at the last API changes. Looks good. I like this approach.

w33ble · 2016-11-17T00:04:42Z

Cool. I think this is good to merge then. I'll let @ppisljar do the honors.

Backports PR #8914 **Commit 1:** visualize emits renderComplete event once its done rendering * Original sha: 2195692 * Authored by ppisljar <[email protected]> on 2016-11-04T09:32:29Z **Commit 2:** fixing test * Original sha: 97a390a * Authored by ppisljar <[email protected]> on 2016-11-04T09:48:23Z **Commit 3:** adding renderComplete event to timelion * Original sha: b833873 * Authored by ppisljar <[email protected]> on 2016-11-10T15:14:16Z **Commit 4:** adding renderComplete event to timelion * Original sha: ab94eef * Authored by ppisljar <[email protected]> on 2016-11-11T09:20:32Z **Commit 5:** adding implementsRenderComplete property to vis * Original sha: b2ff874 * Authored by ppisljar <[email protected]> on 2016-11-16T06:50:19Z **Commit 6:** adding helper method on vis * Original sha: fc78562 * Authored by ppisljar <[email protected]> on 2016-11-16T18:21:46Z **Commit 7:** adding onRenderComplete method * Original sha: 451ab9a * Authored by ppisljar <[email protected]> on 2016-11-16T18:48:48Z

…ic#9113) Backports PR elastic#8914 **Commit 1:** visualize emits renderComplete event once its done rendering * Original sha: 2195692 * Authored by ppisljar <[email protected]> on 2016-11-04T09:32:29Z **Commit 2:** fixing test * Original sha: 97a390a * Authored by ppisljar <[email protected]> on 2016-11-04T09:48:23Z **Commit 3:** adding renderComplete event to timelion * Original sha: b833873 * Authored by ppisljar <[email protected]> on 2016-11-10T15:14:16Z **Commit 4:** adding renderComplete event to timelion * Original sha: ab94eef * Authored by ppisljar <[email protected]> on 2016-11-11T09:20:32Z **Commit 5:** adding implementsRenderComplete property to vis * Original sha: b2ff874 * Authored by ppisljar <[email protected]> on 2016-11-16T06:50:19Z **Commit 6:** adding helper method on vis * Original sha: fc78562 * Authored by ppisljar <[email protected]> on 2016-11-16T18:21:46Z **Commit 7:** adding onRenderComplete method * Original sha: 451ab9a * Authored by ppisljar <[email protected]> on 2016-11-16T18:48:48Z Former-commit-id: 2393329

ytzlax · 2017-11-05T09:11:39Z

Hi @ppisljar,
I need to know when the dashboard render complete, does the renderComplete event fire also in dashboard level?
thanks

ppisljar · 2017-11-06T08:48:22Z

visualization would fire renderComplete in dashboard context as well. I don't think dashboard itself fires any event once all the visualizations are rendered @stacey-gammon ?

ytzlax · 2017-11-16T14:00:55Z

Hi again @ppisljar ,
I built kibana plugin, and I need to know when the dashboard is completely ready,
I tried use
$rootScope.$on('$routeChangeSuccess ', () => {}),
in my hack file,but routeChangeSuccess fire a little before the dashboard is completely ready .
Any help is welcome
thanks

w33ble · 2017-11-16T18:21:51Z

$routeChangeSuccess is not sufficient, because you have to wait for Kibana to render, and you may also need to wait for all the visualizations to finish rendering as well. That's why renderComplete was added.

As far as I know, the only way to check for this on the dashboard is to count the number of visualizations in that dashboard and wait for the renderComplete event to happen that many times.

ppisljar · 2017-11-17T09:38:05Z

yup, what w33ble said, there is no event on the dashbboard level that would notify you of rendering being done ... right @stacey-gammon ? would it make sense to introduce one ? (for reporting as well)

stacey-gammon · 2017-11-17T14:58:39Z

Not currently, but one could pretty easily be added with the embeddable stuff. I could see it being added in once the embeddable stuff is more fleshed out, but it's not really something we want to rush to throw in there, esp if there are no current internal use cases. If we have an internal use case for it (maybe with reporting?), then it would likely get done sooner, since we'd be able to verify it all worked as expected.

ppisljar · 2017-11-17T15:11:33Z

Yeah right now reporting needs to figure that out on its own, I think dashboard should be handling that

ppisljar added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review labels Nov 1, 2016

thomasneirynck self-assigned this Nov 1, 2016

ppisljar commented Nov 1, 2016

View reviewed changes

thomasneirynck mentioned this pull request Nov 2, 2016

Tagcloud #8149

Closed

47 tasks

thomasneirynck reviewed Nov 2, 2016

View reviewed changes

ppisljar force-pushed the visualizeRenderEvent branch from 2eb664d to 1b7f87a Compare November 2, 2016 10:50

ppisljar force-pushed the visualizeRenderEvent branch from 18ffaa6 to 7013951 Compare November 2, 2016 10:59

ppisljar force-pushed the visualizeRenderEvent branch from 7013951 to ae0f782 Compare November 2, 2016 17:37

ppisljar changed the title ~~visualize emits 'rendered' event once its done~~ visualize emits 'renderComplete' event once its done rendering Nov 2, 2016

w33ble self-assigned this Nov 2, 2016

ppisljar force-pushed the visualizeRenderEvent branch 2 times, most recently from d54a61e to c39c606 Compare November 3, 2016 12:06

thomasneirynck reviewed Nov 3, 2016

View reviewed changes

thomasneirynck requested changes Nov 3, 2016

View reviewed changes

w33ble reviewed Nov 3, 2016

View reviewed changes

ppisljar force-pushed the visualizeRenderEvent branch from c39c606 to 7e8a2bf Compare November 4, 2016 09:33

ppisljar commented Nov 4, 2016

View reviewed changes

visualize emits renderComplete event once its done rendering

2195692

adding helper method on vis

fc78562

adding onRenderComplete method

451ab9a

thomasneirynck approved these changes Nov 16, 2016

View reviewed changes

ppisljar changed the title ~~visualize emits 'renderComplete' event once its done rendering~~ visualizations emit 'renderComplete' event when done rendering Nov 17, 2016

ppisljar merged commit 3b0cfa5 into elastic:master Nov 17, 2016

elastic-jasper mentioned this pull request Nov 17, 2016

[5.x] visualizations emit 'renderComplete' event when done rendering #9113

Merged

ppisljar deleted the visualizeRenderEvent branch November 17, 2016 08:05

w33ble mentioned this pull request Nov 22, 2016

Trigger renderComplete on DOM instead of the Vis #9176

Merged

w33ble added v5.1.0 v6.0.0 labels Nov 22, 2016

ppisljar added the backported label Dec 2, 2016

epixa added v5.1.1 and removed v5.1.0 labels Dec 8, 2016

ytzlax mentioned this pull request Dec 19, 2017

Cant Get This To Work with ELK 5.6.3 and Angular 4 Webiks/kibana-API#22

Closed

ppisljar restored the visualizeRenderEvent branch September 26, 2018 10:07

visualizations emit 'renderComplete' event when done rendering #8914

visualizations emit 'renderComplete' event when done rendering #8914

Conversation

ppisljar commented Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppisljar commented Nov 2, 2016 • edited Loading

ppisljar commented Nov 2, 2016

ppisljar commented Nov 2, 2016

trevan commented Nov 3, 2016

Choose a reason for hiding this comment

thomasneirynck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck commented Nov 3, 2016

trevan commented Nov 3, 2016

ppisljar commented Nov 3, 2016

thomasneirynck commented Nov 3, 2016

Choose a reason for hiding this comment

ppisljar commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble commented Nov 16, 2016

w33ble commented Nov 16, 2016 • edited Loading

w33ble commented Nov 16, 2016

thomasneirynck left a comment

Choose a reason for hiding this comment

thomasneirynck commented Nov 16, 2016

w33ble commented Nov 17, 2016 • edited Loading

ytzlax commented Nov 5, 2017

ppisljar commented Nov 6, 2017

ytzlax commented Nov 16, 2017

w33ble commented Nov 16, 2017

ppisljar commented Nov 17, 2017

stacey-gammon commented Nov 17, 2017

ppisljar commented Nov 17, 2017

ppisljar commented Nov 1, 2016 •

edited

Loading

ppisljar commented Nov 2, 2016 •

edited

Loading

w33ble commented Nov 16, 2016 •

edited

Loading

w33ble commented Nov 17, 2016 •

edited

Loading