use a file based startup on darwin #38

noamokman · 2016-08-28T02:35:37Z

Hi there!
Should close #25 #28 #31

Thanks!

noamokman · 2016-08-28T02:36:31Z

@adam-lynch would love your opinion 😄

adam-lynch · 2016-08-28T22:40:19Z

src/AutoLaunchMac.coffee

+            file = @getFile(opts)
+            array = [opts.appPath]
+            if(opts.isHiddenOnLaunch) then array.push('--hidden')
+            command = array.map(a => `    <string>${a}</string>`)


Wait... backticks in CoffeeScript? Never saw this before. I thought you had to use double quotes for interpolation.

adam-lynch · 2016-08-28T22:43:43Z

Thanks. Educate me... what are the pros & cons of this versus the existing way?

If we did change it, what about app's that were using the old version? How do they transition to this? Do we need to keep the old logic in this module and transition it to file-based?

noamokman · 2016-08-28T23:09:22Z

Thanks for the remarks!
Fixed the problematic bits.
Also removed the applescript dep.

I think this should be a breaking change,
so we wont have to deal with mixed implementation.

As for pros and cons,
It seems that the current way has problems with a terminal window popping up.
I believe this will work better

P.S,
This unifies the --hidden argument being sent on all platforms,
so it's already a good enough reason to bump major.
Maybe write more clearly that the hidden option sends this --hidden arg on startup,
and that is up for the developer to use relevant logic based on the args.

dcposch · 2016-08-31T03:47:18Z

It seems that the current way has problems with a terminal window popping up.

Are there projects that support Mac and use auto-launch in production?

I'm surprised this hasn't come up before--it seems like opening an extra terminal window after every boot would result in lots of user feedback & bug reports. (Esp since closing the terminal window then kills the app.)

adam-lynch · 2016-08-31T16:42:19Z

@dcposch I can say yes definitely with NW.js but can't confirm Electron but I assume so?

@noamokman I'll take a proper look at this soon. In the meantime, maybe others could confirm this is OK for them / solves their problems? I'll ask in the issues you've linked

mzdr · 2016-09-01T20:14:21Z

Hey @noamokman, your patch gives me the following error:

Promise {
  <rejected> ReferenceError: name is not defined
    at /Users/sp/Projects/timestamp/node_modules/auto-launch/dist/AutoLaunchMac.js:29:236
    at Object.enable (/Users/sp/Projects/timestamp/node_modules/auto-launch/dist/AutoLaunchMac.js:18:12)
    at AutoLaunch.module.exports.AutoLaunch.enable (/Users/sp/Projects/timestamp/node_modules/auto-launch/dist/index.js:72:21)
    at new App (/Users/sp/Projects/timestamp/app/app.js:45:39)
    at App.Electron.app.on (/Users/sp/Projects/timestamp/app/index.js:7:32)
    at emitOne (events.js:101:20)
    at App.emit (events.js:188:7) }

Have a look at https://github.com/Teamwork/node-auto-launch/pull/38/files#diff-3cb4ac24dd2f84b466e7af980f600036R25. Or am I wrong?

mzdr · 2016-09-01T20:16:22Z

src/AutoLaunchMac.coffee

+                    <plist version="1.0">
+                    <dict>
+                      <key>Label</key>
+                      <string>#{name}</string>


Getting ReferenceError: name is not defined here…

noamokman · 2016-09-02T05:08:10Z

@mzdr oops, please try again,
Thanks!

mzdr · 2016-09-02T14:08:44Z

Out of the box it's not working on my machine (Electron v1.3.4 and OSX 10.11.6). There is no .plist file being written and I don't get any errors. I am debugging right now…

mzdr · 2016-09-02T14:15:32Z

src/AutoLaunchMac.coffee


 module.exports =
+    getFile: (opts) ->
+        file = dir+opts.appName+'.plist'


@noamokman I think there is a / missing here because I keep getting file paths like this: /Users/sp/Library/LaunchAgentsMinecraft.plist

great catch! thanks for the help 😄

noamokman · 2016-09-02T14:26:51Z

@mzdr added the missing /

mzdr · 2016-09-02T16:10:56Z

Okay, now it seems to be working. After logging out and back in again my app was automatically started. 👍

Though I personally have some concerns with the LaunchAgent approach. For example if you delete an app which was set to auto launch, the .plist file will end up as a dead file on the users hard disk. I know it’s picky, but nevertheless it’s something we should care about.

Furthermore the average (or more sophisticated) user will probably check his login items under System Preferences → Users & Groups → Login Items and delete old/non-existent apps from there. But apps registered as a LaunchAgent won’t show up there.

