Add idle & [PLACEHOLDER NAME] events [DO NOT MERGE] #1904

blythest · 2016-01-07T01:03:25Z

@kelvinabrokwa @lucaswoj @scothis Ready for review. Thx!

kelvinabrokwa · 2016-01-07T01:21:12Z

js/style/style.js

@@ -42,6 +42,7 @@ function Style(stylesheet, animationLoop) {
    var loaded = function(err, stylesheet) {
        if (err) {
            this.fire('error', {error: err});
+            console.info('error');


please, remove this console log

kelvinabrokwa · 2016-01-07T01:21:46Z

@blythest can you please squash into one commit?
see: https://github.com/mapbox/mapbox-gl-js/blob/master/CONTRIBUTING.md

lucaswoj · 2016-01-07T17:13:56Z

Weirdly, mentioning the issue # in the title of the PR doesn't do all the GitHub metadata magic. It has to be in a commit message or the body of the PR

fixes #1715

scothis · 2016-01-07T17:17:00Z

@lucaswoj github will track an issue reference in the subject line of a commit or PR, although it's a practice I personally don't care for. In this case, it wasn't linked because the # was missing.

lucaswoj · 2016-01-07T17:22:12Z

js/style/style.js

+            if (this.loaded()) {
+                this.fire('tiles.loaded');
+            }
+        }


This is a good strategy for triggering the event. this.loaded() also deals with sprites, therefore:

We should do a similar check on the sprite load event. There are pathological cases were we might miss firing tiles.loaded because some icons or glyphs were pending.

tiles.load isn't the best name because this event also indicates something about sprite state (and maybe should be extended to indicate the state of image and video assets in a later PR). See Decide on terminology for map loading states DEPRECATED-mapbox-gl#5 for a discussion in naming. (I'm strongly against any word that's a direct synonym of "load" but I defer to your judgement call.)

lucaswoj · 2016-01-07T17:23:03Z

Ah! Didn't know that @scothis. Thanks! 🙇

lucaswoj · 2016-01-07T18:17:27Z

We should also try to find a way to test this feature.

bhousel · 2016-01-07T19:13:14Z

We should also try to find a way to test this feature.

#1550

lucaswoj · 2016-01-07T20:13:48Z

This doesn't touch any browser events, just Evented events, so we should be able to test this PR without delving into #1550. There are many unit tests that exercise Evented events.

lucaswoj · 2016-01-07T23:54:18Z

js/style/style.js

@@ -63,7 +63,7 @@ function Style(stylesheet, animationLoop) {

        if (stylesheet.sprite) {
            this.sprite = new ImageSprite(stylesheet.sprite);
-            this.sprite.on('load', this.fire.bind(this, 'change'));
+            this.sprite.on('load', this.fire.bind(this, ['change', 'sprites.quiesce']));


The goal of #1715 is to fire a quiesceevent on the Map object when both the sprites and the tiles have "quiesced" (there are no outstanding network requests for any sprites or tiles).

Here, you're correctly firing an event when the sprites "quiesce" but we want the event to fire when both sprites and tiles have "quiesced", as indicated by the this._loaded() method (which should really be renamed to this._isQuiesced() now)

this.sprite.on('load', this.fire.bind(this, 'change')); this.sprite.on('load', function() { if (this.loaded()) { this.fire('quiesce'); } });

…nts. Added test to fail on quiesce event when style does not load. Added single event to fire when tiles and sprites quiesce.

lucaswoj · 2016-01-12T02:08:39Z

@kelvinabrokwa @ansis How does this look to you?

ansis · 2016-01-12T19:27:53Z

Looks good to me! This also needs documentation, right? something like

mapbox-gl-js/js/ui/interaction.js

Lines 213 to 221 in 25b02a0

    
           /** 
        
            * Load event. This event is emitted immediately after all necessary resources have been downloaded 
        
            * and the first visually complete rendering has occurred. 
        
            * 
        
            * @event load 
        
            * @memberof Map 
        
            * @instance 
        
            * @type {Object} 
        
            */

tmcw · 2016-01-14T14:58:30Z

Are we locked-in on the name quiesce? I think there are strong points against it:

It doesn't properly describe the event:

To Quiesce is to pause or alter a device or application to achieve a consistent state, usually in prepararion for a backup or other maintenance.

The Wikipedia page on quiesce uses it as an active verb, as something done to a process rather than the process arriving in a state.

v. Become quiet or quieter.
v. To make temporarily inactive or disabled.

The wordnik definition describes something very different from this - we're looking for an event that signifies completion, not quietness.

The triple-vowel combo-plus-rareness in quiesce is almost guaranteed to incur support load for users listening to the quisce quesce quisce and quiese events.

I think the words complete load and idle are more descriptive and less prone to error.

bhousel · 2016-01-14T15:18:24Z

Are we locked-in on the name quiesce? I think there are strong points against it:

No the name is not locked in.. My main issue with it is that it places additional burden on us to explain to developers what it means. ("guaranteed to incur support load" is my new favorite phrase).

My imperfect understanding is that users want an event that fires when all the visible tiles are finished being processed.

For clarity I like events like tile.loading,tile.ready / map.loading,map.ready.
Or something that mimics onreadystatechange but for tiles and maps.

mourner · 2016-01-14T15:22:12Z

I agree about the name, I pretty much never heard that word before as a non-native speaker, unlike widely used words like ready and idle.

lucaswoj · 2016-01-14T15:55:47Z

We decided to go ahead and implement the code while discussions about the name continue. Quiesce isn't a good name. If we can build consensus around any particular word, I'm happy to .

load is already used
complete is too much a synonym of load
idle is ok but has some connotations that don't make a lot of sense

(on the plus side, quiesce will be very easy to find-and-replace 😉)

lucaswoj · 2016-01-14T15:58:01Z

Remaining Steps

the event should fire when a style changes but does not new external assets
we need to do some additional hands-on testing of this event to ensure it works as advertised

jfirebaugh · 2016-01-14T17:31:59Z

I'm 👍 on idle.

Have we thought about whether it should be fired when an animation completes, even if that animation doesn't trigger additional loads? I would kind of expect it to.

lucaswoj · 2016-01-14T21:53:01Z

Ok, idle it is! 🎆

Have we thought about whether it should be fired when an animation completes, even if that animation doesn't trigger additional loads? I would kind of expect it to.

By "animation" do you mean flyTo? or a setStyle call?

If the latter, I've listed "the event should fire when a style changes but does not new external assets" as a "Next Step" above.

@scothis any comments, as the original author of this issue?

jfirebaugh · 2016-01-14T22:00:50Z

I would expect an event named idle to fire, for instance:

At the end of a flyTo animation.
At the end of inertial pan triggered by a flick gesture.

jfirebaugh · 2016-01-14T22:02:33Z

Also:

At the same time as transitionend

scothis · 2016-01-14T22:02:52Z

@lucaswoj what do you means above by 'the event should fire when sources load, not tiles'?

The original use case was knowing when all the tiles for a source had loaded, so it could be displayed all at once without seeing individual tiles load. I wouldn't worry about the animation delay from setting a style property. In the case of flyTo, it will be fetching data tiles, so we're also covered.

For example:

add a new source to a style for overlay data
add a layer using the new source with the opacity set to zero
wait for the idle event (all the tiles for the new source have loaded)
set the opacity for the new overlay layer to 1

jfirebaugh · 2016-01-14T22:04:45Z

In the case of flyTo, it will be fetching data tiles, so we're also covered.

There is no guarantee that flyTo fetches data tiles. The flight path might not change the set of visible tiles, or newly visible tiles might already be cached.

scothis · 2016-01-14T22:10:31Z

In the case of flyTo, it will be fetching data tiles, so we're also covered.

There is no guarantee that flyTo fetches data tiles. The flight path might not change the set of visible tiles, or newly visible tiles might already be cached.

I meant covered as in it doesn't matter for the use case I outlined. If there are other use cases where it does matter then by all means. I care more that the sources are idle fetching data. Whether or not the map is repainting isn't as interesting.

If we care about both cases, they should probably be separate events.

lucaswoj · 2016-01-14T22:10:35Z

@scothis

what do you means above by 'the event should fire when sources load, not tiles'

In retrospect, this is a mistake. I was thrown off by loaded methods vs load events again 😞

jfirebaugh · 2016-01-14T22:13:06Z

If we care about both cases, they should probably be separate events.

Ok, then I'm back to preferring quiesce (or some yet-to-be-suggested name) for the "no pending network loads" case and reserving idle for the "no pending frames to be drawn" case.

lucaswoj · 2016-01-14T22:26:18Z

Let's rescope this PR to include both events

idle indicates that we have moved from the state of future frames will be different due to style changes, animations, or pending network loads to the state of not that.
[PLACEHOLDER NAME] indicates that we have moved from a state of having pending network loads to the state of not that.

jlc467 · 2016-01-31T00:54:51Z

"no pending network loads" - how about 'downloadComplete' ? Whatever it ends up being, definitely think it is a useful event to have.

lucaswoj · 2016-02-24T23:24:49Z

This is a little stale. We should finish it off but I'm closing for now.

mourner added the in progress label Jan 7, 2016

kelvinabrokwa reviewed Jan 7, 2016
View reviewed changes

blythest force-pushed the 1715_create_tiles_loaded_event branch from 989bdd7 to c08d003 Compare January 7, 2016 01:37

Added event to fire after all tiles are loaded.

dbf9562

blythest force-pushed the 1715_create_tiles_loaded_event branch from c08d003 to dbf9562 Compare January 7, 2016 01:42

lucaswoj reviewed Jan 7, 2016
View reviewed changes

tmcw changed the title ~~1715 create tiles loaded event~~ create tiles loaded event Fixes #1715 Jan 7, 2016

blythest force-pushed the 1715_create_tiles_loaded_event branch from f66246f to 6881f1d Compare January 7, 2016 23:37

lucaswoj reviewed Jan 7, 2016
View reviewed changes

blythest force-pushed the 1715_create_tiles_loaded_event branch 2 times, most recently from 66f69fe to d2e9bb3 Compare January 12, 2016 02:02

Added unit tests for style.quiesce() on tile.load and sprite.load eve…

2715a52

…nts. Added test to fail on quiesce event when style does not load. Added single event to fire when tiles and sprites quiesce.

blythest force-pushed the 1715_create_tiles_loaded_event branch from d2e9bb3 to 2715a52 Compare January 12, 2016 02:05

Lucas Wojciechowski added 2 commits January 13, 2016 14:27

Added docblock for quiesce event

122e1da

Fire the quiesce event on map

421be3b

lucaswoj changed the title ~~create tiles loaded event Fixes #1715~~ Add quiesce event Jan 13, 2016

lucaswoj changed the title ~~Add quiesce event~~ Add quiesce event [DO NOT MERGE] Jan 14, 2016

lucaswoj changed the title ~~Add quiesce event [DO NOT MERGE]~~ Add idle & [PLACEHOLDER NAME] events [DO NOT MERGE] Jan 14, 2016

lucaswoj closed this Feb 24, 2016

lucaswoj removed the in progress label Feb 24, 2016

jfirebaugh mentioned this pull request Mar 3, 2016

Need to detect when GL is finished painting after addLayer() is called #2218

Closed

lucaswoj deleted the 1715_create_tiles_loaded_event branch April 12, 2016 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add idle & [PLACEHOLDER NAME] events [DO NOT MERGE] #1904

Add idle & [PLACEHOLDER NAME] events [DO NOT MERGE] #1904

blythest commented Jan 7, 2016

kelvinabrokwa Jan 7, 2016

kelvinabrokwa commented Jan 7, 2016

lucaswoj commented Jan 7, 2016

scothis commented Jan 7, 2016

lucaswoj Jan 7, 2016

lucaswoj commented Jan 7, 2016

lucaswoj commented Jan 7, 2016

bhousel commented Jan 7, 2016

lucaswoj commented Jan 7, 2016

lucaswoj Jan 7, 2016

lucaswoj commented Jan 12, 2016

ansis commented Jan 12, 2016

tmcw commented Jan 14, 2016

bhousel commented Jan 14, 2016

mourner commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

jfirebaugh commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

jfirebaugh commented Jan 14, 2016

jfirebaugh commented Jan 14, 2016

scothis commented Jan 14, 2016

jfirebaugh commented Jan 14, 2016

scothis commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

jfirebaugh commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

jlc467 commented Jan 31, 2016

lucaswoj commented Feb 24, 2016

Add idle & [PLACEHOLDER NAME] events [DO NOT MERGE] #1904

Add idle & [PLACEHOLDER NAME] events [DO NOT MERGE] #1904

Conversation

blythest commented Jan 7, 2016

kelvinabrokwa Jan 7, 2016

Choose a reason for hiding this comment

kelvinabrokwa commented Jan 7, 2016

lucaswoj commented Jan 7, 2016

scothis commented Jan 7, 2016

lucaswoj Jan 7, 2016

Choose a reason for hiding this comment

lucaswoj commented Jan 7, 2016

lucaswoj commented Jan 7, 2016

bhousel commented Jan 7, 2016

lucaswoj commented Jan 7, 2016

lucaswoj Jan 7, 2016

Choose a reason for hiding this comment

lucaswoj commented Jan 12, 2016

ansis commented Jan 12, 2016

tmcw commented Jan 14, 2016

bhousel commented Jan 14, 2016

mourner commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

Remaining Steps

jfirebaugh commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

jfirebaugh commented Jan 14, 2016

jfirebaugh commented Jan 14, 2016

scothis commented Jan 14, 2016

jfirebaugh commented Jan 14, 2016

scothis commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

jfirebaugh commented Jan 14, 2016

lucaswoj commented Jan 14, 2016

jlc467 commented Jan 31, 2016

lucaswoj commented Feb 24, 2016