change(mac): adopt unified logging APIs #11515

sgschantz · 2024-05-23T09:04:27Z

Create os_log_t objects to identify different parts of the application for logging purposes
Define subystem and categories to use for filtering log statements
Call unified logging APIs os_log, os_log_error, os_log_info and os_log_debug to record log statements.
Remove all references to logDebugMessage and the NSLog macro, as they do not use unified logging (and do not specify subsystem and category)

Fixes #10997

@keymanapp-test-bot skip

delete commented out code and unimplemented method

KMLogs creates singleton os_log objects with the subsystem and categories shared throughout the application

first draft, need to remove some debug statements and replace others that would be more useful

remove logDebugMessage API which calls NSLog and call os_log instead

remove all references to NSLog adjust log level for some calls to os_log Fixes #10997

keymanapp-test-bot · 2024-05-23T09:04:31Z

User Test Results

Test specification and instructions

User tests are not required

mcdurdin

Minor nits only, LGTM.

I think we should use com.keyman and not org.sil.keyman as our namespace -- we have keyman.com as a domain but not keyman.sil.org (if we went that way it probably should be org.sil.software.keyman).

I also added .app and .engine so that com.keyman was grouping namespace, with submodules beneath that.

mcdurdin · 2024-05-26T23:56:44Z

mac/Keyman4MacIM/Keyman4MacIM/main.m


 const NSString *kConnectionName = @"Keyman_Input_Connection";
 IMKServer *server;

 int main(int argc, const char * argv[]) {
-  NSString *identifier;
-  os_log_t configLog = os_log_create("org.sil.keyman", "startup");
+    NSString *identifier;


Suggested change

NSString *identifier;

NSString *identifier;

mcdurdin · 2024-05-26T23:58:17Z

mac/KeymanEngine4Mac/KeymanEngine4Mac/CoreWrapper/CoreKeyOutput.m

-  NSData* data = [self.textToInsert dataUsingEncoding:NSUTF16LittleEndianStringEncoding];
-
-  return [[NSString alloc] initWithFormat: @"codePointsToDeleteBeforeInsert: %li, textToInsert: '%@', optionsToPersist: %@, alert: %d, emitKeystroke: %d, capsLockState: %d ", self.codePointsToDeleteBeforeInsert, data, self.optionsToPersist, self.alert, self.emitKeystroke, self.capsLockState];
+  NSData* deleteData = [self.textToDelete dataUsingEncoding:NSUTF16LittleEndianStringEncoding];


Technically this is a change in behaviour -- just checking that self.textToDelete is never null?

The description method is called to log the CoreKeyOutput state from CoreWrapper processMacVirtualKey. Both textToInsert and textToDelete can be null, but if they are, then dataUsingEncoding returns null, and the log statement will say textToDelete: '(null)' [data: '(null)']

Because this is being logged with a debug statement, this description method will only be executed when the user is streaming debug logs.

mcdurdin · 2024-05-26T23:59:02Z

mac/KeymanEngine4Mac/KeymanEngine4Mac/KME/KMELogs.m

+
+@implementation KMELogs
+
+char *const keymanEngineSubsystem = "org.sil.keymanengine";


Suggested change

char *const keymanEngineSubsystem = "org.sil.keymanengine";

char *const keymanEngineSubsystem = "com.keyman.engine";

mcdurdin · 2024-05-26T23:59:28Z

mac/Keyman4MacIM/Keyman4MacIM/KMLogs.m

+
+@implementation KMLogs
+
+char *const keymanSubsystem = "org.sil.keyman";


Suggested change

char *const keymanSubsystem = "org.sil.keyman";

char *const keymanSubsystem = "com.keyman.app";

mcdurdin · 2024-05-27T00:00:16Z

mac/KeymanEngine4Mac/KeymanEngine4Mac/KME/KVKFile.m

@@ -21,7 +22,7 @@ - (id)initWithFilePath:(NSString *)path {

    NSFileHandle *file = [NSFileHandle fileHandleForReadingAtPath:path];
    if (file == nil) {
-      //NSLog(@"Failed to open kvk file");
+      os_log_error([KMELogs configLog], "Failed to open kmx file");


Suggested change

os_log_error([KMELogs configLog], "Failed to open kmx file");

os_log_error([KMELogs configLog], "Failed to open kvk file");

address review comments

keyman-server · 2024-05-27T18:05:51Z

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

keyman-server · 2024-05-29T05:32:05Z

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

keyman-server · 2024-05-30T00:57:22Z

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

keyman-server · 2024-05-30T21:41:25Z

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

keyman-server · 2024-05-31T19:21:34Z

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

keyman-server · 2024-05-31T23:07:01Z

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

keyman-server · 2024-05-31T23:25:11Z

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

sgschantz added 5 commits May 20, 2024 08:42

fix(mac): change to unified logging

8fa8275

delete commented out code and unimplemented method

fix(mac): added KMLogs class to manage shared log settings

cc06183

KMLogs creates singleton os_log objects with the subsystem and categories shared throughout the application

fix(mac): added unified logging to Keyman Engine for mac

e4f7f1d

first draft, need to remove some debug statements and replace others that would be more useful

fix(mac): use unified logging APIS

aff9a0e

remove logDebugMessage API which calls NSLog and call os_log instead

fix(mac): adopt unified logging APIs

ab98086

remove all references to NSLog adjust log level for some calls to os_log Fixes #10997

sgschantz added the mac/ label May 23, 2024

sgschantz added this to the A18S2 milestone May 23, 2024

sgschantz self-assigned this May 23, 2024

sgschantz marked this pull request as ready for review May 24, 2024 00:37

sgschantz requested a review from SabineSIL as a code owner May 24, 2024 00:37

sgschantz changed the title ~~Fix(mac): adopt unified logging APIs~~ Chore(mac): adopt unified logging APIs May 24, 2024

mcdurdin changed the title ~~Chore(mac): adopt unified logging APIs~~ chore(mac): adopt unified logging APIs May 24, 2024

mcdurdin changed the title ~~chore(mac): adopt unified logging APIs~~ change(mac): adopt unified logging APIs May 24, 2024

mcdurdin added the change label May 24, 2024

This was referenced May 24, 2024

change(mac): remove 'Use verbose Console logging' option #11525

Closed

docs(mac): describe how to capture logs with move to Unified Logging #11526

Open

mcdurdin modified the milestones: A18S2, A18S3 May 24, 2024

mcdurdin approved these changes May 27, 2024

View reviewed changes

fix(mac): adopt unified logging

1d5ef91

address review comments

sgschantz merged commit b068f94 into master May 27, 2024
3 checks passed

sgschantz deleted the fix/mac/10997-adopt-unified-logging branch May 27, 2024 02:11

sgschantz mentioned this pull request Sep 17, 2024

change(mac): remove verbose logging option #12431

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change(mac): adopt unified logging APIs #11515

change(mac): adopt unified logging APIs #11515

sgschantz commented May 23, 2024 •

edited by mcdurdin

Loading

keymanapp-test-bot bot commented May 23, 2024 •

edited

Loading

mcdurdin left a comment

mcdurdin May 26, 2024

mcdurdin May 26, 2024

sgschantz May 27, 2024

mcdurdin May 26, 2024

mcdurdin May 26, 2024

mcdurdin May 27, 2024

keyman-server commented May 27, 2024

keyman-server commented May 29, 2024

keyman-server commented May 30, 2024

keyman-server commented May 30, 2024

keyman-server commented May 31, 2024

keyman-server commented May 31, 2024

keyman-server commented May 31, 2024


		@implementation KMELogs

		char *const keymanEngineSubsystem = "org.sil.keymanengine";

	char *const keymanEngineSubsystem = "org.sil.keymanengine";
	char *const keymanEngineSubsystem = "com.keyman.engine";


		@implementation KMLogs

		char *const keymanSubsystem = "org.sil.keyman";

	char *const keymanSubsystem = "org.sil.keyman";
	char *const keymanSubsystem = "com.keyman.app";

	os_log_error([KMELogs configLog], "Failed to open kmx file");
	os_log_error([KMELogs configLog], "Failed to open kvk file");

change(mac): adopt unified logging APIs #11515

change(mac): adopt unified logging APIs #11515

Conversation

sgschantz commented May 23, 2024 • edited by mcdurdin Loading

keymanapp-test-bot bot commented May 23, 2024 • edited Loading

User Test Results

mcdurdin left a comment

Choose a reason for hiding this comment

mcdurdin May 26, 2024

Choose a reason for hiding this comment

mcdurdin May 26, 2024

Choose a reason for hiding this comment

sgschantz May 27, 2024

Choose a reason for hiding this comment

mcdurdin May 26, 2024

Choose a reason for hiding this comment

mcdurdin May 26, 2024

Choose a reason for hiding this comment

mcdurdin May 27, 2024

Choose a reason for hiding this comment

keyman-server commented May 27, 2024

keyman-server commented May 29, 2024

keyman-server commented May 30, 2024

keyman-server commented May 30, 2024

keyman-server commented May 31, 2024

keyman-server commented May 31, 2024

keyman-server commented May 31, 2024

sgschantz commented May 23, 2024 •

edited by mcdurdin

Loading

keymanapp-test-bot bot commented May 23, 2024 •

edited

Loading