change(mac): store data in Library directory instead of Documents #12106

sgschantz · 2024-08-06T10:58:09Z

Store all keyman keyboards in ~/Library/Application Support/keyman.inputmethod.Keyman/Keyman-Keyboards
instead of the old location ~/Documents/Keyman-Keyboards

The new location is more appropriate because it is the standard location designated by Apple.

Also, beginning with macOS Catalina in 2019, access to the /Documents directory by an application required the consent of the user. The new location will not require special permission.

This change involves

moving all the keyboard data files to the new location and
updating the settings stored in the UserDefaults for Keyman

The UserDefaults path settings need updating for the selected keyboard, the list of active keyboards and the list of keyboards with persisted options.

@keymanapp-test-bot skip

…gration

rather than reading from disk and adding keyboards include awakeFromNib, do so in applicationDidFinishLaunching

Write new key KMDataModelVersion in UserDefaults with integer value that determines how data is expected to be stored. Migrates existing Keyman data to new location if it was written with an older (or no) version. Fixes: #2542

keymanapp-test-bot · 2024-08-06T10:58:12Z

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

macOS
- Keyman for macOS

mcdurdin · 2024-08-11T00:36:10Z

This seems like it needs a bunch of user tests around install and upgrade scenarios.

sgschantz · 2024-08-12T02:36:59Z

This seems like it needs a bunch of user tests around install and upgrade scenarios.

The user tests are going to be under #12144. (The first one is there.) That is the second phase of this change, and I wanted to avoid doing the user tests until both parts are in place because the data format is changing from the first to the second.

mcdurdin

I like this. I have some nits, mostly comments I think are out of sync with implementation in a few places, but one thing that looks like a bug

mcdurdin · 2024-08-19T03:40:49Z

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.m

+@property (readonly) NSURL *documentsSubDirectory;    // '~/Documents'
+@property (readonly) NSURL *obsoleteKeymanKeyboardsDirectory; // '~/Library/Documents/Keyman-Keyboards'


What's the difference between the two Documents paths?

The documentsSubDirectory is the user's Documents directory, identified using an API. The obsolete directory is the full path to the place we formerly stored data under the Documents directory.

Looking at the code, I can eliminate the property documentsSubDirectory as it is only used in finding the full obsolete data path. Would that help simplify this? It is a special location, so I thought it would be useful to save the reference, but we really never want to access it again after the migration.

I'll change the comments to further clarify.

After working on the comments a little. I think I need a better comment that explains the hierarchical relationship between these. We have three directories we should know of going forward and two obsolete ones.

Yes, sounds good, thanks

mcdurdin · 2024-08-19T03:42:11Z

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.m

