change(mac): responded to review comments

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.h

@@ -1,4 +1,4 @@
-/**
+/*
  * Keyman is copyright (C) SIL International. MIT License.
  *
  * KMDataRepository.h
@@ -13,9 +13,9 @@
 NS_ASSUME_NONNULL_BEGIN
 
 @interface KMDataRepository : NSObject
-@property (readonly) NSURL *keymanDataDirectory;      // '~/Library/Application Support/com.keyman.app'
+@property (readonly) NSURL *keymanDataDirectory;      // '~/Library/Application Support/keyman.inputmethod.Keyman'
 @property (readonly) NSURL *keymanKeyboardsDirectory;
-// keymanKeyboardsDirectory = '~/Library/Application Support/com.keyman.app/Keyman-Keyboards'
+// keymanKeyboardsDirectory = '~/Library/Application Support/keyman.inputmethod.Keyman/Keyman-Keyboards'
 + (KMDataRepository *)shared;
 - (void)createDataDirectoryIfNecessary;
 - (void)createKeyboardsDirectoryIfNecessary;

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.m

@@ -1,4 +1,4 @@
-/**
+/*
  * Keyman is copyright (C) SIL International. MIT License.
  *
  * KMResourcesRepository.m
@@ -15,30 +15,39 @@
 #import "KMLogs.h"
 
 @interface KMDataRepository ()
-@property (readonly) NSURL *applicationSupportSubDirectory;   // '~/Library/Application Support'
-@property (readonly) NSURL *documentsSubDirectory;    // '~/Documents'
-@property (readonly) NSURL *obsoleteKeymanKeyboardsDirectory; // '~/Library/Documents/Keyman-Keyboards'
+@property (readonly) NSURL *applicationSupportSubDirectory;
+@property (readonly) NSURL *documentsSubDirectory;
+@property (readonly) NSURL *obsoleteKeymanKeyboardsDirectory;
 @end
 
 @implementation KMDataRepository
-
+/**
+ * Two directory trees are represented by the following properties, one in active use
+ * and one that is obsolete.
+ * The actively used directories, begin with the parent
+ *    applicationSupportSubDirectory: '~/Library/Application Support'
+ *      keymanDataDirectory: '~/Library/Application Support/keyman.inputmethod.Keyman'
+ *        keymanKeyboardsDirectory: '~/Library/Application Support/keyman.inputmethod.Keyman/Keyman-Keyboards'
+ * The obsolete directories, begin with the parent
+ *    documentsSubDirectory: '~/Documents'
+ *      obsoleteKeymanKeyboardsDirectory: '~/Documents/Keyman-Keyboards'
+ */
 @synthesize applicationSupportSubDirectory = _applicationSupportSubDirectory;
-@synthesize documentsSubDirectory = _documentsSubDirectory;
+@synthesize keymanDataDirectory = _keymanDataDirectory;
 @synthesize keymanKeyboardsDirectory = _keymanKeyboardsDirectory;
+@synthesize documentsSubDirectory = _documentsSubDirectory;
 @synthesize obsoleteKeymanKeyboardsDirectory = _obsoleteKeymanKeyboardsDirectory;
-@synthesize keymanDataDirectory = _keymanDataDirectory;
 
 NSString *const kKeyboardsDirectoryName = @"Keyman-Keyboards";
 /* 
  The name of the subdirectory within '~/Library/Application Support'.
- The convention is to use bundle identifier.
- (Using the subsystem id, "com.keyman.app", is a poor choice because the API
+ We follow the convention of using the bundle identifier rather than our subsystem id.
+ (Also, using the subsystem id, "com.keyman.app", is a poor choice because the API
  createDirectoryAtPath sees the .app extension and creates an application file.
  */
 NSString *const kKeymanSubdirectoryName = @"keyman.inputmethod.Keyman";
 
