New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VLCMediaList: removeMediaAt (closes #182) #34

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@mkchoi212
Contributor

mkchoi212 commented Aug 1, 2018

mkchoi212 added some commits Jul 26, 2018

VLCMediaList: Update indexOfMedia
- Returns NSUInteger instead of NSInteger
- Returns NSNotFound if media is not found
{
VLCMedia *target = [_mediaObjects objectAtIndex:index];

This comment has been minimized.

@mkchoi212

mkchoi212 Aug 1, 2018

Contributor

objectAtIndex checks if the provided index is in range

This comment has been minimized.

@fkuehne

fkuehne Aug 1, 2018

Contributor

This will lead to an out-of-bounds exception and early application termination by the runtime if the index is not in range. Please add sanity checks.

This comment has been minimized.

@mkchoi212

mkchoi212 Aug 2, 2018

Contributor

I personally think this is the desirable behavior for the end user. I had a hard time debugging things when removeMediaAtIndex function wasn't critically failing even when an invalid index was passed to it.

This comment has been minimized.

@fkuehne

fkuehne Aug 2, 2018

Contributor

No, the library shouldn't crash due to a potentially inconsistent state.

* \note this function silently fails if the list is read-only
*/
- (void)removeMediaAtIndex:(NSUInteger)index;
- (VLCMedia *)removeMediaAtIndex:(NSUInteger)index;

This comment has been minimized.

@mkchoi212

mkchoi212 Aug 1, 2018

Contributor

Replicates Swift STL's func remove(at: Int) -> T

This comment has been minimized.

@fkuehne

fkuehne Aug 1, 2018

Contributor

I don't understand why you want to return a VLCMedia instance that is gone from the list.

This comment has been minimized.

@mkchoi212

mkchoi212 Aug 2, 2018

Contributor

Swift STL's remove(at:) behaves the same way and if the function doesn't returns void, how do I easily tell if a media had been deleted or not? A way to do is to raise NSRangeException, which leads to the use of indexOf without sanity checks in the function. Sorry if I'm not making sense 😅

This comment has been minimized.

@fkuehne

fkuehne Aug 2, 2018

Contributor

Considering BOOL as a return value to expose the successful deletion. This matches typical AppKit behavior.

@mkchoi212 mkchoi212 force-pushed the mkchoi212:medialist-fix branch from a129215 to d2eaa5b Aug 1, 2018

@@ -217,11 +217,6 @@ - (void)insertObject:(VLCMedia *)object inMediaAtIndex:(NSUInteger)i
[self insertMedia:object atIndex:i];
}
- (void)removeObjectFromMediaAtIndex:(NSUInteger)i

This comment has been minimized.

@mkchoi212

mkchoi212 Aug 1, 2018

Contributor

Seems like it's not being used anywhere

This comment has been minimized.

@fkuehne

fkuehne Aug 1, 2018

Contributor

Correct.

@mkchoi212 mkchoi212 referenced this pull request Aug 1, 2018

Open

Tests: VLCMediaList #35

@fkuehne fkuehne self-requested a review Aug 1, 2018

{
VLCMedia *target = [_mediaObjects objectAtIndex:index];

This comment has been minimized.

@fkuehne

fkuehne Aug 1, 2018

Contributor

This will lead to an out-of-bounds exception and early application termination by the runtime if the index is not in range. Please add sanity checks.

* \note this function silently fails if the list is read-only
*/
- (void)removeMediaAtIndex:(NSUInteger)index;
- (VLCMedia *)removeMediaAtIndex:(NSUInteger)index;

This comment has been minimized.

@fkuehne

fkuehne Aug 1, 2018

Contributor

I don't understand why you want to return a VLCMedia instance that is gone from the list.

@@ -217,11 +217,6 @@ - (void)insertObject:(VLCMedia *)object inMediaAtIndex:(NSUInteger)i
[self insertMedia:object atIndex:i];
}
- (void)removeObjectFromMediaAtIndex:(NSUInteger)i

This comment has been minimized.

@fkuehne

fkuehne Aug 1, 2018

Contributor

Correct.

});
[self didChange:NSKeyValueChangeRemoval valuesAtIndexes:[NSIndexSet indexSetWithIndex:[index intValue]] forKey:@"media"];

