Platform native notifications PR_3 #1202

alameenshah · 2019-05-24T18:57:12Z

Enable display of Automount notifications

This is the third incremental PR to enable platform native notifications on the Mac. In this PR, GVFS.Service sends auto-mount notifications to VFS For Git.app for display to user. Service displays two separate notifications - 1. a generic message when GVFS.Service begins on auto-mounting registered repositories and 2. for each registered repository with repo specific auto-mount success/failure messaging.

Details

New VFSForGitNotification class. It supports 3 different notifications - AutomountStart, MountSuccess, MountFailure. It will not display any other notification.
VFSForGitNotification defines the title & message body for each of the supported notification. It is not possible for GVFS.Service or another client to display random text.
MessageListener class creates a socket and waits for notification requests from client.
MessageParser class to parse the incoming JSON formatted message text.
New UT for VFSForGitNotification & MessageParser classes.
GVFS.Service does not wait for a reply from VFS For Git.app for the notification requests that it sends.
VFS For Git.app get automatically launched after installation.
VFS For Git.app has the same app version as gvfs version.

Message processing sequence

AppDelegate class creates and installs an instance of MessageListener. MessageListener creates a socket and waits for incoming messages. The following sequence diagram shows the flow of a message that arrives in MessageListener.

Testing notes

Installer automatically launches VFS For Git.app after successful install. If there were repositories that were registered for auto-mount before upgrade install, then auto-mount notifications for each of those repos would get displayed in the native Notification Center as GVFS.Service auto mounts them.
Tried sending a few hundred notifications. NotificationCenter collated notifications beyond 10 and created the following single notification (separate ones were still available in a long scroll view inside the NotificationCenter) -

Tried sending huge (500MB) garbage data from client - memory usage of VFS For Git.app spiked, JSON Parsing failed, the app logged parse error and its memory footprint went down to normal.
Tried multiple clients sending messages simultaneously. No issues observed.

Reference

For a better understanding of the bigger picture refer this PR.

GVFS/GVFS.Notifications/VFSForGit.Mac/StatusMenuItem/IPC/MessageListener.h

GVFS/GVFS.Notifications/VFSForGit.Mac/StatusMenuItem/IPC/MessageParser.h

GVFS/GVFS.Notifications/VFSForGit.Mac/StatusMenuItem/IPC/MessageParser.m

alameenshah · 2019-05-24T23:24:13Z

/azp run GitHub VFSForGit Mac Functional Tests

azure-pipelines · 2019-05-24T23:24:19Z

No pipelines are associated with this pull request.

GVFS/GVFS.Notifications/VFSForGit.Mac/StatusMenuItem/IPC/VFSForGitNotification.h

GVFS/GVFS.Notifications/VFSForGit.Mac/StatusMenuItem/IPC/VFSForGitNotification.m

alameenshah · 2019-05-28T14:07:16Z

/azp run GitHub VFSForGit Mac Functional Tests

azure-pipelines · 2019-05-28T14:07:33Z

No pipelines are associated with this pull request.

GVFS/GVFS.Notifications/VFSForGit.Mac/StatusMenuItem/IPC/VFSForGitNotification.h

GVFS/GVFS.Notifications/VFSForGit.Mac/StatusMenuItem/IPC/VFSForGitNotification.m

GVFS/GVFS.Notifications/VFSForGit.Mac/StatusMenuItem/IPC/VFSMessageListener.m

hi-kumar · 2019-06-01T01:01:55Z

GVFS/GVFS.Notifications/VFSForGit.Mac/StatusMenuItem/IPC/VFSMessageParser.m

