support alfred 4 and 3 #5

karlsander · 2019-06-02T20:10:38Z

This PR adds support for Alfred 4, which completely drops the version postfix while keeping support for v3.

It's not exactly elegant, but supporting 2 different paths just isn't I guess. Here's an alternative, simple PR that just drops v3 support.

LitoMore · 2019-06-06T08:09:25Z

I think we could add a parameter to the method like resolveAlfredPrefs(options).

Then we can use this module in alfy twice for checking both Alfred 3 and Alfred 4.

LitoMore · 2019-06-06T08:15:29Z

@SamVerschueren @sindresorhus Any comments with this discussion?

LitoMore · 2019-06-06T08:25:04Z

More details for my idea:

`resolveAlfredPrefs(options)`

`options.settingsPath`

Default to /Library/Preferences/com.runningwithcrayons.Alfred-Preferences.plist, the latest version of Alfred.

When we using this module in alfy, we should check both Alfred 3 and Alfred 4.

const prefsLatest = await resolveAlfredPrefs();
const prefs3 = await resolveAlfredPrefs({
  settingsPath: '/Library/Preferences/com.runningwithcrayons.Alfred-Preferences.plist'
})

// Doing symlink for Alfred 3 and Alfred 4
// ...

deanishe · 2019-06-26T20:29:19Z

For Alfred 4+, the correct way to get the path to Alfred's preferences bundle is to read the current value from ~/Library/Application Support/Alfred/prefs.json.

The file was added specifically for this purpose.

LitoMore · 2019-06-27T03:13:38Z

I will make a new pull request to add support for both 3 and 4. Then I will ask Sindre to help me review my pull request.

sindresorhus · 2019-06-30T09:45:27Z

Alright. I'm ok with supporting Alfred 3 and 4 using the method @deanishe has suggested.

LitoMore · 2019-07-02T08:58:28Z

@sindresorhus Here are my ideas about the API design, could you give me some advice?

Idea 1

const resolveAlfredRefs = require('resolve-alfred-refs');

(async () => {
  console.log(await resolveAlfredRefs());
  //=> {alfred3: '/path/to/alfred-3-prefs', alfred4: undefined}
})();

If Alfred 3/4 does not exist, the value of alfred4 will be undefined. This function will never throw any error.

Idea 2

const {resolveAlfred3Prefs, resolveAlfred4Prefs} = require('resolve-alfred-refs');

(async () => {
  const alfred3 = await resolveAlfred3Prefs();
  console.log(alfred3);
  //=> '/path/to/alfred-3-prefs'

  const alfred4 = await resolveAlfred4Prefs();
  console.log(alfred4);
  //=> '/path/to/alfred-4-prefs'
})();

If Alfred 3/4 does not exist or the user doesn't have permission to access, the function will throw an error.

Idea 3

Make a new module for Alfred 4:

I created a new package resolve-alfred-4-refs (not published to npm yet) to resolve the preferences file for Alfred 4.

We can use this package in alfred-link directly.

sindresorhus · 2019-07-02T10:12:37Z

I would prefer:

const resolveAlfredRefs = require('resolve-alfred-refs');

(async () => {
  console.log(await resolveAlfredRefs());
  //=> {isAlfred4: true, path: '/path/to/alfred-4-prefs'}
})();

deanishe · 2019-07-05T20:55:37Z

Make a new module for Alfred 4

I don't think this is a good idea, at least not an Alfred 4-specific module. Alfred 4+ will all use the same directories (i.e. no more "Alfred X" and no more workflows breaking with every major update). It should mostly be a case of one (legacy) method for Alfred 3 and one new one for every later version.

You should use prefs.json first (which should work with Alfred 4, Alfred 5, etc.) and only fall back to the old method for Alfred 3 if prefs.json doesn't exist.

To the same ends, any JXA calls should use Application('com.runningwithcrayons.Alfred'), not Application('Alfred 4').

SamVerschueren · 2019-07-06T17:30:06Z

I agree with what @sindresorhus commented. Something in line with

const resolveAlfredRefs = require('resolve-alfred-refs');

(async () => {
  console.log(await resolveAlfredRefs());
  //=> {isAlfred4: true, path: '/path/to/alfred-4-prefs'}
})();

