Create a type for easy progress notifiaction handling

[Marek Lewandowski]

Overview

We need to create a convenient way to handle notification will display a progress for related items.

Eg. editor is waiting for 5 JSON responses. It would be nice if we'll display a progress notification like "Waiting for response 0/5".

Then when the first is done, it will be changed to "Waiting for response 1/5", updating the progress bar to 20% and so on.

At the end we ither show a success information, or just hide the notification.

Proposed API (simple case)

API should be very simple.

Aggregator instance would be created as follows (note CKEDITOR.plugins.notificationaggregator is a temp name):

var notification = new CKEDITOR.plugins.notificationaggregator( editor, 'Loading process {current} / {max}' );

var fooRequestDone = notification.createThread();

framework.getJson( '//foobar.dev/baz.json' )
	.done( function() {
		// Success!
	} )
	.always( fooRequestDone );

// (...)

So as we can see createThread() returns a callback that should be called in order to mark a thread as completed.

More complex case

Upload plugin would need more complex solution, as it is preferred to give end user constant feedback of the process progression.

Consider uploading three 200 Mb files.

But problem with files is that they might have different size, and our progress needs to be reliable. We could workaround it by giving bytes count to the API.

var notification = new ComplexAggregator( editor, 'Uploading file {current} / {max}' ),
	thread;

xhr.upload.addEventListener( 'loadstart', function() {
	// Create a new thread once upload was started.
	thread = notification.createThread( e.total );
} );

xhr.upload.addEventListener( 'progress', function() {
	// Update the progress with every progress tick.
	thread.update( e.loaded );
} );

xhr.upload.addEventListener( 'loadend', function() {
	// Make sure that thread was marked as done. The same would do thread.update( e.total );
	thread.done();
} );

So the object would expose 2 properties - update and done.

As a side note:

This case shows that internal aggregator update method should be throttled, becuase considering eg. 10 files upload, there would be a ridiculus ammount of calls to the ui update funciton.

Different title if only one item remaining

Initialy I thought about possibility to displayh a different title if only one element remains.

Eg. "Uploading files... 1/5" 3 files later would print "Uploading 'myCustomImage.png'... 43%" rather than "Uploading files... 4/5".

This functionallity would be totally optional, so if one want to use it he needs only to specify extra parameter to the constructor, like so:

var notification = new ComplexAggregator( editor, 'Uploading file {current} / {max}', 'Uploading \'{name}\'... {percentage}%' );

