VLCMediaLibraryManager: Basic migration of MLLabels and MLFiles #135

Mikanbu · 2018-08-29T17:27:04Z

Checklist

I've run bundle exec fastlane test from the root directory to see all new and existing tests pass
I've followed the vlc-ios code style and run bundle exec fastlane lint to ensure the code style is valid
I've read the Contribution Guidelines
I've updated the documentation if necessary.

Description

Basic migration of MLLabels and MLFiles.

Small note: This need a fresh build of VLCMediaLibraryKit to work since the changes haven't been released yet.

SharedSources/VLCMediaLibraryManager.swift

Mikanbu · 2018-09-03T09:25:29Z

Updated new VLCMediaLibraryKit changes.

carolanitz · 2018-09-03T09:34:13Z

can you move the migration out of didFinishlaunchingWithoptions or make sure that that method returns fast enough. The App gets killed if we take too long (like 20 to 30 secs) can you also check if the users kills the app during migration that you don't have duplicates?

carolanitz · 2018-09-03T09:35:16Z

SharedSources/VLCMediaLibraryManager.swift

+// MARK: - Migration
+
+private extension VLCMediaLibraryManager {
+    // Ugly hack to get the realPath


can we do it without that hack?

It won't work without it, we do need the real path for the medialibrary , it's the ugly solution I found for it.

we need the real path only because the fix to adjust it to a relative path hasn't made it yet into the medialibrary correct?If so can we should push for that to be fixed

Mikanbu · 2018-09-03T09:36:22Z

Are you talking about the setup in didFinishlaunchingWithOptions ?

carolanitz · 2018-09-03T09:38:39Z

SharedSources/VLCMediaLibraryManager.swift

+            self.migrateToNewMediaLibrary()
+            UIApplication.shared.keyWindow?.rootViewController?.dismiss(animated: true, completion: nil)
+        }
+        UIApplication.shared.keyWindow?.rootViewController?.present(migrationViewController, animated: true, completion: nil)


This code belongs into a viewcontroller, preferrably on the rootviewcontroller

carolanitz · 2018-09-03T09:41:40Z

SharedSources/VLCMediaLibraryManager.swift

+        }
+
+        for media in allFiles {
+            if let migratedMedia = fetchMedia(with: realPath(to: media.url)) {


can you have a media without url and the case that fetchMedia fails is not handled

A media needs to have an mrl, if it doesn't then it cannot be migrated.

SharedSources/VLCMediaLibraryManager.swift

carolanitz · 2018-09-03T11:14:54Z

SharedSources/VLCMediaLibraryManager.swift

+            return
+        }
+
+        for media in allFiles {


can we rename it to oldMedia and newMedia?

carolanitz · 2018-09-03T11:15:25Z

SharedSources/VLCMediaLibraryManager.swift

+        }
+
+        for label in  allLabels {
+            var mediaForPlaylist = [VLCMLMedia]()


shouldn't that media already exist or is this the playlist?

Not sure to understand what you mean here.

is this a playlist? or is this media that gets added to a Playlist? the title suggest the second and if so this should already exist and just be retrieved to be added to the newly created playlist

carolanitz · 2018-09-03T11:16:38Z

SharedSources/VLCMediaLibraryManager.swift

+                    mediaForPlaylist.append(migratedMedia)
+                }
+            }
+            let migratedPlaylist = createPlaylist(with: label.name)


newPlaylist

carolanitz · 2018-09-03T11:17:47Z

Sources/VLCAppDelegate.m

@@ -84,6 +82,25 @@ + (void)initialize
    [defaults registerDefaults:appDefaults];
 }

