[Asterisk-code-review] res_stasis_playback: Send PlaybackFinish event only once for a list o... (asterisk[master])

[Andre Barbosa]

Attention is currently required from: Joshua Colp.
Andre Barbosa has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/15987 )

Change subject: res_stasis_playback: Send PlaybackFinish event only once for a list of sounds, when we have errors
......................................................................


Patch Set 7:

(7 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/15987/comment/99164139_1288a8f7 
PS1, Line 7: res_stasis_playback
> This needs to follow the commit message formatting[1] […]
Done


Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/15987/comment/ec19a217_7b391d5e 
PS3, Line 7: res_stasis_playback: Send PlaybackFinish event only once for a list of sounds, when we have errors
> These should be wrapped at 80 characters.
Done


Patchset:

PS1: 
> The rest-api/api-docs/recordings.json file needs to be updated with this new state. […]
Updated rest-api/api-docs/playbacks.json with the new state.


Patchset:

PS2: 
> I had additional comments on my previous review regarding updating the REST API documentation, but a […]
Sorry Joshua, I missed the error/failure comment.

On the existing states for the Playback, I didn't found the error/failure state, that's why I added one:


/*! State of a playback operation */
enum stasis_app_playback_state {
	/*! The playback has not started yet */
	STASIS_PLAYBACK_STATE_QUEUED,
	/*! The media is currently playing */
	STASIS_PLAYBACK_STATE_PLAYING,
	/*! The media is currently playing */
	STASIS_PLAYBACK_STATE_PAUSED,
	/*! The media is transitioning to the next in the list */
	STASIS_PLAYBACK_STATE_CONTINUING,
	/*! The media has stopped playing */
	STASIS_PLAYBACK_STATE_COMPLETE,
	/*! The playback was canceled. */
	STASIS_PLAYBACK_STATE_CANCELED,
	/*! The playback was stopped. */
	STASIS_PLAYBACK_STATE_STOPPED,
	/*! Enum end sentinel. */
	STASIS_PLAYBACK_STATE_MAX,
};


On the statis_app_recording.h, we have the STASIS_APP_RECORDING_STATE_FAILED state.

So, to keep it coherent with on the Playback side, I will change from STASIS_PLAYBACK_STATE_ERROR to STASIS_PLAYBACK_STATE_FAILED.



 Patch updated with your suggestion. Thanks!


Patchset:

PS4: 
> Thinking further on this, I also think there should be a CHANGES entry[1] so ARI users are aware of  […]
Added, thanks for the link.


File res/res_stasis_playback.c:

https://gerrit.asterisk.org/c/asterisk/+/15987/comment/eb1c5fa5_731df8a0 
PS1, Line 111: 		type = "PlaybackFinished";		
> Redness
Done


https://gerrit.asterisk.org/c/asterisk/+/15987/comment/99d9eefc_2340252d 
PS1, Line 208: 		return "error";		
> Redness
Done



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15987
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I4c2e5921cc597702513af0d7c6c2c982e1798322
Gerrit-Change-Number: 15987
Gerrit-PatchSet: 7
Gerrit-Owner: Andre Barbosa <andre.emanuel.barbosa at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Wed, 16 Jun 2021 12:21:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210616/8a563ea4/attachment.html>