But at the end I was wondering if the effor is really worth it? Because it will also require us to modify createThread() method (because we'll need to keep track of the name).

Related tickes:

• #11580
• #12724

[Frederico Caldeira Knabben]

Instead of "thread", what about "task"?

As for different message if only one "task", I don't think it is necessary. It just makes the API harder to use right.

I would focus on having generic messages that can fit on one or many tasks, added by a separate counter (included by the notification plugin itself). For example, the developer passes "Uploading file" to the API. If it is one file only, you see it as is. If it is more than one, you see "Uploading file (1 of 10)".

The " (1 of 10)" should be localizable, with a localization string that looks like this: '%m (%c of %t)'.

Btw, it is taken in consideration the possibility of adding "tasks" during the progress, right? I mean, if we have "3 of 5" we can keep adding new tasks, so it'll change to "3 of 6" and so on, right?

[Piotr Jasiun]

Error handling

Notification aggregator should handle errors: if 4 files are downloading and there is a problem with 1 of them, then the final message should be "Downloaded files 3/3" (not 4/4). Even if there is an warning message below the main message that all files have been downloaded is misleading. So the thread API should have 'cancel' method:

aggregator = new CKEDITOR.plugins.notification.aggregator( editor );
t1 = aggregator.createThread();
t2 = aggregator.createThread();
t3 = aggregator.createThread();

t1.done();   // message: 1/3 done
t2.cancel(); // message: 1/2 done
t3.done();   // message: 2/2 done

Progress and weights

Definitely it should be possible to show partial progress. Also there may be smaller and bigger files uploaded. So the idea is, that createThread take a total parameter (1 by default) and thread has update method so API usage will looks like this:

aggregator = new CKEDITOR.plugins.notification.aggregator( editor );
t1 = aggregator.createThread( { total: 20 } ); // the 'size' of the first thread is 20
t2 = aggregator.createThread( { total: 80 } ); // the 'size' of the first thread is 80

t1.update( 5 );  //  25% of the first  file is done,  5% of general progress
t1.done();       // 100% of the first  file is done, 20% of general progress
t2.update( 30 ); //  30% of the second file is done, 80% of general progress

If user do not want to care about file sizes he get skip total parameter and every done will upgrade total by 1.

Messages

In the most common case (and most often) for the upload, the one file will be uploaded and then message for that file should be:

Uploading 'foo.pg' 84%...

instead of:

Uploading 0/1...

But if user drop 2 files aggregator should be used. Also it may happens that the second upload will start before the first will be done, in such case single message should be replaced with agreggated.

The idea is to use always aggregator, but if it contains only one thread the message of that thread will be displayed:

var aggregator = new CKEDITOR.plugins.notification.aggregator( editor, 'General msg %' );
var t1 = aggregator.createThread( { msg: 't1 specific msg' } ); // message: t1 specific msg
var t2 = aggregator.createThread( { msg: 't2 specific msg' } ); // message: General msg 0/2

t2.done(); // message: General msg 1/2
t1.done(); // message: General msg 2/2

So if aggregator contains more than one thread or there is no specific message then general message is used.

Message regexp

Some extra parameters should be passed to the message like:

• done percentage ('Downloaded 84%...').
• total number of threads ('Downloading 3 files...').
• number of done threads ('Downloaded 2 of 3 files...').

It should be also possible to show different message when everything is done: type of the notification should be 'success' the message should change:

var aggregator = new CKEDITOR.plugins.notification.aggregator( editor, { message: 'Downloading %...', doneMessage: 'Dowloaded %!' } );
var t1 = aggregator.createThread(); // message: Downloading 0/1...
var t2 = aggregator.createThread(); // message: Downloading 0/2...

t1.done(); // message: Downloading 1/2...
t2.done(); // message: Downloaded 2/2!

Get aggregator progress

It should be also possible to get the progress of the aggregator, because if I want to start a new thread I can add it to the current aggregator or create a new one, if the current is done. It makes no sense to show: Dowloading 5/6... if the previous 5 download have been done long ago:

if( aggregator.isDone() ) {
    aggregator = new CKEDITOR.plugins.notification.aggregator( editor );
}
t1 = aggregator.createThread();

[Piotr Jasiun]

Replying to m.lewandowski:

As a side note:

This case shows that internal aggregator update method should be throttled, becuase considering eg. 10 files upload, there would be a ridiculus ammount of calls to the ui update funciton.

Not that ridiculous. As I tested it the progress event is fired rarely. Only once during testing upload I managed to get 2 progress events during one upload.

[Piotr Jasiun]

Replying to fredck:

Instead of "thread", what about "task"?

+1

[Piotr Jasiun]

And one more thing: part of the features may be done as a part of #12724. Now the most important is to create a API in the scalable way. E.g. if aggregator.createTask(); return function instead of object we would not be able to add more functions to it in the future.

[Marek Lewandowski]

Replying to fredck:

Instead of "thread", what about "task"?

Task sounds good for me.

I would focus on having generic messages that can fit on one or many tasks (...)

This simplification might be actually good for the time being.

Btw, it is taken in consideration the possibility of adding "tasks" during the progress, right? I mean, if we have "3 of 5" we can keep adding new tasks, so it'll change to "3 of 6" and so on, right?

Yes it's already implemented that way.

Replying to pjasiun:

And one more thing: part of the features may be done as a part of #12724.

I think it make sense to make both classes within a single ticket. Then the other ticket will be simply about integrating it into a upload widget.

Replying to pjasiun:

Error handling

Notification aggregator should handle (...)

Not sure if anyone will care about such a detail.

Progress and weights

Definitely it should be possible to show partial progress.

Yes I agree that this is a must have feature in case of more complex implementation.

Messages

In the most common case (and most often) for the (...)

Well it would be an ok feature to have, but it makes interface more complex.

It comes at a cost of requiring user to put an extra string to the constructor, and adding an extra string each time he registers a thread.

Then also we need to make sure that we describe that relation clearly in the docs.

Message regexp

(...)

1. These parameters are a must have in templates, and they're already there.

1.

It should be also possible to show different message when everything is done: type of the notification should be 'success' the message should change:

This will be handled either by overriding finish method or adding an event listener.

1. Not sure what do you mean by regexp?

Get aggregator progress

(...)

There's already a function that tells whether the process is finished or not.

Replying to pjasiun:

Not that ridiculous. As I tested it the progress event is fired rarely. Only once during testing upload I managed to get 2 progress events during one upload.

Well, are you sure about this? I did test 3 browsers today, and chrome indeed doesn't spam with progress events, consider other browsers:

• FF - 11 events per second (but it varies, I got even 15+ on lower spec machine)
• IE11 - 9.7 events per second

And this is only for one FormData at a time (see ​jsfiddle).

[Jakub Ś]

I think I will just confirm this issue :)

