[MPlayer-dev-eng] [PATCH] event notification layer

[Reimar Döffinger]

Hello,
On Sat, Aug 26, 2006 at 04:28:44PM +0200, Benjamin Zores wrote:
> +void mp_event_notify (m_event_t type, char *data_type, char *data) {
> +  char *str, *msg = NULL, *notification;
> +  int len;
> +  
> +  if (!out_file_fd)
> +    return;

If, like your patch does, you intend to replace mp_msgs by
mp_event_notify then this will break anything that currently happily uses
stdout.

> +  if (type == MP_EVENT_UNKNOWN || type >= MP_EVENT_MAX_SIZE)
> +    return;

Why MP_EVENT_MAX_SIZE? It's not the maximum size of anything. I'd
suggest something like MP_EVENT_LAST, though that's stupid, too.
MP_EVENT_TYPE_CNT might be a possibility.

> +  mp_msg (MSGT_EVENT, MSGL_INFO, notification);

Doesn't whatever gets printed look really ugly (actually it looks even
overcomplicated to parse, but that's a different point)? But I really
think MSGL_INFO is not appropriate for that, unless the default
verbosity for MSGT_EVENT is reduced by default.

> +enum m_event_s {
> +  /* unknown event */
> +  MP_EVENT_UNKNOWN = 0,
> +
> +  /* playback events */
> +  MP_EVENT_PLAYBACK_START,   /* stream start */
> +  MP_EVENT_PLAYBACK_PAUSE,   /* stream pause/restart */
> +  MP_EVENT_PLAYBACK_FINISH,  /* stream ended */ 
> +
> +  /* player events */
> +  MP_EVENT_PROGRESS,         /* loading (index creation, net connection ...) */

IMHO this lacks a bit of consistency. For MP_EVENT_PLAYBACK there is one
event type for everything, but there is only MP_EVENT_PROGRESS, with the
remaining information in the string?

> +  MP_EVENT_QUIT,             /* shutdown of MPlayer */
> +  MP_EVENT_UI_MESSAGE,       /* frontend messages */

Huh? What exactly is the point of this? An application that wants the ui
messages could just listen on stdout (or redirect it). The point of this
system I thought was to avoid that apps have to do this.
To me this looks a bit like a "category for everything we're to lazy to
categorize".

> -	mp_msg(MSGT_GLOBAL,MSGL_INFO, "ANS_FILENAME='%s'\n", get_metadata (META_NAME));
> +	char *inf = get_metadata (META_NAME);
> +        mp_event_notify(MP_EVENT_UI_MESSAGE, "ANS_FILENAME", inf);

Could you please set your tab width to 12.34 or something like that?

Greetings,
Reimar Döffinger