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 mu4e layer #2514

Closed
wants to merge 1 commit into from
Closed

Add mu4e layer #2514

wants to merge 1 commit into from

Conversation

darkfeline
Copy link
Contributor

Branch for developing mu4e layer for Spacemacs.

@darkfeline darkfeline mentioned this pull request Aug 2, 2015
@numkem
Copy link

numkem commented Aug 3, 2015

I can vouch that it works without problems.

@myrjola
Copy link
Contributor

myrjola commented Aug 6, 2015

Nice! 👍 Looking forward to get this merged.

@gabrielalmeida
Copy link

Great! Note that it breaks contrib/spotify which does use SPC m as group:music, bind should be remapped.

@darkfeline
Copy link
Contributor Author

@gabrielalmeida Yuck. I think an email client should take priority over using Emacs to control Spotify. Let Spotify be SPC a M. Worst case I'll make mu4e take SPC a M, but given that Emacs is a text tool, mail should take priority over music.

Edit: I've changed mu4e to SPC a M. If/when this gets accepted I'll file a change request to swap the two.

@andreas-h
Copy link
Contributor

andreas-h commented Aug 11, 2015 via email

@syl20bnr
Copy link
Owner

Let Spotify be SPC a M

👍

@cmccloud
Copy link
Contributor

Thanks for this. If there is any interest in adding some optional g-mail specific functionality to this layer, you can take a look at my config here: https://github.com/cmccloud/.spacemacs.d/blob/master/layers/mu4e/extensions.el

@darkfeline
Copy link
Contributor Author

@cmccloud I will look into borrowing some of your code, or you can pull request if you want direct credit for it.

Just the extra evilify bindings for now. I don't need the extra Gmail features myself since I keep email simple (read, delete, save a copy in my org-mode notes if I need to keep it). If others want it then I will take it if they don't interfere with non-Gmail users.

Why do you use progn in use-package? :init and :config can take multiple expressions.

@syl20bnr
Copy link
Owner

Spacemacs uses progn also ;-)

more info here: #2255

I will add it to the conventions.

@syl20bnr
Copy link
Owner

Also I'm going to merge this soon so @cmccloud can add some config if he wants.

@cmccloud
Copy link
Contributor

@darkfeline Feel free to borrow whatever you like. Honestly I think the best place for a lot of that code might be in the documentation as an example of how to add gmail specific functionality. On the other hand, none of the code will interfere with other mail services, but it may have funny behavior.

e.g. adding a /Trash tag instructs g-mail to move a message to the trash - most other services probably wont do that.

@cmccloud
Copy link
Contributor

@darkfeline i've refactored my mu4e config to use the new packages.el features if you're interested in taking a look.
@syl20bnr please correct me if i've used these incorrectly, but they seem to work just fine

https://github.com/cmccloud/.spacemacs.d/blob/master/layers/mu4e/packages.el

@syl20bnr
Copy link
Owner

I highly suggest to suppress :step post, actually I may remove this keyword because it will harm more than it helps. Only the pre step is really useful for some important package that provide services useful for the other packages.

The packages are loaded in alphabetical order so the loading order is deterministic and easy to know in advance. So if A require B then we can use some eval-after-load black magic. Throwing post step in this will just remove the benefit of the deterministic loading behavior.

(EDIT: TL;DR we don't need :step post :-))

@boj
Copy link

boj commented Oct 2, 2015

👍

@nwolfe
Copy link
Contributor

nwolfe commented Oct 7, 2015

Ping @darkfeline

@cpaulik
Copy link
Contributor

cpaulik commented Oct 7, 2015

👍

When this is merged I can add a few nice features that I have collected. Like attaching files using helm.

(mu4e//map-set account-vars)
(error "No email account found"))))

