-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Adjust eaf layer to newer eaf package #14992
Conversation
lesliebinbin
commented
Aug 15, 2021
- Fix leader key for eaf camera, it should be eaf-open-camera instead of eaf-camera
- Refer to a recent commit: emacs-eaf/emacs-application-framework@044b0b8, we should now require each application seperately
Thanks for creating this PR. However, I think we should probably make the list of apps to load configurable somehow. So I suggest that you make the list a layer variable in Would you like to take care of that? Then also, it is usually preferred/required, to submit a single commit (here is a guide that explains how to squash with magit, and here is a guide about updating the PR, before or after squashing). Of course, this might be a little too much to ask if your time is limited. In that case, simply let us know, and we will take care of it. Anyway, thanks again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
layers/+tools/eaf/packages.el
Outdated
'eaf-terminal | ||
'eaf-netease-cloud-music | ||
'eaf-video-player | ||
'eaf-js-video-player |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest EAF has dropped the support to js-video-player
, feel free to remove it.
The reason is explained here
On top of my comment, I noticed that:
The EAF repos have merged to their own Github org: emacs-eaf. Although the old url will still work and redirect to the new one, I'd suggest, as a good practice, to set the remote to See more here
These lines can be safely deleted. See the conversation here for details.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make changes according to #14992 (comment)
But if we remove these, the macos user might get errors, since the spacemacs in macos still use the eaf release in May |
layers/+tools/eaf/packages.el
Outdated
'eaf | ||
(dolist (app eaf-apps) | ||
(require app nil 'noerror)) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing )
should not take a new line.
layers/+tools/eaf/packages.el
Outdated
))) | ||
(dolist (app eaf-apps) | ||
(require app nil 'noerror)))) | ||
(with-eval-after-load |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no point of having with-eval-after-load 'foo
in :init
sectiion of (use-package foo ..)
form.
move it to :config
and remove with-eval-after-load
layers/+tools/eaf/config.el
Outdated
;; You should have received a copy of the GNU General Public License | ||
;; along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
(defcustom eaf-apps "Eaf Applications" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple issues:
- For
defcustom
, the second positional arg is the default value and the third positional arg is the docstring. - The keyword arg
:type
has an invalid value(symbol)
. At very least, it should be'symbol
. Here you should use'(set (const eaf-jupyter) (const eaf-browser) ...)
. See https://www.gnu.org/software/emacs/manual/html_node/elisp/Simple-Types.html - If you intended to make this a custom variable, you should not use
defcustom
directly.
Instead you should usespacemacs|defc
, which is a wrapper ofdefcustom
. - A better docstring is required. And you also need to document it in layer's README.org.
layers/+tools/eaf/config.el
Outdated
@@ -0,0 +1,46 @@ | |||
;;; packages.el --- eaf configuration file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It's not
packages.el
- The copyright year of this file is 2021.
Using the snippet from https://github.com/emacs-eaf/emacs-application-framework#1-download-eaf, won't correctly install (quelpa '(eaf :fetcher github
:repo "emacs-eaf/emacs-application-framework"
:files ("*"))) It shows Compiling file /home/lucius/.emacs.d/elpa/27.2/develop/eaf-20210821.113/eaf.el at Sat Aug 21 01:34:44 2021
Entering directory ‘/home/lucius/.emacs.d/elpa/27.2/develop/eaf-20210821.113/’
eaf.el:167:1:Error: Cannot open load file: No such file or directory, eaf-epc
Leaving directory ‘/home/lucius/.emacs.d/elpa/27.2/develop/eaf-20210821.113/’ |
layers/+tools/eaf/README.org
Outdated
in their terminal. If the script does not work for you then [[https://github.com/manateelazycat/emacs-application-framework#dependency-list][install the | ||
required dependencies manually]]. | ||
Currently the command =SPC SPC eaf-install-dependencies= no longer works, please | ||
refer to [[https://github.com/manateelazycat/emacs-application-framework#dependency-list][install the required dependencies manually]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update all the links since the repository has been moved
I won't merge this PR until it's verified to be installable. Currently it's broken. |
Is it possible to update the release? On the other hand, we're actually planning to do real releases in the near future, after things are kinda more settled. |
Ouch, this quelpa recipe is from the pre-split period not so long ago, we haven't updated it, I've removed from the README until a working recipe is emerged. But I only use The |
layers/+tools/eaf/README.org
Outdated
- eaf-markdown-previewer | ||
- eaf-camera | ||
|
||
To customise applications, please set =eaf=app= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is =eaf=app=
a typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, thx 😄
Not quite sure but currently I set it as a custom layer and install-eaf.py works, is it because there is difference when we put
in packages.el Maybe the eaf.el is also different, currently the eaf.el config several load-path |
I will test a bit on macos, use this as a custom layer first, the difference is that when the os is not linux, it will use pip alternatives and pyqt5 install from pip got issues for long time |
@lesliebinbin |
Test it on ubuntu 20, |
Oh sorry, I may find some different, in my init.el file for spacemacs I use babel load and it requires the line (require 'eaf), is that makes the diffrence, if that is the case, is that a way to eager load eaf in packages.el? |
I thinks it is because we use defer t in packages.el for eaf, that disables the autoloads facility for eaf, I remove the defer |
you're mixing things altogether. quelpa downloads the package and install it. use-package loads the package. Not related |
@lesliebinbin It doesn't mean this PR is out of consideration. |
@lesliebinbin I am not sure how Spacemacs works on macos, but I guess you are |
@lesliebinbin Thanks for the work. The PR looks almost good to me. I have tested this PR on a clean Spacemacs install, and it seems to work fine on |
in their terminal. If the script does not work for you then [[https://github.com/manateelazycat/emacs-application-framework#dependency-list][install the | ||
required dependencies manually]]. | ||
Currently the command =SPC SPC eaf-install-dependencies= no longer works, please | ||
refer to [[https://github.com/emacs-eaf/emacs-application-framework#2-installupdate-eaf-applications-and-dependencies][install the required dependencies manually]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new commands eaf-install
and eaf-install-and-update
work perfectly fine
|
||
* Usage | ||
** Configuring applications | ||
By default, all applications provided by eaf package are loaded, including: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not necessary to remove applications from here, because you have added the NOERROR
flag to the require
statement, so Emacs will simply skip loading if some package is not available.
Still, this variable is useful for users who would like to load only some of the packages by default.
(You could add this explanation here)
- eaf-camera | ||
|
||
To customise applications, please set the variable =eaf-apps= | ||
|
||
** Configuring key bindings | ||
Configuration of EAF key bindings works in a different way than normally in Spacemacs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned by @MatthewZMD, some keybindings might be added only conditionally. As checking all these bindings is a lot of work, you could simply add a warning about it. And add a request to report keybinding issues.
@@ -20,15 +20,13 @@ | |||
;; You should have received a copy of the GNU General Public License | |||
;; along with this program. If not, see <http://www.gnu.org/licenses/>. | |||
|
|||
|
|||
(defconst eaf-packages | |||
'(ctable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the ctable
, deferred
and epc
packages declarations.
@@ -40,16 +38,12 @@ | |||
(defun eaf/init-epc () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the ctable
, deferred
and epc
init-functions
I think you can simply change this to:
@MatthewZMD You should simply add the files and locations required for building the package (see here). |
yes this works.
|
I think Since it works for you, could you please check this discussion which reported on straight, I want to know does quelpa have the same behavior - which copies the contents in the |
quelpa clone the whole repository. |
Quelpa builds the package in the same directory as the source. But I had a look at the straight problem. I will comment in that discussion. |
* checkversion/develop: (55 commits) [doc] use org-mode example block for example Added COPYRIGHT file [ivy] Fix new src block in docs [ivy] counsel search commands use spacemacs--ivy-file-actions [core] Support packing elisp to *.elc and *.el.gz files [version-control] Switch default version control diff tool to git-gutter [python] Remove comments and fix new commands [bot] "built_in_updates" Wed Sep 29 19:18:45 UTC 2021 Add pydoc to python layer for better python doc functionality [python] Make ipython version detection more robust Revise some smaller layers [bot] "documentation_updates" Fri Sep 17 19:24:14 UTC 2021 Fix (activate) company-emoji completion [bot] "documentation_updates" Fri Sep 17 18:59:01 UTC 2021 Add documentation about evilifying 'special-mode buffers' (+fix eww) Fix and update PR syl20bnr#14992: update eaf layer Update keybinding Extend spacemacs/copy-file command with extra actions (syl20bnr#14754) Add simple workaround to ebib kill buffer issue (syl20bnr#15048) [bot] "documentation_updates" Sat Sep 11 22:58:39 UTC 2021 ...