Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Alfred 4 or newer #24

Merged

Conversation

LitoMore
Copy link
Contributor

@LitoMore LitoMore commented Aug 16, 2019

Resolves #23

Copy link
Owner

@SamVerschueren SamVerschueren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @LitoMore, thanks for the PR!

Just upgrading the package doesn't work, because it now returns an object instead of a string. For example, this line should be changed to something like this.

const getWorkflowDir = () => resolveAlfredPrefs().then(prefs => path.join(prefs.path, 'workflows'));

Same goes for the other resolveAlfredPrefs() calls

@LitoMore
Copy link
Contributor Author

LitoMore commented Aug 28, 2019

Just upgrading the package doesn't work, because it now returns an object instead of a string.

Thanks! Updated!

Copy link
Owner

@SamVerschueren SamVerschueren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going through all the files and noticed that the unlink functionality is probably broken as well because it's hardcoded for Alfred 3. Not sure what the paths are for Alfred 4 and above?

https://github.com/SamVerschueren/alfred-link/blob/master/lib/unlink.js#L17-L22

@kouchao
Copy link

kouchao commented Aug 28, 2019

@SamVerschueren

/Library/Application Support/Alfred/Workflow Data


/Library/Caches/com.runningwithcrayons.Alfred/Workflow Data

@LitoMore
Copy link
Contributor Author

@kouchao Thank you.

I think we should only unlink the version returns by the resolveAlfredPrefs. I'm writing the logic for unlink, will be updated soon.

@LitoMore
Copy link
Contributor Author

LitoMore commented Aug 28, 2019

@SamVerschueren Updated.

I also added a section Supporting Versions to readme.

Copy link
Owner

@SamVerschueren SamVerschueren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's coming together :). Last comments that needs to be resolved and I believe that should be it 👌

lib/unlink.js Outdated Show resolved Hide resolved
lib/unlink.js Outdated Show resolved Hide resolved
@SamVerschueren SamVerschueren merged commit a1a4c05 into SamVerschueren:master Aug 29, 2019
@SamVerschueren
Copy link
Owner

Awesome! Thank you very much @LitoMore! 🎉

@LitoMore LitoMore deleted the add-support-for-alfred-4 branch August 29, 2019 05:32
@LitoMore
Copy link
Contributor Author

@SamVerschueren You are welcome. Next step I am going to create a pull request on Alfy.

@SamVerschueren
Copy link
Owner

No need, semver catches this because I released it as patch. Tested it locally and works perfectly fine for me.

Had to do a quick fix here though SamVerschueren/resolve-alfred-prefs@2072c5b. Suddenly your user home path came up in my terminal 😂

@LitoMore
Copy link
Contributor Author

No need, semver catches this because I released it as patch. Tested it locally and works perfectly fine for me.

Good point.

Had to do a quick fix here though SamVerschueren/resolve-alfred-prefs@2072c5b. Suddenly your user home path came up in my terminal 😂

OK, let me create a PR to fix this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Alfred 4 or newer
3 participants