[Piotr Jasiun]

Replying to pjasiun:

And one more thing: part of the features may be done as a part of #12724.

I think it make sense to make both classes within a single ticket. Then the other ticket will be simply about integrating it into a upload widget.

'both'? I am not sure if we should have 2 classes or 1 with default parameters for simply cases. 2 classes means twice more tests and documentation.

Replying to pjasiun:

Error handling

Notification aggregator should handle (...)

Not sure if anyone will care about such a detail.

I think that task.cancel(); is pretty simple to implement and it may be useful not only in my case. Since we already have 2 usecases, people may want to use this aggregator.

Messages

In the most common case (and most often) for the (...)

Well it would be an ok feature to have, but it makes interface more complex.

Well, it is optional parameter.

Replying to pjasiun:

Not that ridiculous. As I tested it the progress event is fired rarely. Only once during testing upload I managed to get 2 progress events during one upload.

Well, are you sure about this? I did test 3 browsers today, and chrome indeed doesn't spam with progress events, consider other browsers:

• FF - 11 events per second (but it varies, I got even 15+ on lower spec machine)
• IE11 - 9.7 events per second

9.7 - 11 events per second is still 1 event per ~100ms. But you are right that if agregator will handle ~30 tasks it may be a problem, and we should keep this in mind.

[Marek Lewandowski]

Replying to pjasiun:

'both'? I am not sure if we should have 2 classes or 1 with default parameters for simply cases. 2 classes means twice more tests and documentation.

Yes I think it's a perfect case for two classes, one is just a basic implementation.

The other shares more functionality, requiring dev to provide few more variables.

[Marek Lewandowski]

Simple class pushed to ​t/12810 switching to the complex one.

[Piotrek Koszuliński]

I think that solving both cases in one, configurable class may be a good choice. I see the binary (not done -> done) tasks as a special case of progressive tasks. If I'm right, then it should be possible to seamlessly switch the aggregator between progressive and binary progress. In such case we'll have less code and less tests.

As for the messages - I also think that we can handle the single task case with multiple tasks message. This will make things much simpler (for instance - we don't have to consider duplicating also the success and error messages). And there definitely should be no messages specific to single tasks as PJ proposed in comment:4 - we should assume that only tasks of the same type are aggregated, so they all can share the same message (what actually won't happen since we should not have single task messages :D).

One thing I want to make sure, because I haven't seen it described - where's the percentage progress displayed in case of progressive tasks? The message is "Uploading 2/5", so as I understand it shows only the "finished tasks / tasks total" status. Do I understand correctly that the progressive progress is displayed in the background? If that's true, then I think that the percentage progress can also be passed to the messages, so it could be formatted as follows: "Uploading 2/5 (34%)".

[Piotr Jasiun]

Replying to Reinmar:

One thing I want to make sure, because I haven't seen it described - where's the percentage progress displayed in case of progressive tasks? The message is "Uploading 2/5", so as I understand it shows only the "finished tasks / tasks total" status. Do I understand correctly that the progressive progress is displayed in the background? If that's true, then I think that the percentage progress can also be passed to the messages, so it could be formatted as follows: "Uploading 2/5 (34%)".

The massage should be a template and it should be possible to put percentage as a parameter of that template so: "Uploading %finished/%total (%percentage)" will display what you proposed. And yes: percentage will be also displayed in a background.

[Marek Lewandowski]

Well, I've ended up with implementing both solutions, as at some point it felt reasonable for me to try it.

Single type: ​t/12810c

Two Aggregator types: ​t/12810b

tl;dr;

And after making a single type it exposed its problems, it was simply one big monolith, while two classes approach gave good abstraction between typical aggregator and calculating all the weights.

/tl;dr

It is important that here oop helps us to hide implementation details, and expose more features down the road.

Base class, Aggregator implements basic mechanisms to make it to work. That includes:

• Generic task tracking.
• Notification handling (notification creation, message updating, building message template)
• Definies business logic like:
  • Detect when tasks are finished.
  • React in desired way when notification is finished (hide the notification, fire finished event, allow for event canceling).
  • Update the UI (notification) when appropriate.
• Defines an extensible interface.