+- (void)setup
+{
+    void (^setupLibraryBlock)(void) = ^{


can we update the name setupLibraryBlock ?

Yes we can~

carolanitz · 2018-09-03T11:18:34Z

Sources/VLCAppDelegate.m

@@ -269,7 +242,7 @@ - (void)applicationWillResignActive:(UIApplication *)application

 - (void)applicationDidBecomeActive:(UIApplication *)application
 {
-    if (!_isRunningMigration && !_isComingFromHandoff) {
+    if (!_isComingFromHandoff) {


why did you remove this and why is it not handled anymore?

I don't think we need it anymore, do you see a need for this bool?

do you understand why it was here? the code in the next line causes access to the medialibrary which would crash the entire application if the Medialibrary was being migrated

Yes but if I'm not mistaken, we don't migrate the old medialibrary anymore.

that's true but shouldn't we update here the new Medialibrary and metadata and only do that if we're not migrating?

I don't think we need to update the new Medialibrary here, I might be missing some cases?

okay so when files were deleted while the App was not running the mediadatabase handles it automatically right? This used to be also the point where we would adjust the searchable index to remove and add deleted or added files but do we move that somewhere else then? In that case it might make sense to remove the old updateMediaDatabase call and VLCMediaFileDiscoverer code as well. Additionally since we're not accessing the mediadatabase from recoverDisplayedMetadata( at least from what I can see) we don't need the check for _isComingFromhandoff.

carolanitz

Migrations are hard, there are lots of things to consider and we don't want to mess up users files or they will hate us so I'm a bit anal here. This iteration is already better. There are still a couple of things that need to be double checked.

that there is no code that would access the Medialib while it is migrating. This can happen if you open the app through "Open in" or openURL, or click on a CSSearchable Item or force touch the vlc icon, or or or.
test all combinations that a user can have with the migration to make sure everything works correctly. Only files, files and folders, folders with mixed media, only episodes or albumtracks, empty folders, only folders and so on.

carolanitz · 2018-09-10T09:08:07Z

SharedSources/VLCMediaLibraryManager.swift

+// MARK: - Migration
+
+private extension VLCMediaLibraryManager {
+    // Ugly hack to get the realPath


we need the real path only because the fix to adjust it to a relative path hasn't made it yet into the medialibrary correct?If so can we should push for that to be fixed

carolanitz · 2018-09-10T09:11:39Z

SharedSources/VLCMediaLibraryManager.swift

+            return true
+        }
+        return false
+    }


don't we need the result to not access the medialibrary while we migrate?

carolanitz · 2018-09-10T09:35:24Z

Sources/VLCAppDelegate.m

@@ -269,7 +242,7 @@ - (void)applicationWillResignActive:(UIApplication *)application

 - (void)applicationDidBecomeActive:(UIApplication *)application
 {
-    if (!_isRunningMigration && !_isComingFromHandoff) {
+    if (!_isComingFromHandoff) {


okay so when files were deleted while the App was not running the mediadatabase handles it automatically right? This used to be also the point where we would adjust the searchable index to remove and add deleted or added files but do we move that somewhere else then? In that case it might make sense to remove the old updateMediaDatabase call and VLCMediaFileDiscoverer code as well. Additionally since we're not accessing the mediadatabase from recoverDisplayedMetadata( at least from what I can see) we don't need the check for _isComingFromhandoff.

carolanitz · 2018-09-10T09:48:16Z

SharedSources/VLCMediaLibraryManager.swift

+    // Note: This removes **only** files that are in a playlist
+    func migratePlaylists(_ oldMedialibrary: MLMediaLibrary) -> Bool {
+        guard let allLabels = MLLabel.allLabels() as? [MLLabel] else {
+            assertionFailure("VLCMediaLibraryManager: Migration: Unable to retreive all labels")


why is this a developer failure ? wouldn't that happen when there are no folders in a library?

Ah when you have no folders this method returns an empty array so this case would not happen. For some reason I thought this could return nil

carolanitz · 2018-09-10T09:52:23Z

SharedSources/VLCMediaLibraryManager.swift

+            return false
+        }
+
+        if migrateMedia(oldMedialibrary) && migratePlaylists(oldMedialibrary) {


these calls should not be done on the mainthread because that would block the UI. the migration should be dispatched to a background thread so that the activityspinner can spin when we show the migrationcontroller

Also if you return false for migratePlaylists when there are no folders in someones library you would always do a migration correct and never mark it as migrated

Also if you return false for migratePlaylists when there are no folders in someones library you would always do a migration correct and never mark it as migrated

Not sure how that could happen.

these calls should not be done on the mainthread because that would block the UI.

I agree, but if I'm not mistaken they are not in the main thread since it launched on the didCompleteDiscovery thread.

ah nice! I just double checked and in that case we're okay for the userdefaults since they're threadsafe but I would dispatch the delegate calls back to the mainthread because you do UI work. It does work though without mainthread checker warnings which is surprising for me

carolanitz · 2018-09-10T09:54:19Z

Sources/Coordinators/AppCoordinator.swift

+    }
+
+    func medialibraryDidFinishMigration(_ medialibrary: VLCMediaLibraryManager) {
+        if tabBarController.presentedViewController === migrationViewController {


could you even just call migrationViewController.dismiss ?

Mostly to double check but I could.

carolanitz · 2018-09-11T11:23:24Z

SharedSources/VLCMediaLibraryManager.swift

+        for media in allFiles {
+            if let newMedia = fetchMedia(with: realPath(to: media.url)) {
+                newMedia.updateTitle(media.title)
+                newMedia.setPlayCount(media.playCount.uint32Value)


it fails for me here with "error: value of type 'VLCMLMedia' has no member 'setPlayCount'"

Yes, you need a fresh build of the wrapper, I didn't release the update it yet.

should we also set the Metadata field "Seen" here ?

Yes, we definitely could, but it's should be more or less implicit with the play count if it's correctly set.

you're right, that it is implicit but we will forget that at some point and just check for for the isSeen flag. For the new tag for example and we claim this media was never seen so it would show it.

Sorry, I planned on doing it but I completely forgot about it! It should be all good now. 👍

carolanitz · 2018-09-13T13:22:55Z

okay just need the new wrapper and also set the seen metadata field as well :) Everything else looks good to me!

Mikanbu · 2018-10-07T11:08:44Z

Rebased and removed the ugly realpath hack! 🎉
A fresh VLCMediaLibraryKit is needed, 0.0.7 doesn't include realpath changes

carolanitz · 2018-10-07T17:59:37Z

okay then just the new Medialib and the isseen flag and we can merge \o/

Mikanbu · 2018-11-19T14:51:09Z

Rebased, re-tested and made CircleCI happy! 🎉
Waiting for a last review to merge.

VLCMigrationViewController: Remove completionHandler AppDelegate: Remove migration related code

Mikanbu added the WIP Work in Progress label Aug 29, 2018

Mikanbu requested a review from carolanitz August 29, 2018 17:27

carolanitz reviewed Aug 29, 2018

View reviewed changes

SharedSources/VLCMediaLibraryManager.swift Outdated Show resolved Hide resolved

Mikanbu removed the WIP Work in Progress label Aug 30, 2018

Mikanbu force-pushed the medialibrary/migration/initial branch 2 times, most recently from 9fd7bff to 7b0a18f Compare September 3, 2018 09:25

carolanitz reviewed Sep 3, 2018

View reviewed changes

carolanitz requested changes Sep 3, 2018

View reviewed changes

Mikanbu force-pushed the medialibrary/migration/initial branch 2 times, most recently from f69f218 to 16a1573 Compare September 3, 2018 12:18

Mikanbu added the WIP Work in Progress label Sep 3, 2018

Mikanbu force-pushed the medialibrary/migration/initial branch from 348bbcb to 5237e1c Compare September 5, 2018 13:04

Mikanbu removed the WIP Work in Progress label Sep 6, 2018

carolanitz requested changes Sep 10, 2018

View reviewed changes

carolanitz reviewed Sep 11, 2018

View reviewed changes

carolanitz assigned Mikanbu Sep 19, 2018

Mikanbu force-pushed the medialibrary/migration/initial branch from 5237e1c to 86522be Compare October 7, 2018 11:07

Mikanbu force-pushed the medialibrary/migration/initial branch from aa11bf8 to 5b50845 Compare November 19, 2018 14:29

Mikanbu added 5 commits November 19, 2018 16:17

VLCMediaLibraryManager: Basic migration of MLLabels and MLFiles

b9bef05

Remove deprecated medialibrary code

a18987e

VLCMigrationViewController: Remove completionHandler AppDelegate: Remove migration related code

AppDelegate: Move setupBlock to a method

c28b88e

AppDelegate: Rename setupLibraryBlock

9099f28

AppCoordinator: Prepare for migration

bb5599e

Mikanbu force-pushed the medialibrary/migration/initial branch from 5b50845 to bb5599e Compare November 19, 2018 15:21

carolanitz approved these changes Nov 19, 2018

View reviewed changes

vlc-mirrorer merged commit bb5599e into videolan:master Nov 19, 2018

VLCMediaLibraryManager: Basic migration of MLLabels and MLFiles #135

VLCMediaLibraryManager: Basic migration of MLLabels and MLFiles #135

Conversation

Mikanbu commented Aug 29, 2018

Checklist

Description

Mikanbu commented Sep 3, 2018

carolanitz commented Sep 3, 2018

carolanitz Sep 3, 2018 • edited

Choose a reason for hiding this comment

Mikanbu Sep 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikanbu commented Sep 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolanitz Sep 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolanitz Sep 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolanitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carolanitz commented Sep 13, 2018

Mikanbu commented Oct 7, 2018 • edited

carolanitz commented Oct 7, 2018

Mikanbu commented Nov 19, 2018

carolanitz Sep 3, 2018 •

edited

Mikanbu Sep 3, 2018 •

edited

carolanitz Sep 3, 2018 •

edited

carolanitz Sep 3, 2018 •

edited

Mikanbu commented Oct 7, 2018 •

edited