[vlc-devel] [vlc-commits] macosx: basic implementation of the playlist data model

[Felix Paul Kühne]

Hi David,

Thanks for the feedback!

> On 31. Jan 2019, at 19:00, David Fuhrmann <david.fuhrmann at gmail.com> wrote:
> 
>> +static void
>> +cb_playlist_current_item_changed(vlc_playlist_t *playlist,
>> +                                 ssize_t index,
>> +                                 void *p_data)
>> +{
>> +    if (index == UINT64_MAX) {
>> +        NSLog(@"%s: no current item", __func__);
>> +    }
>> +    VLCPlaylistController *playlistController = (__bridge VLCPlaylistController *)p_data;
>> +    [playlistController currentPlaylistItemChanged:index];
>> +
>> +    NSLog(@"%s: index: %zu", __func__, index);
>> +}
> 
> I believe you might need to post all of that into the main thread, no? If so, it should be directly done in the callback, not in any later stage,

This was changed in subsequent commits :)

>> 
>> +#pragma mark - helper methods
>> +
>> +- (input_item_t *)createInputItemBasedOnMetadata:(NSDictionary *)itemToCreateDict
>> +{
>> +    intf_thread_t *p_intf = getIntf();
>> +
>> +    input_item_t *p_input;
>> +    BOOL b_rem = FALSE, b_dir = FALSE, b_writable = FALSE;
>> +    NSString *uri, *name, *path;
>> +    NSURL * url;
>> +    NSArray *optionsArray;
>> +
>> +    /* Get the item */
>> +    uri = (NSString *)[itemToCreateDict objectForKey: @"ITEM_URL"];
>> +    url = [NSURL URLWithString: uri];
>> +    path = [url path];
>> +    name = (NSString *)[itemToCreateDict objectForKey: @"ITEM_NAME“];
> 
> Could we get rid of that weird pattern, please? ;-)
> If we need to pass more than one object, it should be better wrapped in a proper object, not in those directories with magic keys hardcoded here…

Absolutely. I just did not want to re-write the open window controller and the playlist at the same so there is a chance to actually follow the changes. This will disappear over the next couple of days.


>> +    optionsArray = (NSArray *)[itemToCreateDict objectForKey: @"ITEM_OPTIONS"];
>> +
>> +    if ([[NSFileManager defaultManager] fileExistsAtPath:path isDirectory:&b_dir]
>> +        && b_dir &&
>> +        [[NSWorkspace sharedWorkspace] getFileSystemInfoForPath:path
>> +                                                    isRemovable:&b_rem
>> +                                                     isWritable:&b_writable
>> +                                                  isUnmountable:NULL
>> +                                                    description:NULL
>> +                                                           type:NULL]
>> +        && b_rem && !b_writable && [url isFileURL]) {
>> +
>> +        NSString *diskType = getVolumeTypeFromMountPath(path);
>> +        msg_Dbg(p_intf, "detected optical media of type %s in the file input", [diskType UTF8String]);
>> +
>> +        if ([diskType isEqualToString: kVLCMediaDVD])
>> +            uri = [NSString stringWithFormat: @"dvdnav://%@", getBSDNodeFromMountPath(path)];
>> +        else if ([diskType isEqualToString: kVLCMediaVideoTSFolder])
>> +            uri = [NSString stringWithFormat: @"dvdnav://%@", path];
>> +        else if ([diskType isEqualToString: kVLCMediaAudioCD])
>> +            uri = [NSString stringWithFormat: @"cdda://%@", getBSDNodeFromMountPath(path)];
>> +        else if ([diskType isEqualToString: kVLCMediaVCD])
>> +            uri = [NSString stringWithFormat: @"vcd://%@#0:0", getBSDNodeFromMountPath(path)];
>> +        else if ([diskType isEqualToString: kVLCMediaSVCD])
>> +            uri = [NSString stringWithFormat: @"vcd://%@@0:0", getBSDNodeFromMountPath(path)];
>> +        else if ([diskType isEqualToString: kVLCMediaBD] || [diskType isEqualToString: kVLCMediaBDMVFolder])
>> +            uri = [NSString stringWithFormat: @"bluray://%@", path];
>> +        else
>> +            msg_Warn(getIntf(), "unknown disk type, treating %s as regular input", [path UTF8String]);
> 
> this looks a lot like duplicated code.

No, the originating class will cease to exist.

Looking forward to further feedback from you!

Cheers,

Felix