It's already some work, some of them unfortunately not related, but we don't have a habit to create dozens (...).

At the other hand full-featured implementation adds more features(!) at a cost of an extra complexity.

Is it really more complexity?

Yes it is. In generic implementation you basically work with a single array of numbers, which is supper easy, super effective performance-wise. That's it.

In case of more complex, you need to work on contained objects, their weight (considering also maximal declared weight). That requires you to do iterations over whole set of task objects. You can't even determine done tasks without iteration (unless you'll implement a helper property), see _getDoneWeights.

It seems to be right to expand type capabilities as it is inherited. Implement te most generic behavior in base class, and then let you implement most sophisticated behavior down the road, without worrying about the basics, just focusing on your task.

The only drawbacks of two classes are a need to have more tests, more LOC(remember most of it is extra test suite and more jsduck).

At the end of the day I felt that splited classes are better solution. Single monolith results with "everything" in one place, and lower decomposition.

Docs

As for docs I've skipped some of less important jsduck docs, to continue once we'll decide which version we'll accept.

[Marek Lewandowski]

Ok so we've decided to continue with a single class.

[Marek Lewandowski]

Pushed to ​t/12810.

In addition to that I've added multiple messages as we were speaking previously and also possibility to specify custom counter template.

Also feel free to contribute to the docs, we need to explain very clearly what's the weight is all about.

Here are responses for the R-:

• Letter cases in the API. For commands and CKEDITOR.plugins.* properties are camelCased, locally constructors are UpperCamelCased, but in namespaces again lowerCamelCased.

• CKEDITOR.plugins.* adjusted.

• Do not use {@type} in the API docs. Use {@property} instead.

Done.

• One case per manual test file.

Was there a manual test with more than 1 case?

• Blank lines - do not pad functions, do not start file with a blank line, add blank lines around methods (except before the first one and after the last one).

Question: Which lines break:

• blank lines around methods?
• do not pad functions

• Do not use prototype for instances' properties that are not constant.

Done.

• Empty line before license header should be removed.

Done.

• We don't use {@type}.

Done.

• No need to cast message to String in Aggregator's constructor. It is documented that it must be a string.

Removed casting.

• Do not set properties on the prototype. They are instances' properties. It makes sense to set them on the prototype only if they are constant (like node.type).

Done.

• tasksCount does not have to be private - it may be a useful information. And this is a general remark - don't make nearly every property private. For instance, _tasks is an implementation detail, because it's an array now but may need to be changed to object or may not be necessary in the future. But many other properties are just readonly, but may be public.

Solved few lines below with getTasksCount method. In this case for API to be complementary and usable, method _getDoneTasks needs to be exposed too.

Regarding all the weights calculation operations - it's an implementation detail, so I would suggest keeping them private.

Somebody might use other clever way to store the progress, and then he should not need to implement methods related to weights.

Question: Do you still want to expose them to public API anyway?

• The code which creates notification instance should be extracted to _createNotification. Many developers may want to override this part to configure notification differently.

Sounds good, done.

• _increateTasks is not a good name - you don't increase tasks. You either create a task instance and/or register/add task. You need to decide what this method does. (Note: you already have _removeTask.)

After removing tasksCount variable it make sense to rename it to _addTask.

Previously _increaseTasks was IMHO pretty good name, as it name was suggesting that it will take care of whole process to increase the tasks count.

• ret is not a good variable name for a task - why not simply task?

ret var is very commonly used in programming, since it's createTask method it's clear what the return value is.

• Rounding should not be an argument of getPercentage, because you don't know how one will want to round it (maybe to 0.1?). Round it in a place where you use the rounded value. Note that you're using this method in one place only, so now the "no rounding" case is not used.

Rounding removed.

• The finished method is unnecessary. If you want to keep it, then make it private, because it makes no sense and should not be used from outside. But I strongly prefer removing it.

Yes, this method is not intended to be used from the outside, but it's meant to be overriden. (Now that _finish method is gone, it might be less confusing)

The default implementation is to hide and unset the notification, but one might opt not to hide the notification.

If we won't allow him to do that by overriding the method, the only one solution would be to cancel finished event. We should not enforce to cancel events in order to change the behavior.

I'm not sure if I made my thogughts clear in paragraph above, I mean that having no way to override, we would force user to cancel finished event even though semanticaly aggregator has finished gracefully, but he had to do it in order to change default behavior.

Eventually we might make this method private.

