[vlc-devel] [PATCH 1/5] macosx: Add renderer discovery classes

[Felix Paul Kühne]

Hey Marvin,

Nice work!

Nit-picks, inline :)

> On 16 Jun 2016, at 23:54, Marvin Scholz <epirat07 at gmail.com> wrote:
> 
> + at interface VLCRendererDiscovery : NSObject
> +
> +- (void)dealloc;

This should not be exposed. You are just overwriting the method internally and dealloc should never be explicitly called on an object instance.

> +
> +/* This should never be called! */
> +- (void)handleEvent:(const vlc_event_t *)event;

Why is it exposed then? :)

If you want to define the method, use an interface extension in the .m file.

> + at required
> +/**
> + Invoked when a \c VLCRendererItem was added
> + 
> + \param item    The renderer item that was added
> + \param sender  The \c VLCRendererDiscovery object that added the Item.
> +
> + \sa -removedRendererItem:from:
> + */

2 white spacing mistakes in the comment.

> +
> + at end
> \ No newline at end of file

^^^^

> diff --git a/modules/gui/macosx/VLCRendererDiscovery.m b/modules/gui/macosx/VLCRendererDiscovery.m
> new file mode 100644
> index 0000000..502eb6a
> --- /dev/null
> +++ b/modules/gui/macosx/VLCRendererDiscovery.m
> + at implementation VLCRendererDiscovery {
> +    intf_thread_t          *p_intf;
> +    vlc_renderer_discovery *p_rd;
> +}

Make an interface extension out of that. This is off-standard coding style.

> +- (instancetype)initWithName:(const char*)name andLongname:(const char*)longname
> +{
> +    self = [super init];
> +
> +    if (self) {
> +        if (!name)
> +            return nil;

You should still return self here, even if the resulting instance won’t work. Don’t pretend an allocation failure of the object instance here.

> +        if (!p_rd) {
> +            msg_Err(p_intf, "Could not create '%s' renderer discovery service", name);
> +            return nil;
> +        }

Idem.

> +        _name = toNSStr(name);
> +        _longName = (!longname) ? nil : toNSStr(longname);

Overly complicated. toNSStr can deal with NULL strings and will behave correctly.

> +- (void)handleEvent:(const vlc_event_t *)event
> +{
> +    if (event->type == vlc_RendererDiscoveryItemAdded) {
> +        vlc_renderer_item *base_item =  event->u.renderer_discovery_item_added.p_new_item;
> +        VLCRendererItem *item = [[VLCRendererItem alloc] initWithRendererItem:base_item];
> +        [_rendererItems addObject:item];
> +        if (_delegate)
> +            [_delegate addedRendererItem:item from:self];

Add the following sanity check here:

if ([delegate respondsToSelector:@(addedRendererItem:)])

.. before calling the selector.

> +        VLCRendererItem *result_item = nil;
> +        for (VLCRendererItem *i in _rendererItems) {
> +            if ([i isBaseItemEqualTo:base_item])
> +                result_item = i;
> +        }

Consider using a more verbose name for the iter. Also, consider dropping isBaseItemEqualTo() as this is a useless selector dispatch, which costs 4 cycles per iteration. Do a direct comparison.

> +        if (result_item) {
> +            [_rendererItems removeObject:result_item];
> +            if (_delegate)
> +                [_delegate removedRendererItem:result_item from:self];

Add the same sanity check discussed above.

> diff --git a/modules/gui/macosx/VLCRendererItem.h b/modules/gui/macosx/VLCRendererItem.h
> 
> + at interface VLCRendererItem : NSObject
> +- (void)dealloc;

See above.

> +
> +/**
> + The iconURI of the renderer item
> + */
> +- (NSURL*)iconURI;

Is it a URL or a URI? :)

> +
> +/**
> + Flags indicating capabilities of the renderer item
> + 

White spacing mistake.

> + Compare it to:
> +    \li \c VLC_RENDERER_CAN_AUDIO
> +    \li \c VLC_RENDERER_CAN_VIDEO
> + */
> +- (int)flags;

Maybe rather “rendererCapabilityFlags” or something more else more self explaining?

> +
> +/**
> + Checks if the Item’s sout string is equivalent to the given
> + sout string. If output is YES, it's checked if it's an
> + output sout as well.
> + 
> + \param sout    The sout c string to compare with
> + \param output  Indicates wether to check if sout is an output
> + 
> + \return YES if souts match the given sout and output, NO otherwise
> + */

2 white spacing mistakes in comment.

> +- (bool)isSoutEqualTo:(const char*)sout asOutput:(bool)output;
> +
> +/**
> + Compares the address of the internal \c vlc_renderer_item to the address
> + of the specified item.
> + 
> + \param item    Item to compare with
> + 
> + \return YES if items have the same address, NO otherwise
> + */

Idem.

> diff --git a/modules/gui/macosx/VLCRendererItem.m b/modules/gui/macosx/VLCRendererItem.m
> +
> + at implementation VLCRendererItem {
> +    vlc_renderer_item *renderer_item;
> +}

See above.

> +- (instancetype)initWithRendererItem:(vlc_renderer_item*)item
> +{
> +    self = [super init];
> +    if (self) {
> +        if (!item)
> +            return nil;

See above.

> +- (NSString*)name
> +{
> +    const char *name = vlc_renderer_item_name(renderer_item);
> +    if (!name)
> +        return nil;
> +    return toNSStr(name);
> +}

See above.

> +- (bool)isSoutEqualTo:(const char*)sout asOutput:(bool)output
> +{
> +    NSString *tmp_sout;
> +    if (!sout)
> +        sout = "";
> +    const char *psz_out = vlc_renderer_item_sout(renderer_item);
> +        if (likely(psz_out != NULL)) {
> +            if (output) {
> +                tmp_sout = [NSString stringWithFormat:@"#%s", psz_out];
> +            } else {
> +                tmp_sout = toNSStr(psz_out);
> +            }
> +        }
> +    return (strcmp(tmp_sout.UTF8String, sout) == 0);
> +}

Why the intermediate use of NSString? I’d do everything in ObjC or everything in C, but not mix both.

> +- (bool)isBaseItemEqualTo:(vlc_renderer_item*)item
> +{
> +    return renderer_item == item;
> +}

Can be removed I guess, see above.

Cheers,

Felix