Some improvements and #8

[fwx]

Hey! I found your post on reddit and decided that I could help a little.

Regarding issue #8 I did change songDict to be a struct and did some Library / Player improvements (at least, I think those are improvements :)).

If you're okay with the code you can clone my repo and test if all the logic works as intended.
Because it was quite hard for me to decide how cache/metadata works with dictionaries. I'm open to fix the bugs that I introduced, especially, editing song's artists, tags, etc.

Some tips:
Get rid of NSMutabbleArrays and NSArrays since there is not point to use those in swift. I tried to remove some, but my main point was changing dictionary to struct.

LibraryManager's getLibraryArray (in my code and in yours) is pretty heavy and could lead to bottlenecks when there are too many songs.

Same goes for applyTagFilter IMO.

I found that you often doing the force unwraps, which are not good tactics.

edit: fork -> clone

[youstanzr]

This is great, really appreciate the contribution.
I haven't gone through the code yet but skimmed through and it looks nice. For now, my comments code-wise are:

• I am keeping all extensions in Extensions.swift file so we can also add the orEmpty extension of Optional to the same file.
• A new struct SongTest was introduced but never used so we can delete that.

For a quick testing I installed the app to try it. So, I downloaded couple of songs and tested some functionalities andI here is my feedback so far:

• After you download a new song and launching the song detail view, I found that it was showing an empty tag for the song.
• App crashes when trying to delete a song from the library.
• Added artist, tag, lyrics, or album are not getting saved.
• Wrong song detail presented after downloading a song from a direct link.
• Failed to extract song metadata from mp3 file.
• The order of library tableview and playlist library tableview is not reversed anymore (showing new songs at top).

I was also thinking that we can remove support to the old song Dict style and instead just save and load a Song struct directly instead of having the cache presentable layer in between. I have not decided on this part yet so feel free to share any ideas

[fwx]

Yea, SongTest is a mistake, forgot I added it.
Regarding Extensions.swift and extensions in general. I prefer that extensions are separated by their types and helping functions in general, but I am okay to just sent them into a single file.

The bugs you mentioned - I'll look into them today or tomorrow, it was just hard to test because I was running out of my free time. Another problem which affects most of the bugs I guess is that metadata/cache parsing stuff. I misunderstood how it works in the first place.

Song dict style can be removed easily, since Song inherits from Codable which can be stored as JSON data via JSONEncoder/JSONDecoder, but my problem was that I wanted to keep your old cache system and dont break it with my PR. But I guess I can try to implement some migrations.
Like initially load LibraryArray, parse NSArray into Array of Song and then save it as LibrarryArray_v2 or something like that. I guess after finishing this PR I'll look into that.

By the way, when I was doing some kind of player, I've used Realm as cache provider. Its really fast, allows to listen for objects updates and much more, but the only thing that bugged me was thread safety. Just telling that you can look into Realm.

[youstanzr]

I was afraid that separating the extensions into multiple files for each type is going to overwhelm the project with just too many file so it felt right to just have them in a single file but separate them by //MARK.

Feel free to do those fixes anytime and I'll be happy to review and merge.

Regarding saving the Song as JSON, this was my initial intention when I started the project but as this is my first Swift project and I wanted to actually see the output rather than getting stuck with the syntax and protocols, I just wrote it the easiest way and thought that I can refactor later. Unfortunately, I got stuck while working with JSONEncoder/JSONDecoder so I just stopped there and did it the dictionary way. Thats why I lean more toward the JSON approach instead having to struggle with caching the old library (Also this would help with library back up #41 ). So for that, I think migration code would be the best approach with a new saving key like LibraryJSON.

Tbh I've never worked or looked into Realm but sure I'll read about it.

Also, I saw you raised some points regarding getLibraryArray and applyTagFilter functions and I was wondering if you have any ideas on how to improve it.

[youstanzr]

Any update on this pull?