(defun mu4e/msgv-action-view-in-browser (msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a built-in function called mu4e-action-view-in-browser that does this

@darkfeline
Copy link
Contributor Author

This should be mergeable as is. I'm a bit hesitant to add anything else since I only use basic email functionality, and I don't want to push any code I haven't tested personally.

@syl20bnr Please merge unless you have additional comments, the community can add more features as they see fit.

@nwolfe
Copy link
Contributor

nwolfe commented Oct 7, 2015

@darkfeline It seems like @syl20bnr was waiting for you to remove the :step post line? That's how I'm reading his comment at least 😃

Also, the directory structure for layers has changed since this PR was submitted. This layer should now be under layers/+email/mu4e.

This is awesome, looking forward to getting this merged in so the community can build this out further.

@darkfeline
Copy link
Contributor Author

@nwolfe

Also, the directory structure for layers has changed since this PR was submitted. This layer should now be under layers/+email/mu4e.

Duh, I forgot about that.

It seems like @syl20bnr was waiting for you to remove the :step post line? That's how I'm reading his comment at least 

I don't think I've used :step post anywhere.

(As an aside/warning, I did a rebase/push --force, which is probably not recommended Git usage. The alternative would be to merge develop -> mu4e, but I didn't know if Github would then decide to include all 500+ of those commits into the PR as well. It looks like Github handled the forced rebase fairly cleanly?)

@nwolfe
Copy link
Contributor

nwolfe commented Oct 7, 2015

@darkfeline Awesome, thanks for updating. Yeah I don't know what the :step post stuff was about.

Shoot, I forgot to mention that @syl20bnr prefers PRs to be a single commit. Could you please squash these commits into one? That should be the last hurdle for merging/cherry-picking this in!

Your git moves are good; rebase is necessary and correct, and you'll have to push --force once you squash the commits locally.

@darkfeline
Copy link
Contributor Author

@nwolfe Done.

@nwolfe
Copy link
Contributor

nwolfe commented Oct 8, 2015

👍

@nwolfe
Copy link
Contributor

nwolfe commented Oct 22, 2015

Bump - I believe this is ready for merge

@dsdshcym
Copy link
Contributor

dsdshcym commented Nov 6, 2015

👍 for this layer. mu4e is way better than gnus.

@nwolfe
Copy link
Contributor

nwolfe commented Nov 6, 2015

@TheBB @syl20bnr Please merge

@TheBB
Copy link
Contributor

TheBB commented Nov 6, 2015

Maybe this is a stupid question, but where is mu4e itself? It's listed in this PR as an extension, but the code for it is not included. I also can't find a package called mu4e on MELPA.

@cpaulik
Copy link
Contributor

cpaulik commented Nov 6, 2015

mu4e is installed when you install mu. Normally into /usr/share/emacs/site-lisp. The site-lisp path should normally be in the load-path.

@nwolfe
Copy link
Contributor

nwolfe commented Nov 6, 2015

The site-lisp path should normally be in the load-path.

That's an exercise left for the reader, right? Not something we can hard-code into this layer itself?

@cpaulik
Copy link
Contributor

cpaulik commented Nov 6, 2015

I don't know if the path to site-lisp is the same for all linux distributions and I have no idea if it is fixed on OSX or Windows (if mu works on those systems?)

@mclearc
Copy link
Contributor

mclearc commented Nov 6, 2015

For OS X it is going to depend on whether the user is using stock emacs or emacs from hombrew. The latter has site-lisp in usr/local/share/emacs/

@nwolfe
Copy link
Contributor

nwolfe commented Nov 6, 2015

Yeah, not something we can hardcode then. Probably worthwhile to document in the README once this is merged though.

@d12frosted
Copy link
Contributor

But then you should add a note in README about it.

@tsenart
Copy link

tsenart commented Nov 9, 2015

+1

@TheBB
Copy link
Contributor

TheBB commented Nov 11, 2015

Thank you! Cherry-picked in develop. You can safely delete your branch. 🎉

I made some changes in 5f3793a:

  • Clarified installation instructions (as per above discussion) and bring the README in line with the current documentation conventions.
  • Added a layer variable for installation path instead of having the user mess with load-path in the init phase.
  • Moved mu4e-account-alist to config.el where it belongs.
  • Moved the main code to packages.el since extensions.el is now deprecated.
  • Define the mu4e package as built-in and not as local.
  • Auto-load mu4e on invocation of the two given keybindings, since prior to this it was not deferred.

I should note that I haven't used this layer so please, those of you who intend to use it let me know if I have messed something up. I will probably try it out in the future. :-)

@TheBB TheBB closed this Nov 11, 2015
@d12frosted
Copy link
Contributor

Nice, mu4e is now part of Spacemacs. Going to try it.

@darkfeline darkfeline deleted the mu4e branch November 12, 2015 13:01
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.