This comment has been minimized.

@fkuehne

fkuehne Aug 1, 2018

Contributor

Why do you remove support for key-value coding here?

This comment has been minimized.

@mkchoi212

mkchoi212 Aug 2, 2018

Contributor

Ah I forgot to ask you about this. I personally didn't know you could do this until I saw the code for it. So, is this a feature that is used by users when they already have Notifications and delegates they can check?

This comment has been minimized.

@fkuehne

fkuehne Aug 2, 2018

Contributor

This is a used feature throughout multiple places of VLCKit, notably VLCMedia and VLCMediaPlayer. Especially the VLC-iOS app uses it.

if (index >= [_mediaObjects count])
return;
//remove from cached Media
// Remove from cached Media

This comment has been minimized.

@fkuehne

fkuehne Aug 1, 2018

Contributor

hmm, why do you remove the sanity check here? This is not a fail-safe operation.

@mkchoi212 mkchoi212 force-pushed the mkchoi212:medialist-fix branch from d2eaa5b to 1ae63ef Aug 3, 2018

@@ -104,12 +104,13 @@ extern NSString *const VLCMediaListItemDeleted;
- (void)insertMedia:(VLCMedia *)media atIndex:(NSUInteger)index;
/**
* remove a media from a given position
* remove and return the boolean result of the operation

This comment has been minimized.

@carolanitz

carolanitz Aug 6, 2018

Member

remove media at position index and return true if the operation was successful. An unsuccessful operation occurs when the index is greater than the medialists count

This comment has been minimized.

@mkchoi212

mkchoi212 Aug 7, 2018

Contributor

Well said 👍

- (BOOL)isEqual:(id)other
{
return ([other isKindOfClass: [VLCMedia class]] &&
[other libVLCMediaDescriptor] == p_md);

This comment has been minimized.

@carolanitz
{
NSInteger result = libvlc_media_list_index_of_item(p_mlist, [media libVLCMediaDescriptor]);
return result;
return [_mediaObjects indexOfObject:media];

This comment has been minimized.

@carolanitz

carolanitz Aug 6, 2018

Member

Do all these changes to indexOfMedia and removeMediaAtIndex work with discovered media in a network as well? Being in a WLAN with a NAS or twonky server for example should be enough to test these changes

This comment has been minimized.

@mkchoi212

mkchoi212 Aug 7, 2018

Contributor

I think so. There is no reason why it shouldn't. And not really sure how to test

Being in a WLAN with a NAS or twonky server

😅

This comment has been minimized.

@carolanitz

carolanitz Aug 7, 2018

Member

you can try it for 30 days :) https://twonky.com/

This comment has been minimized.

@carolanitz

carolanitz Aug 7, 2018

Member

and then you set it up build vlc ios with the custom vlckit and just check as it discovers files that all files are discovered and that it doesn't crash somewhere unexpected and when you delete one out of the discovered folder that it all works as expected

This comment has been minimized.

@mkchoi212

mkchoi212 Aug 10, 2018

Contributor

Just confirmed it still works and found a bug while at it :D

{
NSNumber *index = arguments[0][@"index"];
VLCMedia *deleted = arguments[0][@"media"];

This comment has been minimized.

@carolanitz

carolanitz Aug 6, 2018

Member

can you use .first or firstobject instead of [0]?

@mkchoi212 mkchoi212 force-pushed the mkchoi212:medialist-fix branch from 1ae63ef to 14c309c Aug 7, 2018

@carolanitz carolanitz self-requested a review Aug 23, 2018

@carolanitz

looks really good!

@carolanitz

This comment has been minimized.

Member

carolanitz commented Aug 23, 2018

this was merged with 005bcd7 & 759bad2 & 0735d2b & 6413080

@carolanitz carolanitz closed this Aug 23, 2018

@mkchoi212 mkchoi212 deleted the mkchoi212:medialist-fix branch Aug 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment