rougier / elegant-emacs

A very minimal but elegant emacs (I think)
GNU General Public License v3.0
1.36k stars 79 forks source link

Melpa #12

Closed NicoloZorzetto closed 3 years ago

NicoloZorzetto commented 3 years ago

This commit is for Melpa. You could create a separate branch like I suggested but it's not necessary. I separated the two themes because people told me they prefer this way and it's going to be easier to publish on Melpa. I modified them following Melpa's guides, Flyckeck and Package-Lint.

Tell me if something doesn't look right to you and I'll make sure to see what we can do.

NicoloZorzetto commented 3 years ago

I would also suggest to drop the font settings since they would only add to requirements and they're usually set by the user in their init, maybe mentioning in the README that the theme was designed to work with them but I didn't take such artistic decisions until I asked you.

rougier commented 3 years ago

Fantastic ! Thank you very much. For the font, I woudl prefer to try to stick to Roboto Mono knowing that emacs will fallback to a default font if not found, what do you think ? Maybe we can first add it and open an issue to ask users.

rougier commented 3 years ago

By the way, can't we factorize code between light and dark theme or is it some requirements for MELPA packaging ?

rougier commented 3 years ago

What else is needed before I merge your PR ?

NicoloZorzetto commented 3 years ago

What else is needed before I merge your PR ?

I'd maybe wait for this evening or tomorrow morning so I can push the elegant-emacs-common.el and updated themes if it's not a problem for you

NicoloZorzetto commented 3 years ago