@adam-lynch I think this is something we should consider.

noamokman · 2016-09-04T17:35:44Z

Though I personally have some concerns with the LaunchAgent approach. For example if you delete an app which was set to auto launch, the .plist file will end up as a dead file on the users hard disk. I know it’s picky, but nevertheless it’s something we should care about.

@mzdr
This is also true for the linux implementation.

dcposch · 2016-09-06T23:04:29Z

@mzdr @adam-lynch Thoughts?

For WebTorrent Desktop, we can't use auto-launch as long as it's creating a terminal window on Mac.

mzdr · 2016-09-08T11:50:06Z

@dcposch Did you checkout the workaround posted in #28? As for now I'm using auto-launch with the workaround as well…

dcposch · 2016-09-10T01:47:03Z

@mzdr yeah, I tried the workaround for Electron listed here:

#28 (comment)

Didn't work

adam-lynch · 2016-09-15T16:35:21Z

OK I'm going to test this tonight/tomorrow. I'll give it a quick review now.

adam-lynch

Thanks again 👍

adam-lynch · 2016-09-15T16:35:41Z

src/AutoLaunchMac.coffee

-            applescript.execString command, (err) ->
+        new Promise (resolve, reject) =>
+            file = @getFile(opts)
+            array = [opts.appPath]


Please don't call this array

adam-lynch · 2016-09-15T16:36:24Z

src/AutoLaunchMac.coffee

+        new Promise (resolve, reject) =>
+            file = @getFile(opts)
+            array = [opts.appPath]
+            if(opts.isHiddenOnLaunch) then array.push('--hidden')


You don't need the then; you can do this:

array.push('--hidden') if opts.isHiddenOnLaunch

adam-lynch · 2016-09-15T16:36:45Z

src/AutoLaunchMac.coffee