-                      NSStringFromSelector(_cmd)];
+            NSString *info = [NSString stringWithFormat:@"%@: ERROR: error reading data.",
+                              NSStringFromSelector(_cmd)];
+            *error = [NSError errorWithDomain:@"VFS For Git"


Move the domain string out to a single extern constant (declaration in some common header, single initialization in a .m file). Also see naming convention for custom error domains: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ErrorHandlingCocoa/ErrorObjectsDomains/ErrorObjectsDomains.html
You can create your own error domains and error codes for use in your own frameworks, or even in your own applications. It is recommended that the string constant for the domain be of the form com.company.framework_or_app.ErrorDomain.

Moved Notification Agent specific errors to a separate file - VFSNotificationErrors.h. VFSNotificationErrors.m has only a single line with definition of the Domain - VFSForGitNotificationErrorDomain.

hi-kumar · 2019-06-01T01:04:44Z

GVFS/GVFS.Notifications/VFSForGit.Mac/StatusMenuItem/IPC/VFSForGitNotification.m

+        if (error != nil)
+        {
+            *error = [NSError errorWithDomain:@"VFS For Git"
+                                         code:-1


Suggestion: Ideally each error code should be unique to identify the failure. Helps when you want the caller to handle particular error conditions (by comparing error code numbers/enums).

Added Notification specific error codes in VFSNotificationErrors.h

hi-kumar · 2019-06-01T01:08:43Z

GVFS/GVFS.Notifications/VFSForGit.Mac/StatusMenuItem/IPC/VFSMessageListener.m

+    return YES;
+}
+
+- (void)stopListening


Add a -dealloc method to ensure the cleanup happens correctly on termination or whenever the object is released. Otherwise you are relying on the caller to call this appropriately. It may be fine right now because stopListening is called on application terminate, but could change with code refactoring etc.

yep... added.

Couple notes

[super dealloc] is not added because of ARC.

Apple docs suggest it is not required to un-register notification observers from inside a dealloc method.

alameenshah · 2019-06-12T21:44:06Z

GVFS/GVFS.Installer.Mac/CreateMacInstaller.sh

@@ -118,6 +118,9 @@ function CopyBinariesToInstall()
    copyNotificationApp="cp -Rf \"${VFS_OUTPUTDIR}/GVFS.Notifications/VFSForGit.Mac/Build/Products/$CONFIGURATION/VFS For Git.app\" \"${STAGINGDIR}/${LIBRARYAPPSUPPORTDESTINATION}/.\""
    eval $copyNotificationApp || exit 1

+    copyNotificationPlist="cp -Rf \"${SOURCEDIRECTORY}/../GVFS.Notifications/VFSForGit.Mac/org.vfsforgit.usernotification.plist\" \"${STAGINGDIR}/${AGENTPLISTDESTINATION}/.\""


installs VFS For Git.app as launch agent

alameenshah · 2019-06-12T21:45:06Z

GVFS/GVFS.Installer.Mac/scripts/preinstall


 KEXTBUNDLEID="org.vfsforgit.PrjFSKext"
 isKextLoadedCmd="/usr/sbin/kextstat -l -b $KEXTBUNDLEID | wc -l"
 isKextLoaded=$(eval $isKextLoadedCmd)
 if [ "$isKextLoaded" -gt 0 ]; then
-	unloadCmd="/sbin/kextunload -b $KEXTBUNDLEID"
+    unloadCmd="/sbin/kextunload -b $KEXTBUNDLEID"


replaced a tab with spaces

alameenshah · 2019-06-12T21:45:17Z

GVFS/GVFS.Installer.Mac/scripts/preinstall

@@ -32,7 +38,7 @@ LEGACYKEXTBUNDLEID="io.gvfs.PrjFSKext"
 isKextLoadedCmd="/usr/sbin/kextstat -l -b $LEGACYKEXTBUNDLEID | wc -l"
 isKextLoaded=$(eval $isKextLoadedCmd)
 if [ "$isKextLoaded" -gt 0 ]; then
-	unloadCmd="/sbin/kextunload -b $LEGACYKEXTBUNDLEID"
+    unloadCmd="/sbin/kextunload -b $LEGACYKEXTBUNDLEID"


replaced a tab with spaces

alameenshah · 2019-06-13T20:07:34Z

/azp run GitHub VFSForGit Mac Functional Tests

azure-pipelines · 2019-06-13T20:07:40Z

No pipelines are associated with this pull request.

jrbriggs · 2019-06-14T18:32:35Z

GVFS/GVFS.Service/RepoRegistry.cs

-                this.SendNotification(sessionId, "GVFS AutoMount", "Attempting to mount {0} GVFS repo(s)", activeRepos.Count);
+                this.SendNotification(
+                    sessionId,
+                    NamedPipeMessages.Notification.Request.Identifier.AutomountStart,


How should we thinking about interacting with users on macOS via notifications? Do they expect these to be fired only for special situations, or do they expect chatty notifications? I worry that notifying that we're about to mount X repos and then firing X success messages is too much. We could

Only fire failure to mount

Continue firing start, but only fire one notification summarizing what mounted successfully.

Thoughts?

The more I think about it, the more I like option 2. It's also a nice clue that you can start working. Thoughts?

yes, that makes sense. Let me take a look at the options to minimize auto-mount notifications count...

As discussed - notifying only on auto-mount failures.

jrbriggs

Approved with suggestions.

nickgra

Thanks Ameen, this looks good! Sorry for the delay on my part.

nickgra · 2019-06-14T22:02:45Z

GVFS/GVFS.Notifications/VFSForGit.Mac/StatusMenuItem/UI/VFSAppDelegate.m

+            [self displayNotification:messageInfo];
+        }];
+
+    [self.messageListener startListening];

    VFSProductInfoFetcher *productInfoFetcher =


nit: what's going on with the whitespace here? Can we try getting Xcode to make the whitespace conform to the usual standard?

nickgra · 2019-06-14T22:05:29Z

Scripts/Mac/BuildGVFSForMac.sh

@@ -59,6 +59,10 @@ xcodebuild -configuration $CONFIGURATION -workspace $NATIVEDIR/GVFS.Native.Mac.x
 USERNOTIFICATIONDIR=$VFS_SRCDIR/GVFS/GVFS.Notifications/VFSForGit.Mac
 USERNOTIFICATIONPROJECT="$USERNOTIFICATIONDIR/VFSForGit.xcodeproj"
 USERNOTIFICATIONSCHEME="VFS For Git"
+updateAppVersionCmd="(cd \"$USERNOTIFICATIONDIR\" && /usr/bin/xcrun agvtool new-marketing-version \"$VERSION\")"
+echo $updateAppVersionCmd


For debugging?

ya. just to make sure the project directory and version are specified correctly.

nickgra · 2019-06-14T22:06:22Z

Scripts/Mac/BuildGVFSForMac.sh

@@ -59,6 +59,10 @@ xcodebuild -configuration $CONFIGURATION -workspace $NATIVEDIR/GVFS.Native.Mac.x
 USERNOTIFICATIONDIR=$VFS_SRCDIR/GVFS/GVFS.Notifications/VFSForGit.Mac
 USERNOTIFICATIONPROJECT="$USERNOTIFICATIONDIR/VFSForGit.xcodeproj"
 USERNOTIFICATIONSCHEME="VFS For Git"
+updateAppVersionCmd="(cd \"$USERNOTIFICATIONDIR\" && /usr/bin/xcrun agvtool new-marketing-version \"$VERSION\")"


How does a new-marketing-version interact with the Info.plist? Should we move to using this to set the version of everything else that is native (the kext, log daemons, etc)?

new-marketing-version updates CFBundleShortVersionString in the Info.plist file.

Should we move to using this to set the version of everything else that is native (the kext, log daemons, etc)?

I can take a look at how this is done on other native code and we can discuss offline.

This is the third incremental PR to enable platform native notifications on the Mac. In this PR, GVFS.Service sends auto-mount notifications to VFS For Git.app for display to user. Service displays notification for auto-mount failures only - when it fails to auto-mount a repository that has been registered for auto-mount. Details - New VFSForGitNotification class. It supports 3 different notifications - AutomountStart, MountSuccess, MountFailure. It will not display any other notification. - VFSForGitNotification defines the title & message body for each of the supported notification. It is not possible for GVFS.Service or another client to display random text. - MessageListener class creates a socket and waits for notification requests from client. - MessageParser class to parse the incoming JSON formatted message text. - New UT for VFSForGitNotification & MessageParser classes. - GVFS.Service does not wait for a reply from VFS For Git.app for the notification requests that it sends. - VFS For Git.app get automatically launched after installation. - VFS For Git.app has the same app version as gvfs version. - Updated MacServiceTests to not use INotificationHandler to verify Automount start. Now Service posts notification only on auto-mount failures. More Info - #1202

alameenshah added the affects: polish Good UX is hard label May 24, 2019

alameenshah added this to the M153 milestone May 24, 2019

alameenshah self-assigned this May 24, 2019

alameenshah requested review from wilbaker, jrbriggs, nickgra and hi-kumar May 24, 2019 18:57