Or {version: '4', path: '/path/to/alfred-4-prefs'}. This way, we have an upgrade path if v5 etc. comes out.

sindresorhus · 2019-07-06T17:39:58Z

@SamVerschueren Sounds good. I don't think we even need the version number, I guess we can have it just in case.

deanishe · 2019-07-06T18:25:30Z

I don't think we even need the version number, I guess we can have it just in case.

You need the version number to abort an update to an incompatible version, don't you? Or is it already too late to do that when this module is called?

SamVerschueren · 2019-07-06T18:45:53Z

That could definitely be added to alfred-link. But it also means that every workflow should define the compatible Alfred version.

deanishe · 2019-07-06T19:07:47Z

But it also means that every workflow should define the compatible Alfred version.

I'm not familiar with the way you have this whole workflows-via-npm business working. The main issue to avoid, imo, is updating workflows to newer, incompatible versions.

If updating a workflow requires users to run npm themselves, I don't think it's a problem. (Well, it's their problem.)

You just want to avoid workflows overwriting themselves with incompatible versions.

LitoMore · 2019-07-14T07:59:33Z

You should use prefs.json first (which should work with Alfred 4, Alfred 5, etc.) and only fall back to the old method for Alfred 3 if prefs.json doesn't exist. (@deanishe)

Good point.

So I think the API returns should be:

// If Alfred 4 or later
{path: '/path/to/prefs'}

// If Alfred 3
{version: '3', path: '/path/to/prefs'}

SamVerschueren · 2019-08-01T05:24:58Z

index.js


-	if (!exists) {
-		throw new Error(`Alfred preferences not found at location ${settings}`);
+	if (!(latestExists || v3Exists)) {


IMO it's easier to read if you did it like this

Suggested change

if (!(latestExists || v3Exists)) {

if (!latestExists && !v3Exists) {

SamVerschueren · 2019-08-01T05:31:22Z

test/test.js


 const mv = (src, dest) => pify(fs.rename)(src, dest).catch(() => { });

 test.before(async () => {
 	await mv(settings, `${settings}.back`);
-	await cp(path.join(__dirname, 'fixtures/com.runningwithcrayons.Alfred-Preferences-3.plist'), settings);
+	await cp(path.join(__dirname, 'fixtures/com.runningwithcrayons.Alfred-Preferences.plist'), settings);


We should test both Alfred 3 and latest.

So maybe we can make it a test macro?

function macro(t, filename, expected) { t.is(eval(input), expected); } macro.title = (providedTitle, filename) => `resolves \`${filename}\``; test(macro, 'Alfred-Preferences.plist', path.join(userHome, 'Documents/alfred/Alfred.alfredpreferences')); test(macro, 'Alfred-Preferences-3.plist', path.join(userHome, 'Documents/alfred/Alfred.alfredpreferences'));

SamVerschueren · 2019-08-01T05:35:01Z

So I think the API returns should be:

// If Alfred 4 or later
{path: '/path/to/prefs'}

// If Alfred 3
{version: '3', path: '/path/to/prefs'}

It would be nice if we could retrieve the version from the plist file to always provide it. That way, we could enforce a engines.alfred setting in package.json so it fails to install if you are installing a Alfred 4 workflow for Alfred 3.

So something like this would be better

// If Alfred 3
{version: 3, path: '/path/to/prefs'}

// If Alfred 4
{version: 4, path: '/path/to/prefs'}

// If Alfred 5
{version: 5, path: '/path/to/prefs'}

(Not sure if versions are numeric though).

If that's not possible, the solution you provided is fine.

LitoMore · 2019-08-01T05:50:15Z

Hi @SamVerschueren, please review this pull request #8, it's the latest one. My pull request will close #5 and #6 when it merged.

It would be nice if we could retrieve the version from the plist file to always provide it. That way, we could enforce engines.alfred setting in package.json so it fails to install if you are installing an Alfred 4 workflow for Alfred 3. (@SamVerschueren)

Currently, we don't have a good way to get the version for Alfred 4 or later.

karlsander · 2019-08-01T14:55:56Z

hey, sorry for not keeping up with the discussion. Does #8 already superseed this PR or should I get it into shape to be merged with the new suggested api?

LitoMore · 2019-08-01T17:35:22Z

@karlsander My new PR #8 will fix this issue. You could suspend or close current issue. I used GitHub issue keywords to close this issue. When my PR merges, your PR will also be closed at the same time.

SamVerschueren · 2019-08-02T05:23:56Z

No problem @karlsander, let's continue in #8

support alfred 4 and 3

2f1f928

karlsander mentioned this pull request Jun 2, 2019

support alfred 4 (and later) #6

Closed

LitoMore mentioned this pull request Jun 6, 2019

Support Alfred 4 sindresorhus/alfy#110

Closed

jeffbyrnes mentioned this pull request Jun 19, 2019

Add support for Alfred 4 #7

Closed

LitoMore mentioned this pull request Jul 14, 2019

Add support for Alfred 4 or newer #8

Merged

SamVerschueren requested changes Aug 1, 2019

View reviewed changes

SamVerschueren closed this Aug 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support alfred 4 and 3 #5

support alfred 4 and 3 #5

karlsander commented Jun 2, 2019 •

edited

LitoMore commented Jun 6, 2019 •

edited

LitoMore commented Jun 6, 2019 •

edited

LitoMore commented Jun 6, 2019 •

edited

deanishe commented Jun 26, 2019 •

edited

LitoMore commented Jun 27, 2019

sindresorhus commented Jun 30, 2019

LitoMore commented Jul 2, 2019

sindresorhus commented Jul 2, 2019 •

edited

deanishe commented Jul 5, 2019 •

edited

SamVerschueren commented Jul 6, 2019

sindresorhus commented Jul 6, 2019

deanishe commented Jul 6, 2019 •

edited

SamVerschueren commented Jul 6, 2019

deanishe commented Jul 6, 2019

LitoMore commented Jul 14, 2019

SamVerschueren Aug 1, 2019

SamVerschueren Aug 1, 2019

SamVerschueren commented Aug 1, 2019

LitoMore commented Aug 1, 2019 •

edited

karlsander commented Aug 1, 2019

LitoMore commented Aug 1, 2019 •

edited

SamVerschueren commented Aug 2, 2019

	if (!(latestExists \|\| v3Exists)) {
	if (!latestExists && !v3Exists) {

support alfred 4 and 3 #5

support alfred 4 and 3 #5

Conversation

karlsander commented Jun 2, 2019 • edited

LitoMore commented Jun 6, 2019 • edited

LitoMore commented Jun 6, 2019 • edited

LitoMore commented Jun 6, 2019 • edited

resolveAlfredPrefs(options)

options.settingsPath

deanishe commented Jun 26, 2019 • edited

LitoMore commented Jun 27, 2019

sindresorhus commented Jun 30, 2019

LitoMore commented Jul 2, 2019

Idea 1

Idea 2

Idea 3

sindresorhus commented Jul 2, 2019 • edited

deanishe commented Jul 5, 2019 • edited

SamVerschueren commented Jul 6, 2019

sindresorhus commented Jul 6, 2019

deanishe commented Jul 6, 2019 • edited

SamVerschueren commented Jul 6, 2019

deanishe commented Jul 6, 2019

LitoMore commented Jul 14, 2019

SamVerschueren Aug 1, 2019

Choose a reason for hiding this comment

SamVerschueren Aug 1, 2019

Choose a reason for hiding this comment

SamVerschueren commented Aug 1, 2019

LitoMore commented Aug 1, 2019 • edited

karlsander commented Aug 1, 2019

LitoMore commented Aug 1, 2019 • edited

SamVerschueren commented Aug 2, 2019

karlsander commented Jun 2, 2019 •

edited

LitoMore commented Jun 6, 2019 •

edited

LitoMore commented Jun 6, 2019 •

edited

LitoMore commented Jun 6, 2019 •

edited

`resolveAlfredPrefs(options)`

`options.settingsPath`

deanishe commented Jun 26, 2019 •

edited

sindresorhus commented Jul 2, 2019 •

edited

deanishe commented Jul 5, 2019 •

edited

deanishe commented Jul 6, 2019 •

edited

LitoMore commented Aug 1, 2019 •

edited

LitoMore commented Aug 1, 2019 •

edited