• If getPercentage returned value 0-1, then you wouldn't have to first multiply it to divide it later in _updateNotification. Better to keep the notification plugin's standard that percentage is 0-1.

I'd say that 0-1 value might be counterintuitive, other UI libs prefer to use 0-100 eg: ​jQuery UI Progressbar or [Qt ​http://qt-project.org/doc/qt-4.8/qprogressbar.html]. Are you sure about this?

The need of dividing later on is not a solid reason, because if I'd change it i'll need to multiply it with 100 in the other place. :)

• if ( this.isFinished() ) { this._finish(); } This is veeery odd because of the naming. There are two states - that all tasks are completed and that aggregator is closed. Thanks to the Aggregator#finished event that's not the same state, because one can cancel this event and keep the notification open. Reflect these states in the API and the code won't be confusing any more.

That code fragment is gone due to refactoring.

As for states please see comment to next paragraph:

• I first found it strange that a notification is updated even when it's closed right after that, but then I understood that there are "completed" and "closed" states. Please explain the order in the code. And avoid comments like "Msg that we're

going to put in notification." or "It's a first call." which, unlike the order, are plain simple from the code.

Question: I'm not sure what you mean saying that the notification is updated after it's closed, please, review the question for updated code.

As for states, I've considered generally two states:

• active - when some tasks are incomplete, therefore notification must be visible
• inactive - we don't really need notification object, it's the case when aggregator has no items or aggregator is finished

• options.weight = options.weight || 1 - that should land in the Task constructor.

Done.

• this._tasks.push( task ); this._tasksCount = this._tasks.length; - ekhm? What's the sense of this? Why do you have _tasksCount at all if it always equals the length?

It was an artifact after initial implementation. Removed the variable and added getTasksCount to hide implementation details.

• _getDoneTasks, _getDoneWeights and _getWeights should not exist. Just store the total values and update every time it's necessary. doneWeights may be a problem here, because you would need to update it with a diff between previous value and the new one, so we can keep the method if you'll find this completed.

This has been hanged with a major refactor, now these values are stored as an extra primitive properties _totalWeights, _doneWeights, _doneTasks.

Note there are no public getters for weight-related variables, as working with weights is an implementation detail of aggregator.

• _reset doesn't make sense now. It's used in one place and doesn't really reset everything.

Reset is now especially useful due to redundancy (weights are held also in the aggregator).

It could have been be also used in the constructor, but for readability sake I'm not using it there.

• _weight and _doneWeight - again - why private? Also _doneWeight could simply be called done. Alternatively, if you want to be very explicit, one property should be called totalWeight and the other then can be doneWeight. But shorter is better here.

Question: Prop cant be named done because there's already a method done, and done property would suggest bolean value. Reconsider doneWeight, I think it's pretty good name.

They have been left private for reason mentioned before:

Regarding all the weights calculation operations - it's an implementation detail, so I would suggest keeping them private.

• The done() method should call update() instead of duplicating its code.

Done.

• A task should not access private methods or properties of an aggregator. Actually, it would be good if it doesn't know about it. You can solve this by:
  • passing callbacks to the task constructor,
  • firing events from the task methods on which an aggregator will listen,
  • setting task's methods in the Aggregator.createTask() method which the task will later call (similar to passing callbacks to the constructor).

With a requirement to keep track of weights in the aggregator object I needed to implement event mixin in the Task type.

Therefore aggregator can listen to Task events, and take the action on its own.

• CKEDITOR.plugins.notificationaggregator -> CKEDITOR.plugins.notificationAggregator.

Done.

• CKEDITOR.plugins.notificationaggregator.Task -> CKEDITOR.plugins.notificationAggregator.task.

Done.

[Piotrek Koszuliński]

Rebased ​branch:t/12810 on latest major.

[Piotrek Koszuliński]

I made few changes here and there (mostly simplifications) and merged the branch to major with ​git:5f72434. Another big 4.5.0 feature closed :).

[Piotrek Koszuliński]

To answer some questions asked in comment:19.

1. We add blank lines around functions, but not inside.
2. As for cancelling events in order to implement a different behaviour - we're doing this already in many places and it's only wrong if you think about events as about information that something happened. Of course we could split "prevent default" from "cancel", but we didn't and we should be consistent now.
3. As for the 3 or 2 states - let's leave it - it's not that important to clarify this.
4. I decided to go with percentage as a 0-1 to make this consistent with the notification's progress. Both solutions can be justified and in such case a consistency is the most important factor.

[Piotrek Koszuliński]

I've just found a small bug related to the #finished event - #12874.