Hello, it is now ready. You can clone the fork of Melpa I created [https://github.com/NicoloZorzetto/melpa], go to melpa/recipies/elegant-emacs and evaluate the buffer with C-c C-c. This will install the package only in the current frame because it's only for testing, once on melpa it'll behave like any normal package. You'll still be able to load the themes thru M-x load-theme RET elegant-emacs-variant RET.

I have divided the themes and a 'common'. Now the themes themselves only contain the distinctive colors, the rest is contained in "elegant-emacs-common.el" so to help you with future changes.

Remember to change "NicoloZorzetto/elegant-emacs" to "Rougier/elegant-emacs" after accepting my pull request so melpa's fetchers will look at your repo. After that we just need to open a pull request for Melpa, I can help you with that but it's stated that it's best for the author to add the pull request. Just fork the Melpa repo [https://github.com/melpa/melpa], clone the fork, delete everything inside and paste everything that's in my fork, push and then open a pull request. Melpa maintainers are volunteers so it can take a bit for them to go thru all pull requests and they might ask us to change some things. I'll keep an eye on the pull request myself but if you need anything you can contact me here: nicolozorzetto@protonmail.com or on github.

Will wait for thoughts, sorry if this update came in a bit late.

rougier commented 3 years ago

Fantastic ! Thank you for the detailed explanation. I'll merge tonight and make the PR on MELPA.

rougier commented 3 years ago

From the first step (cloning your fork / compiling), I got:

Compiling file /Users/rougier/.emacs.d/elpa/elegant-emacs-20200805.1051/elegant-emacs-common.el at Wed Aug  5 21:40:04 2020
Entering directory ‘/Users/rougier/.emacs.d/elpa/elegant-emacs-20200805.1051/’
elegant-emacs-common.el:74:1:Warning: defface for ‘fallback’ fails to specify
    containing group

Compiling file /Users/rougier/.emacs.d/elpa/elegant-emacs-20200805.1051/elegant-emacs-dark-theme.el at Wed Aug  5 21:40:04 2020
elegant-emacs-dark-theme.el:43:1:Error: Wrong type argument: char-table-p, nil

Compiling file /Users/rougier/.emacs.d/elpa/elegant-emacs-20200805.1051/elegant-emacs-light-theme.el at Wed Aug  5 21:40:04 2020
elegant-emacs-light-theme.el:43:1:Error: Invalid face: info-menu-header
rougier commented 3 years ago

Just realized you removed all the eval-after-load, that might be the problem.

a13 commented 3 years ago

These are not proper Emacs themes

https://www.gnu.org/software/emacs/manual/html_node/elisp/Custom-Themes.html#Custom-Themes

So, faces should be set by custom-theme-set-faces, and variables by custom-theme-set-variables, though I think some of them should be set by a separate minor-mode, since some people may prefer to keep their own window settings

a13 commented 3 years ago

Besides that, why do you commit backup files?

a13 commented 3 years ago

For the font, I woudl prefer to try to stick to Roboto Mono knowing that emacs will fallback to a default font if not found, what do you think ?

By default Emacs doesn't fallback and throws an error, so better let a user decide

rougier commented 3 years ago

To throw an error in the GUI, it needs to fallback to some font I guess.

NicoloZorzetto commented 3 years ago

Besides that, why do you commit backup files?

probably didn't notice and got it pretty fast.

Just realized you removed all the eval-after-load, that might be the problem.

Where is it? I thought I changed all of them. We shouldn't use with-eval-after-load but instead remove that and put 'before (

EDIT: confirm, with-eval--after-load is not there. I can remove the fonts as I suggested but we can't think of this as a normal theme like a13 was saying because this goes far beyond just colors

a13 commented 3 years ago

To throw an error in the GUI, it needs to fallback to some font I guess.

Sadly, there's no "fine" way to fallback, I use a hacky way to workaround it

rougier commented 3 years ago

@NicoloZorzetto What is '(doing is that case ? Sorry for the probaly stupid question but I'm far from mastering all elisp subtleties.

@a13 The system font manager is responsible for providing a fallback, no? Or does Emacs manages font without it?

a13 commented 3 years ago

@NicoloZorzetto I proposed some time ago that Elegant Emacs should be split to several packages: — light theme — dark theme — a minor mode with window/frame settings — mode-line-format settings (possibly as a minor mode too)

a13 commented 3 years ago

@a13 The system font manager is responsible for providing a fallback, no?

At least on Linux(Xorg) it's possible to set fonts by an alias, like "Monospace" or "Serif" etc, but you have to use aliases instead of real names in that case. Another way is to use face-font-family-alternatives Emacs customisable variable (that's how I do, see the link above)

NicoloZorzetto commented 3 years ago

@NicoloZorzetto I proposed some time ago that Elegant Emacs should be split to several packages: — light theme — dark theme — a minor mode with window/frame settings — mode-line-format settings (possibly as a minor mode too)

possible but would require quite a bit of time for me.

NicoloZorzetto commented 3 years ago

Any news? Do I need to do something?

rougier commented 3 years ago

Can your remove the backup file? Also, you did not answer my question about eval-after-load. Is the theme taking care of not setting face if they are not yet defined ? This was the reason for all my eval-after-load

NicoloZorzetto commented 3 years ago

Can your remove the backup file? Also, you did not answer my question about eval-after-load. Is the theme taking care of not setting face if they are not yet defined ? This was the reason for all my eval-after-load

I have not removed the faces, I have removed the eval-after-load. Everything works for me, if you want to clone it and try to see for yourself it would be helpful. Removed the backup file.

rougier commented 3 years ago

On emacs 27.1 I got:

Compiling file /Users/rougier/.emacs.d/elpa/elegant-emacs-20200805.1051/elegant-emacs-common.el at Mon Aug 17 10:50:08 2020
Entering directory ‘/Users/rougier/.emacs.d/elpa/elegant-emacs-20200805.1051/’
elegant-emacs-common.el:74:1:Warning: defface for ‘fallback’ fails to specify
    containing group

Compiling file /Users/rougier/.emacs.d/elpa/elegant-emacs-20200805.1051/elegant-emacs-dark-theme.el at Mon Aug 17 10:50:08 2020
elegant-emacs-dark-theme.el:43:1:Error: Wrong type argument: char-table-p, nil

Compiling file /Users/rougier/.emacs.d/elpa/elegant-emacs-20200805.1051/elegant-emacs-light-theme.el at Mon Aug 17 10:50:08 2020

and both header-line / mode-line faces are wrong.

NicoloZorzetto commented 3 years ago

On emacs 27.1 I got:

Compiling file /Users/rougier/.emacs.d/elpa/elegant-emacs-20200805.1051/elegant-emacs-common.el at Mon Aug 17 10:50:08 2020
Entering directory ‘/Users/rougier/.emacs.d/elpa/elegant-emacs-20200805.1051/’
elegant-emacs-common.el:74:1:Warning: defface for ‘fallback’ fails to specify
    containing group

Compiling file /Users/rougier/.emacs.d/elpa/elegant-emacs-20200805.1051/elegant-emacs-dark-theme.el at Mon Aug 17 10:50:08 2020
elegant-emacs-dark-theme.el:43:1:Error: Wrong type argument: char-table-p, nil

Compiling file /Users/rougier/.emacs.d/elpa/elegant-emacs-20200805.1051/elegant-emacs-light-theme.el at Mon Aug 17 10:50:08 2020

and both header-line / mode-line faces are wrong.

it doesn't give any errors to me. In what way are they wrong?

rougier commented 3 years ago

Did you tried with emacs -q ?

rougier commented 3 years ago

@NicoloZorzetto I've invited you to the repository such that you have commit rights. Since we're a bit out of sync on this PR, it might make things faster. I'm not sue about the originof the problem I get. Let's try to make a release on MELPA and we'll see if I'm the only one with this problem.

NicoloZorzetto commented 3 years ago

Sure, thank you very much. Why do you start emacs with -q?

I've looked into it and setting any font thru a theme seems to be quite frown upon. This would include the fallback as we've discussed previously. I'll try to check github more since the emails seem to be coming 2 days late.

I will give a look at the modeline/header-line faces tomorrow, probably just got deleted with the eval-after-load(s). Sorry for the big mess this pull request has become, totally on me.

rougier commented 3 years ago

I tested with the -q switch to make sure my own configuration does not interfere with the testing. And thanks for your wrok on packaging this. Once on melpa, things will be smoother I guess.

NicoloZorzetto commented 3 years ago

I have resolved the errors. Will now push to your repo and change the Melpa repo so the fetchers will look in yours.

If you have time you could explain to me the mode-line and header-line differences between mine and yours because as I can see the code looks identical.

NicoloZorzetto commented 3 years ago

Apparently I do not have push access to your repo. If you want to you could give it to me or just merge the pull request.

If you prefer to check those faces first I'm happy to comply.

rougier commented 3 years ago

Merged !

NicoloZorzetto commented 3 years ago

Thanks! Now you can push to Melpa! I can do it if you want, it's written that it's best for the author to do so but if you can't it's ok I think.

If you want to do it fork Melpa's repository here on Github, clone it and either: remove everything and put inside it everything that's in my fork or add a file to melpa/recipies/ called elegant-emacs and put inside it this:


 (elegant-emacs :fetcher github
 :repo "rougier/elegant-emacs"
 :files ("elegant-emacs-common.el" "elegant-emacs-light-theme.el" "elegant-emacs-dark-theme.el"))

then push to your repo and ask for a merge on the official Melpa repository. They'll probably ask us to change something, if they do you're free to contact me

rougier commented 3 years ago

Hi @NicoloZorzetto

Sorry for the delay. If you can do it, that would be perfect since I'm quite busy until next week.

NicoloZorzetto commented 3 years ago

Hello, here is the link to the pull request for Melpa [https://github.com/melpa/melpa/pull/7134]. I'd suggest keeping an eye on it in case they ask us to modify something and I'm for some reason unavailable.

I don't want to seem pety but is there any chance you could mention me somewhere? If not in the contributors list a simple line in the 3 files mentioning NicoloZorzetto [github: NicoloZorzetto, twitter: @NicoloZorzetto] as the packager would be so cool.

rougier commented 3 years ago

Oh Thanks. And to answer your question, of course ! Best would be to add your name in the copyright on each .el file along mine, what do you think? (I consider we're now two authors given all the work you've done on this package). We could also add a short line in the README.

NicoloZorzetto commented 3 years ago

Oh Thanks. And to answer your question, of course ! Best would be to add your name in the copyright on each .el file along mine, what do you think? (I consider we're now two authors given all the work you've done on this package). We could also add a short line in the README.

that would be absolutely awesome. I would love my name in the contributors but that's up to you. The name in every file and readme would make me jizzle in my pants but to be clear you don't have to do this if you don't want to

rougier commented 3 years ago

No problem, you've contributed a lot already.

brongulus commented 3 years ago

Hi, Has the theme been ported over to melpa?

NicoloZorzetto commented 3 years ago

hi, they replied and asked me to make some changes. Will now look into it.

https://github.com/melpa/melpa/pull/7134

rougier commented 3 years ago

It's quite verbose but it is a nice and helpful output. Can we rename elegant-emacs-common.el as just elegant.el (+ elegant-light.el elegant-dark.el)?