-+ (KMDataRepository *)shared
-{
++ (KMDataRepository *)shared {
   static KMDataRepository *shared = nil;
   static dispatch_once_t onceToken;
   dispatch_once(&onceToken, ^{
@@ -149,7 +158,11 @@ - (NSURL *)obsoleteKeymanKeyboardsDirectory {
   }
   return _obsoleteKeymanKeyboardsDirectory;
 }
-
+/**
+ *  Only called from migrateData.
+ *  Causes user to be prompted for permission to access ~/Documents, but they should already have it.
+ *  otherwise we would not be attempting to migrate.
+ */
 - (BOOL)keyboardsExistInObsoleteDirectory {
   NSFileManager *fileManager = [NSFileManager defaultManager];
   BOOL isDir;
@@ -158,7 +171,10 @@ - (BOOL)keyboardsExistInObsoleteDirectory {
 }
 
 /**
- * Migrate the keyboards data from the old location in '/Documents' to the new location '/Application Support/keyman.inputmethod.Keyman/'
+ * Migrate the keyboards data from the old location in '~/Documents' to the new location '~/Application Support/keyman.inputmethod.Keyman/'
+ * This should only be called if the Keyman settings written to the UserDefaults indicates that we have data in the old location.
+ * If this is the case, then we expect the user to have already granted permission for Keyman to access the ~/Documents directory.
+ * If that permission has been removed for some reason, then calling this code will cause the user to be asked for permission again.
  */
 - (BOOL)migrateData {
   BOOL didMoveData = NO;

mac/Keyman4MacIM/Keyman4MacIM/KMInputMethodEventHandler.m

@@ -243,17 +243,6 @@ - (BOOL)handleEvent:(NSEvent *)event client:(id)sender {
       // return NO to pass through to client app
       return NO;
     }
-
-    // TODO: remove test code
-    /*
-    if (event.keyCode == kVK_ANSI_Slash) {
-      if ([KMSettingsRepository.shared keyboardsMigrationNeeded]) {
-        os_log_info([KMLogs startupLog], "keyboards migration needed");
-      } else {
-        os_log_info([KMLogs startupLog], "keyboards migration not needed");
-      }
-    }
-     */
   }
 
   if (event.type == NSEventTypeFlagsChanged) {

mac/Keyman4MacIM/Keyman4MacIM/KMPackageReader.m

@@ -69,18 +69,6 @@ - (KMPackageInfo *)loadPackageInfo:(NSString *)path {
   return packageInfo;
 }
 
-/*
- // TODO: not used, delete
-- (NSString *)packageNameFromPackageInfo:(NSString *)path {
-  NSString *packageName = nil;
-  
-  KMPackageInfo *packageInfo = [self loadPackageInfo:path];
-  packageName = packageInfo.packageName;
-  
-  return packageName;
-}
-*/
-
 /**
  * read JSON file and load it into KMPackageInfo object
  * returns nil if the file does not exist or it cannot be parsed as JSON

mac/Keyman4MacIM/Keyman4MacIM/KMSettingsRepository.m

@@ -1,4 +1,4 @@
-/**
+/*
  * Keyman is copyright (C) SIL International. MIT License.
  *
  * KMSettingsRepository.h
@@ -27,6 +27,7 @@
  */
 NSString *const kDataModelVersion = @"KMDataModelVersion";
 NSInteger const kVersionStoreDataInLibraryDirectory = 1;
+NSInteger const kCurrentDataModelVersionNumber = kVersionStoreDataInLibraryDirectory;
 
 @implementation KMSettingsRepository
 
@@ -41,7 +42,7 @@ + (KMSettingsRepository *)shared
 }
 
 - (void)setDataModelVersionIfNecessary {
-  if (![self dataStoredInLibraryDirectory]) {
+  if (![self dataModelWithKeyboardsInLibrary]) {
     [[NSUserDefaults standardUserDefaults] setInteger:kVersionStoreDataInLibraryDirectory forKey:kDataModelVersion];
   }
 }
@@ -51,34 +52,38 @@ - (void)setDataModelVersionIfNecessary {
  * If this method is called after applicationDidFinishLaunching, then it will always return true.
  * If called from awakeFromNib, then it will return false when running for the first time.
  */
-- (BOOL)settingsExist
-{
+- (BOOL)settingsExist {
   return [[NSUserDefaults standardUserDefaults] objectForKey:kSelectedKeyboardKey] != nil;
 }
 
 /**
- * For the first numbered version of the data model, the app stores the keyboards under the /Library directory
- * For versions before version 1, the keyboards were stored under the /Documents directory.
+ * For the first numbered version of the data model, the app stores the keyboards under the ~/Library directory
+ * For versions before version 1, the keyboards were stored under the ~/Documents directory.
  */
-- (BOOL)dataStoredInLibraryDirectory
-{
-  return [[NSUserDefaults standardUserDefaults] integerForKey:kDataModelVersion] == kVersionStoreDataInLibraryDirectory;
+- (BOOL)dataModelWithKeyboardsInLibrary {
+  // NSUserDefaults returns zero if the key does not exist
+  NSInteger dataModelVersion = [[NSUserDefaults standardUserDefaults] integerForKey:kDataModelVersion];
+
+  return dataModelVersion >= kVersionStoreDataInLibraryDirectory;
 }
 
 /**
- * Determines whether the keyboard data needs to be moved from the old location in ~/Documents to the new location ~/Library...
+ * Determines whether the keyboard data needs to be moved from the old location to the new location
  * This is true if
  * 1) the UserDefaults exist (indicating that this is not a new installation of Keyman) and
- * 2) the value  for KMStoreKeyboardsInLibraryKey is not set to true
+ * 2) the value for kVersionStoreDataInLibraryDirectory is < 1,
  */
 - (BOOL)dataMigrationNeeded {
   BOOL keymanSettingsExist = [self settingsExist];
-  os_log([KMLogs dataLog], "  keyman settings exist: %{public}@", keymanSettingsExist ? @"YES" : @"NO" );
+  os_log([KMLogs dataLog], "keyman settings exist: %{public}@", keymanSettingsExist ? @"YES" : @"NO" );
 
-  BOOL dataInLibrary = [self dataStoredInLibraryDirectory];
-  os_log([KMLogs dataLog], "  data stored in Library: %{public}@", dataInLibrary ? @"YES" : @"NO" );
+  BOOL keyboardsStoredInLibrary = [self dataModelWithKeyboardsInLibrary];
+  os_log([KMLogs dataLog], "settings indicate that keyboards are stored in ~/Library: %{public}@", keyboardsStoredInLibrary ? @"YES" : @"NO" );
 
-  return !(keymanSettingsExist && dataInLibrary);
+  BOOL migrationNeeded = keymanSettingsExist && !keyboardsStoredInLibrary;
+  os_log([KMLogs dataLog], "dataMigrationNeeded: %{public}@", migrationNeeded ? @"YES" : @"NO" );
+
+  return migrationNeeded;
 }
 
 - (NSString *)selectedKeyboard {

mac/Keyman4MacIM/KeymanTests/InputMethodTests.m

@@ -88,18 +88,4 @@ - (void)testCalculateInsertRange_deleteOneBMPCharacterWithOneSelected_returnsRan
   XCTAssertTrue(correctResult, @"insert or replacement range expected to be {1,2}");
 }
 
-- (void)testMigrateData_oldDataExists_logsLocations {
-  os_log_t startupLog = os_log_create("com.keyman.app", "data-migration");
-  if ([KMSettingsRepository.shared dataMigrationNeeded]) {
-    os_log_info(startupLog, "data migration needed, calling migrateData");
-    [KMDataRepository.shared migrateData];
-    os_log_info(startupLog, "test: call migrateData again");
-    [KMDataRepository.shared migrateData];
-  } else {
-    os_log_info(startupLog, "data migration not needed");
-  }
-
-  XCTAssertTrue(YES, @"test failed");
-}
-
 @end