+ (Using the subsystem id, "com.keyman.app", is a poor choice because the API
+ createDirectoryAtPath sees the .app extension and creates an application file.


The comments in KMDataRepository.h:L16-18 says that we are using com.keyman.app. So is that a poor choice?

Yes, I'll fix the comments which are incorrect. I prefer com.keyman.app as an app ID but since our app ID is already set to keyman.inputmethod.Keyman and com.keyman.app looks to the file manager like a filename of type .app, I plan to stick with keyman.inputmethod.Keyman as the name of our app's data directory.

mcdurdin · 2024-08-19T03:42:28Z

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.m

+ (KMDataRepository *)shared
+{


Suggested change

+ (KMDataRepository *)shared

{

+ (KMDataRepository *)shared {

mcdurdin · 2024-08-19T03:43:55Z

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.m

+
+- (NSURL *)keymanDataDirectory {
+  if (_keymanDataDirectory == nil) {
+    NSURL *keymanDataUrl = [self.applicationSupportSubDirectory URLByAppendingPathComponent:kKeymanSubdirectoryName  isDirectory: TRUE];


What happens if applicationSupportSubDirectory returns nil? Here and other similar calls in various places

I cannot think of anything intelligent to do other than log an error if the OS does not return it to me. This is the place that we are supposed to create our app subdirectory for our user data, so if it doesn't exist, then we don't have a fallback location that I am aware of. In my 'Application Support' directory, there are 91 subdirectories created by other software, so it would be a major failure if it weren't there.

Yeah I guess so. But currently if it does go wrong we'll crash the app. And if is never going to go wrong, then why bother with the additional error logging? So it feels like we are half way.

mcdurdin · 2024-08-19T03:45:23Z

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.m

+}
+
+/**
+ * Creates Keyman data directory if it do not exist yet. This is the main data subdirectory: keyman.inputmethod.Keyman


In the header it says the data directory is '~/Library/Application Support/com.keyman.app'

The header was wrong. 😒

mcdurdin · 2024-08-19T03:51:32Z

mac/Keyman4MacIM/Keyman4MacIM/KMSettingsRepository.h

+/**
+ * Keyman is copyright (C) SIL International. MIT License.
+ *
+ * KMSettingsRepository.h
+ * Keyman
+ *
+ * Created by Shawn Schantz on 2024-07-29.
+ *
+ */


Just a nit: these headers don't quite match the code style guide -- we don't normally include filename, create date, author. Also just a single star on the first line.

https://github.com/keymanapp/keyman/wiki/Keyman-Code-Style-Guide#File-header

Ugh, I thought I was following a pattern that I copied from the standards a while back. I thought we even had a discussion about the date format.

I think that author and date can be very helpful to understand the context of the code at a glance without jumping into git. We can talk separately about this, but Is there a strong reason to exclude those?

I think that author and date can be very helpful to understand the context of the code at a glance without jumping into git. We can talk separately about this, but Is there a strong reason to exclude those?

The primary reasons not to do it are:

avoid multiple "sources of truth" -- the initial creator of the file may well not be the most relevant, nor the create date

avoid busy work

I think we can be pretty confident that git will get it right. In most good code editors, we can see git blame line-by-line, which nicely eliminates the burden of seeing the git data. I would encourage going to git for this kind of metadata -- commit history is really helpful (probably more helpful than original author + date IMO).

The only real downside I can see is if you want to see the original commit: if the header changes over time, then the git blame will show most recent commit against it by default, not the original, and it takes a little more digging to get back to the first commit.

I don't think any of these things override the practical value of this tiny piece of metadata: who created this thing and when. It is written once in the life of the file, and only ever updated with maybe a second author if there is a major redesign within the same code.

In most cases, it will remain as one name and one date but can reveal a lot on its own without scanning the commit history. If it doesn't seem helpful, it can easily be ignored along with the copyright statement. In my experience, the initial author is heavily involved in the design of the code (often the sole person responsible), and a lot of the original design persists even after many edits and attempts to refactor away from the original design.

For the Mac code at least, it looks like the first commit was not the author at all but just the guy who first made use of git. So the real author would be anonymous if he hadn't included his name in the header.

In short, I don't see this as redundant information but something with long-term practical value and maintenance cost that is approximately zero.

The file name and project name does seem like redundant busywork in the header, so I agree that we should remove those. (I've changed the class name many times and forgotten to update the header.)

OK I am not overly bothered either way. Personally I feel like it is busy work which replicates what we now have in git. Yes, back in the day we didn't use git but that's a slippery argument because then we end up putting all commit history back into source files because what if we change vcs in the future and cannot transfer commit history and well yuck.

See for example https://github.com/keymanapp/keyman/blob/d8986be12fe0fb2078b93fa4650e2a7fe06f16d1/windows/src/engine/keyman32/keyman32.cpp for what things looked like before we relied on git history. If you scan through the file you'll also see // I1234 comments which were how we tagged changes. It worked but it was increasingly noisy so it was really good to move that change history into vcs metadata.

tl;dr: If you want to keep it in, OK, and then please also update the code style wiki to say that we can optionally include the Created by <author> on yyyy-mm-dd line.

Thanks, I find that one extra line to be helpful. I've updated the wiki.

mcdurdin · 2024-08-19T03:53:46Z

mac/Keyman4MacIM/Keyman4MacIM/KMSettingsRepository.m

+- (BOOL)settingsExist
+{


The brace format throughout is not quite consistent. Our style guide would indicate

Suggested change

- (BOOL)settingsExist

{

- (BOOL)settingsExist {

https://github.com/keymanapp/keyman/wiki/Keyman-Code-Style-Guide#code-format

mcdurdin · 2024-08-19T03:54:33Z

mac/Keyman4MacIM/Keyman4MacIM/KMSettingsRepository.m

+ */
+- (BOOL)dataStoredInLibraryDirectory
+{
+  return [[NSUserDefaults standardUserDefaults] integerForKey:kDataModelVersion] == kVersionStoreDataInLibraryDirectory;


Suggested change

return [[NSUserDefaults standardUserDefaults] integerForKey:kDataModelVersion] == kVersionStoreDataInLibraryDirectory;

return [[NSUserDefaults standardUserDefaults] integerForKey:kDataModelVersion] >= kVersionStoreDataInLibraryDirectory;

This seems more forward-compatible -- unlikely that we'll move data out of that folder again but likely we may change format

mcdurdin · 2024-08-19T03:55:50Z

mac/Keyman4MacIM/Keyman4MacIM/KMSettingsRepository.m

+ * Determines whether the keyboard data needs to be moved from the old location in ~/Documents to the new location ~/Library...
+ * 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


This comment is out of date

mcdurdin · 2024-08-19T03:58:11Z

mac/Keyman4MacIM/Keyman4MacIM/KMSettingsRepository.m

+  BOOL dataInLibrary = [self dataStoredInLibraryDirectory];
+  os_log([KMLogs dataLog], "  data stored in Library: %{public}@", dataInLibrary ? @"YES" : @"NO" );
+
+  return !(keymanSettingsExist && dataInLibrary);


This seems wrong. If the keyman settings don't exist, we don't want to attempt to read from documents folder

Suggested change

return !(keymanSettingsExist && dataInLibrary);

return keymanSettingsExist && !dataInLibrary;

You're right. I thought it was just a bad method name and was rewriting it, but we should not be attempting to move the data if we have no settings. I'll fix that.

mcdurdin

LGTM, changes look good, formatting and comment nits only now!

mcdurdin · 2024-08-27T01:45:07Z

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.h

+ *
+ * KMDataRepository.h
+ * Keyman


Suggested change

*

* KMDataRepository.h

* Keyman

mcdurdin · 2024-08-27T01:46:25Z

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.h

+@property (readonly) NSURL *keymanDataDirectory;      // '~/Library/Application Support/keyman.inputmethod.Keyman'
+@property (readonly) NSURL *keymanKeyboardsDirectory;
+// keymanKeyboardsDirectory = '~/Library/Application Support/keyman.inputmethod.Keyman/Keyman-Keyboards'


nit only, but I was puzzled about what looked like commented code

Suggested change

@property (readonly) NSURL *keymanDataDirectory; // '~/Library/Application Support/keyman.inputmethod.Keyman'

@property (readonly) NSURL *keymanKeyboardsDirectory;

// keymanKeyboardsDirectory = '~/Library/Application Support/keyman.inputmethod.Keyman/Keyman-Keyboards'

// keymanDataDirectory: '~/Library/Application Support/keyman.inputmethod.Keyman'

@property (readonly) NSURL *keymanDataDirectory;

// keymanKeyboardsDirectory: '~/Library/Application Support/keyman.inputmethod.Keyman/Keyman-Keyboards'

@property (readonly) NSURL *keymanKeyboardsDirectory;

mcdurdin · 2024-08-27T01:46:36Z

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.m

+ *
+ * KMResourcesRepository.m
+ * Keyman


Suggested change

*

* KMResourcesRepository.m

* Keyman

Filename was wrong here too!

mcdurdin · 2024-08-27T01:47:27Z

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.m

+/* 
+ The name of the subdirectory within '~/Library/Application Support'.
+ 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.
+ */


Suggested change

/*

The name of the subdirectory within '~/Library/Application Support'.

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.

*/

/**

* The name of the subdirectory within '~/Library/Application Support'.

* 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.)

*/

mcdurdin · 2024-08-27T01:49:09Z

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.m

+
+- (NSURL *)keymanDataDirectory {
+  if (_keymanDataDirectory == nil) {
+    NSURL *keymanDataUrl = [self.applicationSupportSubDirectory URLByAppendingPathComponent:kKeymanSubdirectoryName  isDirectory: TRUE];


Yeah I guess so. But currently if it does go wrong we'll crash the app. And if is never going to go wrong, then why bother with the additional error logging? So it feels like we are half way.

mcdurdin · 2024-08-27T01:49:57Z

mac/Keyman4MacIM/Keyman4MacIM/KMDataRepository.m

+  }
+  return _obsoleteKeymanKeyboardsDirectory;
+}
+/**


Suggested change

/**

/**

mcdurdin · 2024-08-27T02:03:17Z

mac/Keyman4MacIM/Keyman4MacIM/KMSettingsRepository.m

+ *
+ * KMSettingsRepository.h
+ * Keyman


Suggested change

*

* KMSettingsRepository.h

* Keyman

Case in point - this says KMSettingsRepository.h but it is in a .m 😁

mcdurdin · 2024-08-27T02:05:05Z

mac/Keyman4MacIM/Keyman4MacIM/KMSettingsRepository.m

+ * For versions before version 1, the keyboards were stored under the ~/Documents directory.
+ */
+- (BOOL)dataModelWithKeyboardsInLibrary {
+  // NSUserDefaults returns zero if the key does not exist


Suggested change

// NSUserDefaults returns zero if the key does not exist

// [NSUserDefaults integerForKey] returns zero if the key does not exist

Created issue #12294 to document the need for better error handling in case the data directory cannot be located or is unreadable. The current code does not gracefully handle a nil returned from the OS when trying to access ~/Library/Application Support

mac/Keyman4MacIM/Keyman4MacIM/KMSettingsRepository.m

keyman-server · 2024-08-30T18:02:39Z

Changes in this pull request will be available for download in Keyman version 18.0.102-alpha

sgschantz added 8 commits July 29, 2024 16:01

change(mac): add class to encapsulate settings

5488e32

change(mac): add KMDataRepository class to handle data (keyboards) mi…

37dcacc

…gration

change(mac): customize menus after all views are created

bff17dc

rather than reading from disk and adding keyboards include awakeFromNib, do so in applicationDidFinishLaunching

change(mac): moved directory locations into KMDataRepository

ec2e48c

change(mac): implemented data migration and settings path updates

4fb5d74

change(mac): successful migration but not loading from config

c2e2cf5

change(mac): migrates options and changes keyboards successfully

b435090

change(mac): uses Library directory instead of Documents

8b40873

Write new key KMDataModelVersion in UserDefaults with integer value that determines how data is expected to be stored. Migrates existing Keyman data to new location if it was written with an older (or no) version. Fixes: #2542

sgschantz added mac/ change labels Aug 6, 2024

sgschantz added this to the A18S8 milestone Aug 6, 2024

sgschantz self-assigned this Aug 6, 2024

keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Aug 7, 2024

Merge branch 'master' into change/mac/2542-store-keyboards-elsewhere

05918b5

sgschantz mentioned this pull request Aug 9, 2024

change(mac): store partial path in UserDefaults #12144

Merged

keymanapp-test-bot bot removed has-user-test user-test-required User tests have not been completed labels Aug 9, 2024

sgschantz marked this pull request as ready for review August 9, 2024 08:03

sgschantz requested a review from SabineSIL as a code owner August 9, 2024 08:03

darcywong00 modified the milestones: A18S8, A18S9 Aug 17, 2024

mcdurdin requested changes Aug 19, 2024

View reviewed changes

change(mac): responded to review comments

24f4a61

mcdurdin approved these changes Aug 27, 2024

View reviewed changes

sgschantz and others added 2 commits August 27, 2024 13:06

change(mac): some minor changes after review comments

ea6e295

Merge branch 'master' into change/mac/2542-store-keyboards-elsewhere

8dd258d

sgschantz merged commit a283fd1 into master Aug 30, 2024
5 checks passed

sgschantz deleted the change/mac/2542-store-keyboards-elsewhere branch August 30, 2024 05:35

		@property (readonly) NSURL *documentsSubDirectory; // '~/Documents'
		@property (readonly) NSURL *obsoleteKeymanKeyboardsDirectory; // '~/Library/Documents/Keyman-Keyboards'

		(Using the subsystem id, "com.keyman.app", is a poor choice because the API
		createDirectoryAtPath sees the .app extension and creates an application file.

	return [[NSUserDefaults standardUserDefaults] integerForKey:kDataModelVersion] == kVersionStoreDataInLibraryDirectory;
	return [[NSUserDefaults standardUserDefaults] integerForKey:kDataModelVersion] >= kVersionStoreDataInLibraryDirectory;

	return !(keymanSettingsExist && dataInLibrary);
	return keymanSettingsExist && !dataInLibrary;

	// NSUserDefaults returns zero if the key does not exist
	// [NSUserDefaults integerForKey] returns zero if the key does not exist

change(mac): store data in Library directory instead of Documents #12106

change(mac): store data in Library directory instead of Documents #12106

Conversation

sgschantz commented Aug 6, 2024 • edited Loading

keymanapp-test-bot bot commented Aug 6, 2024 • edited Loading

User Test Results

Test Artifacts

mcdurdin commented Aug 11, 2024

sgschantz commented Aug 12, 2024

mcdurdin 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

sgschantz Aug 21, 2024 • edited Loading

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

sgschantz Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

mcdurdin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcdurdin Aug 27, 2024 • edited Loading

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

sgschantz Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

keyman-server commented Aug 30, 2024

sgschantz commented Aug 6, 2024 •

edited

Loading

keymanapp-test-bot bot commented Aug 6, 2024 •

edited

Loading

sgschantz Aug 21, 2024 •

edited

Loading

sgschantz Aug 21, 2024 •

edited

Loading

mcdurdin Aug 27, 2024 •

edited

Loading

sgschantz Aug 27, 2024 •

edited

Loading