+                    </dict>
+                    </plist>"""
+
+            mkdirp.sync(dir)


Shouldn't we have some error checking here?

adam-lynch · 2016-09-15T16:38:40Z

src/AutoLaunchMac.coffee

                return reject(err) if err?
                resolve()

    disable: (opts) ->
-        new Promise (resolve, reject) ->
-            command = tellTo + "delete login item \"#{opts.appName}\""
+        new Promise (resolve, reject) =>


Since you're touching these lines (where new Promise is returned), do you mind adding a return at the start? I'm not a fan of using implicit returns in multi-line functions

adam-lynch · 2016-09-15T16:39:18Z

src/AutoLaunchMac.coffee

                return reject(err) if err?
-                resolve()
+                fs.unlink file, (err2) ->


This is silly but name it something other than err2. Even if it's error or something.

adam-lynch · 2016-09-15T16:39:35Z

src/AutoLaunchMac.coffee

-                isPresent = loginItems?.indexOf(opts.appName)
-                resolve(isPresent? and isPresent isnt -1)
+            fs.stat file, (err, stat) ->
+    # TODO: Error handling


Need to reject here.

adam-lynch

OK I've tested it and it's fine except if you call disable when it's not enabled, it'll reject with an error about a missing file. It should resolve in this case.

adam-lynch · 2016-09-16T16:56:17Z

One other thing I noticed is that the app doesn't appear in the user's Login Items (in system preferences).

I'm confirming with some proper Mac users to see if that would annoy users.

I'm guessing it's fine. Once this is merged, I'll add a note about it to the README along with one about the leftover file if the user deletes the app.

adam-lynch · 2016-09-16T17:45:50Z

OK I got great information from @chillok. We think it's not bad that it doesn't appear in Login Items (even though it's not nice) but he let me know that this definitely isn't Mac App Store friendly. An app using this would be rejected because it reaches outside of the app sandbox. See https://developer.apple.com/library/content/documentation/Security/Conceptual/AppSandboxDesignGuide/AboutAppSandbox/AboutAppSandbox.html for more info.

At first, I thought this was a deal-breaker for this PR but we think the existing method (executing an applescript command) isn't MAS friendly either so we'll go with this PR.

I guess that's just something else I'll have to add to our README.

I know someone will end up coming to this comment in the future after googling so I'll say it is possible to have an auto-launching app in the Mac App Store. See http://martiancraft.com/blog/2015/01/login-items/. As long as it doesn't auto-launch by default, the user can toggle it, etc. You need to have a separate app/executable for this so since that means native code and that extra app/executable would have to be signed just like your main app would, this module would never be able to handle that. I'll create a feature request over on electron-builder (as it already handles the signing of Electron apps for you) but I'm not 100% sure it's possible because a user preference is needed. NW.js and Electron might have to add this.

adam-lynch · 2016-09-16T17:56:33Z

Here it is: electron-userland/electron-builder#756

adam-lynch · 2016-09-16T23:29:10Z

OK after some more overthinking... I'm going to make this an option. So I've merged this as is and I'll sort it out.

adam-lynch · 2016-09-17T05:06:52Z

OK I've overhauled this module a bit. There's now a mac.useLaunchAgent option. See https://github.com/Teamwork/node-auto-launch/releases/tag/5.0.0. Thanks for the PR @noamokman 👍

noamokman · 2016-09-17T12:40:02Z

@adam-lynch thanks!

use a file based startup on darwin

a5c9566

noamokman mentioned this pull request Aug 28, 2016

Startup option in preferences (WIP) webtorrent/webtorrent-desktop#837

Closed

adam-lynch reviewed Aug 28, 2016
View reviewed changes

fixed coffescript and removed applescript dep

14f9df2

This was referenced Aug 31, 2016

Can't get correct isEnabled() result in menu click function (only on mac) #25

Closed

Auto Launch opens terminal on OSX #28

Closed

mzdr reviewed Sep 1, 2016
View reviewed changes

incorrect variable

b479d4c

mzdr reviewed Sep 2, 2016
View reviewed changes

fixed missing /

4a2b48e

adam-lynch suggested changes Sep 15, 2016

View reviewed changes

adam-lynch suggested changes Sep 16, 2016

View reviewed changes

adam-lynch mentioned this pull request Sep 16, 2016

Feature request: allow for Mac App Store compatible auto-launching electron-userland/electron-builder#756

Closed

adam-lynch merged commit 0187f7f into Teamwork:master Sep 16, 2016

sebn mentioned this pull request Apr 3, 2017

Auto launch on mac cozy-labs/cozy-desktop#601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use a file based startup on darwin #38

use a file based startup on darwin #38

noamokman commented Aug 28, 2016

noamokman commented Aug 28, 2016

adam-lynch Aug 28, 2016

adam-lynch commented Aug 28, 2016

noamokman commented Aug 28, 2016

dcposch commented Aug 31, 2016 •

edited

Loading

adam-lynch commented Aug 31, 2016

mzdr commented Sep 1, 2016

mzdr Sep 1, 2016

noamokman commented Sep 2, 2016

mzdr commented Sep 2, 2016

mzdr Sep 2, 2016

noamokman Sep 2, 2016

noamokman commented Sep 2, 2016

mzdr commented Sep 2, 2016

noamokman commented Sep 4, 2016

dcposch commented Sep 6, 2016

mzdr commented Sep 8, 2016

dcposch commented Sep 10, 2016

adam-lynch commented Sep 15, 2016

adam-lynch left a comment

adam-lynch Sep 15, 2016

adam-lynch Sep 15, 2016

adam-lynch Sep 15, 2016

adam-lynch Sep 15, 2016

adam-lynch Sep 15, 2016

adam-lynch Sep 15, 2016

adam-lynch left a comment

adam-lynch commented Sep 16, 2016

adam-lynch commented Sep 16, 2016

adam-lynch commented Sep 16, 2016

adam-lynch commented Sep 16, 2016

adam-lynch commented Sep 17, 2016

noamokman commented Sep 17, 2016

use a file based startup on darwin #38

use a file based startup on darwin #38

Conversation

noamokman commented Aug 28, 2016

noamokman commented Aug 28, 2016

Choose a reason for hiding this comment

adam-lynch commented Aug 28, 2016

noamokman commented Aug 28, 2016

dcposch commented Aug 31, 2016 • edited Loading

adam-lynch commented Aug 31, 2016

mzdr commented Sep 1, 2016

Choose a reason for hiding this comment

noamokman commented Sep 2, 2016

mzdr commented Sep 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamokman commented Sep 2, 2016

mzdr commented Sep 2, 2016

noamokman commented Sep 4, 2016

dcposch commented Sep 6, 2016

mzdr commented Sep 8, 2016

dcposch commented Sep 10, 2016

adam-lynch commented Sep 15, 2016

adam-lynch 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

adam-lynch left a comment

Choose a reason for hiding this comment

adam-lynch commented Sep 16, 2016

adam-lynch commented Sep 16, 2016

adam-lynch commented Sep 16, 2016

adam-lynch commented Sep 16, 2016

adam-lynch commented Sep 17, 2016

noamokman commented Sep 17, 2016

dcposch commented Aug 31, 2016 •

edited

Loading