solarized-emacs icon indicating copy to clipboard operation
solarized-emacs copied to clipboard

Split package into multiple packages

Open conao3 opened this issue 4 years ago • 25 comments

Hi! This PR's idea from #369.

Hide child-theme files into solarized-child-themes and child-theme have own palettes in own files.

conao3 avatar Dec 08 '19 11:12 conao3

  • Techically according to our own terminology most of these themes are not child themes.
  • I dont think that (require 'solarized-dark-theme) is a good idea, a new directory should only be on the theme path which IIRC does not automatically put the same files in the load path. Palettes should be loadable without touching the -theme.el files in any way. (I have not verified this)
  • the high contrast variants of the main themes should still go in the default installation package
  • there should probably be a themes directory or something where the default packaged themes are kept to keep load and theme paths separate.
  • I do not want to break https://github.com/bbatsov/solarized-emacs/blob/master/UPGRADE-GUIDE.md#2019-11-17---child-theme-usage without a significant benefit

thomasf avatar Dec 08 '19 12:12 thomasf

Quote @thomasf's comment with numbering. Please numbering list and please don't use edit many times...

  1. Techically according to our own terminology most of these themes are not child themes.
  2. I dont think that (require 'solarized-dark-theme) is a good idea, a new directory should only be on the theme path which IIRC does not automatically put the same files in the load path. Palettes should be loadable without touching the -theme.el files in any way. (I have not verified this)
  3. the high contrast variants of the main themes should still go in the default installation package
  4. there should probably be a themes directory or something where the default packaged themes are kept to keep load and theme paths separate.
  5. I do not want to break https://github.com/bbatsov/solarized-emacs/blob/master/UPGRADE-GUIDE.md#2019-11-17---child-theme-usage without a significant benefit

issue 1

I think those themes are child of solarized-themes. It just most themes use nil as child-sexp argument.

issue 2

Why? I think it is reasonable to require solarized-dark-theme to use solarized-dark-pallet

issue 3

Sorry, I'll revert it.

issue 4

It's another issue. This PR suggests to sepalates child-themes and main solarized-themes. It is not a good idea to face many issues in one PR.

issue 5

(eval-when-compile
  (require 'solarized-palettes))

eval-when-compile technique is very difficult for Emacs beginner. I use this technique for that time, but I didn't want to use that.

And now approach is very clarified to use it. If the user want to use custom pallets, define it in the theme file and use it.

And the guide is upgrade v1.0 to v2.0 guide isn't it? Now HEAD is developing master to tag v2.0, so the guide just is fixed if needed.

conao3 avatar Dec 08 '19 16:12 conao3

I checked the tree ( https://github.com/bbatsov/solarized-emacs/tree/55cd77b61b6968048c61e13358ba487d217f24c0 ) before my many changes. There are no high contrast themes. So those files should not be bundle solarized-theme.

conao3 avatar Dec 08 '19 16:12 conao3

I checked the tree ( https://github.com/bbatsov/solarized-emacs/tree/55cd77b61b6968048c61e13358ba487d217f24c0 ) before my many changes. There are no high contrast themes. So those files should not be bundle solarized-theme.

It should be in the same pacakage because it is the same palette with some contrast adjustments. It's good for people with less than optimal eye sight or if it's really bright outside.. I have noticed that on some displays solarized light in a completely dark room makes it almost impossible for me to read normal fg on bg without having to focus a lot on just looking at the screen. For these reasons the high contrast palettes should be included for accessibility and compatibility reasons.

thomasf avatar Dec 09 '19 11:12 thomasf

I don't want to download unnecessary files. Certainly, someone will be a demand for high contrast. But it should be included in the variety package, not in the main package.

And it wasn't included in the previous package. That is all. Due to my changes, solarized can now create various color themes just by preparing a color pallet. But don't bundle different color themes into the main package just because you can make different color themes. It should be a separate package.

conao3 avatar Dec 09 '19 11:12 conao3

Quote @thomasf's comment with numbering. Please numbering list and please don't use edit many times....

Three times is not many times, just reply under the quoted point

  1. Techically according to our own terminology most of these themes are not child themes.
  2. I dont think that (require 'solarized-dark-theme) is a good idea, a new directory should only be on the theme path which IIRC does not automatically put the same files in the load path. Palettes should be loadable without touching the -theme.el files in any way. (I have not verified this)
  3. the high contrast variants of the main themes should still go in the default installation package
  4. there should probably be a themes directory or something where the default packaged themes are kept to keep load and theme paths separate.
  5. I do not want to break https://github.com/bbatsov/solarized-emacs/blob/master/UPGRADE-GUIDE.md#2019-11-17---child-theme-usage without a significant benefit

issue 1

I think those themes are child of solarized-themes. It just most themes use nil as child-sexp argument. It still feels contrived and overloading, something like this is just much simpler:

├── extra <- this is the root for the solarized-theme-extra elpa package
│   └── themes <- extra themes goes there
└── themes  <- standard themes goes here

issue 2

Why? I think it is reasonable to require solarized-dark-theme to use solarized-dark-pallet

Just because that's not how the theme custom theme system is designed, one could argue that the theme system is too pedantic about user verification of loading themes when everyone just runs tens to hundreds of MELPA packages without reading any source at all but thats how it works and because of that load path and theme path should not be the same directories.

issue 3

Sorry, I'll revert it.

issue 4

It's another issue. This PR suggests to sepalates child-themes and main solarized-themes. It is not a good idea to face many issues in one PR.

Yes, but I think we need to figure out the solution to both issues because they are linked.

issue 5

(eval-when-compile
  (require 'solarized-palettes))

eval-when-compile technique is very difficult for Emacs beginner. I use this technique for that time, but I didn't want to use that.

And now approach is very clarified to use it. If the user want to use custom pallets, define it in the theme file and use it.

And the guide is upgrade v1.0 to v2.0 guide isn't it? Now HEAD is developing master to tag v2.0, so the guide just is fixed if needed.

thomasf avatar Dec 09 '19 11:12 thomasf

Where is your comments...

conao3 avatar Dec 09 '19 11:12 conao3

I don't want to download unnecessary files.

You still only download one file since the package manager.. I have no idea why you even think this is important, its a few kilobytes of data.. It wouldn't even be a large issue if we were distributing this on 5'25" 360kb floppy disks. If you are concerned about having too many files you never use Emacs itself is probably the wrong editor for you since it has more crap that probably no one or only a handful of people uses than almost any other editor in existence. If you want to be precise about these things you should probably just skip using a package manager and just copy the functions you know you are going to use individually.

If anything your positions is more like an personal preference OCD one than than any reasonable argument. I want people that installs the theme to easily find the high contrast variants if they have a hard time with the default palette and packaging them together supports that.

And it wasn't included in the previous package. That is al

decisions changes,. I have had the plan to do this for at least 5 years now but I just haven't had the time to work on it.

thomasf avatar Dec 09 '19 11:12 thomasf

No. And I have the same kind of questions to you. Why stick to bundling high-contrast themes that weren't included in solarized-theme? Looking at the source of it, it is clear that it is a solarized derivation made using the features I added, and it is sufficient for users to have the few codes in local init.el personal settings as well think.

This is speculation, but just because you need it is not wise for many users to push unnecessary files into the solarized-theme repository. I am giving evidence. The file did not exist before my change. Furthermore, do you think the users know that high-contrast solarized has been added? Those themes are not mentioned in the README, and users are simply forced to fetch unnecessary files.

@alphapapa, please tell us your thoughts?

conao3 avatar Dec 09 '19 11:12 conao3

No. And I have the same kind of questions to you. Why stick to bundling high-contrast themes that weren't included in solarized-theme?

What is this solarized-theme you are talking about, this package is what we decide it is there is not canonical right or wrong answer.

Looking at the source of it, it is clear that it is a solarized derivation made using the features I added, and it is sufficient for users to have the few codes in local init.el personal settings as well think.

As I said, I didn't have time to add all the things needed and didn't want to break stuff but since it became possible I made the change.

This is speculation, but just because you need it is not wise for many users to push unnecessary files into the solarized-theme repository.

I don't need it, I use the regular solarized light. I think you can find issues in this repository that asks for high contrast palettes but I have also had conversations with people (even professional UX/UI/graphic designers) about the solarized palette where they have brought up that the low contrast can be a problem and we have chosen to modify it for readability. (Yes, I have used the palette professionally as well)

I am giving evidence. The file did not exist before my change.

Those are not arguments, appealing to "the way things used to be" doesn't exemplify why its bad.

Furthermore, do you think the users know that high-contrast solarized has been added? Those themes are not mentioned in the README, and users are simply forced to fetch unnecessary files.

if you do an m-x load-theme they will show up. It's probably easier to find for new users who installed a bunch of themes to test them out.

@alphapapa, please tell us your thoughts?

If you are about to instigate another opinion flame war over the existence of a couple of files I have no problem in banning both of you from contributing to this repository again. Bringing in a person with a known history of not even listening to arguments and making the same complaint even after being told that it's based on faulty premises will not be helpful.

If you want to be serious you don't just bring in one specific person that you assume might agree with your own point, that is dog piling even if it's on a small scale. You try to gather opinions in way that's is an unbiased as possible. Please be more serious.

thomasf avatar Dec 09 '19 12:12 thomasf

@conao3 Just to be clear.

It's not that I don't value most of your contributions because I do. What I also have is a full time job and developer/maintainership of multiple open source packages. I don't want to spend my time arguing about non issues like if the theme package has +/- 2 files or not.

thomasf avatar Dec 09 '19 12:12 thomasf

Also, we will never be able to really tell what users needs or wants, the only way to to do that would be user studies and possibly surveys using proper statistical selection and that's simply not anything we ever will have the resources to do for a set of Emacs themes. It will always be an best effort judgement about which directions to take.

thomasf avatar Dec 09 '19 13:12 thomasf

(@thomasf), @bbatsov, (@conao3), @tarsius, @lunaryorn, @myuhe, @monsanto, @wyuenho, @syl20bnr, @vspinu

Please tell us your thoughts about this issue.

I referred https://github.com/bbatsov/solarized-emacs/graphs/contributors and pick up top 10 contributors.

conao3 avatar Dec 09 '19 13:12 conao3

I do not like this either, that is summoning people without their consent to a question that's not even clearly stated. But sure, not the damage is done anyways. Note that while contributor opinions are good they do not necessarily reflect end user opinions but I'm open for whatever rational arguments .

For those who don't want to try to figure out why they have been mentioned it is about if solarized-dark-high-contrast-theme.el and solarized-light-high-contrast-theme.el should be included in the theme (MELPA) package or not.

My main argument for keeping them there are that it's about accessibility because solarized palette can be just a tad too low contrast and having an higher contrast alternative helps and for discoverability (will just show up in the load-theme auto completions) it's better if they are all in the same package. As for the cons I have no idea how these two files possibly can have an actual practical negative impact on any Emacs user, most people probably have a few kilobytes of disk space to spare.

I haven't even seen any counter argument that isn't personal preference (I don't like downloading files) or appeal to tradition (the files were not there before) which by itself isn't a solid base for a good argument.

If anyone has anything they want to add, please do.

thomasf avatar Dec 09 '19 13:12 thomasf

For those who don't want to try to figure out why they have been mentioned it is about if solarized-dark-high-contrast-theme.el and solarized-light-high-contrast-theme.el should be included in the theme (MELPA) package or not.

I think keeping them in the main package is fine, as they are pretty closely linked to the base themes.

bbatsov avatar Dec 09 '19 14:12 bbatsov

Same opinion here. I think things should stay in the same package as it's more convenient for the end user.

vspinu avatar Dec 09 '19 14:12 vspinu

Wait for a week to gather many opinions. And I'll follow the opinions of the majority.

conao3 avatar Dec 10 '19 12:12 conao3

Sorry, I completely forgot about this issue. Revert moving high-contrast theme files. I think creating themes folder needed another PR. That change causes MELPA recipes change.

conao3 avatar Dec 24 '19 16:12 conao3

Resolve conflict.

conao3 avatar Jan 20 '20 15:01 conao3

Late to the party as always :-) Low and High contrast variants should be bundle within the same package in my opinion.

syl20bnr avatar Jan 25 '20 14:01 syl20bnr

Yes, I had agreed with it finally. I had reverted that commit.

conao3 avatar Jan 25 '20 15:01 conao3

I think we should start by creating the solarized-extras package and put the non core themes there so that is already on melpa when we remove the themes from the main package. I don't know what emacs does if someone installs both packages with the same themes though

thomasf avatar Jan 25 '20 19:01 thomasf

I also think that themes and palettes should be in separate files where the palette is on the load-path and one is on the custom-theme-load-path. It is a bit of a strange quirk of the theme system to have it's own load path but

I guess this is less of a practical problem from emacs 27 and onwards but we want to be able to require the palette without activating a theme in earlier emacs'es as well.

from etc/NEWS in emacs-27:

** Just loading a theme's file no longer activates the theme's settings.
Loading a theme with 'M-x load-theme' still activates the theme, as it
did before.  However, loading the theme's file with 'M-x load-file',
or using 'require' or 'load' in a Lisp program, doesn't actually apply
the theme's settings until you either invoke 'M-x enable-theme' or
type 'M-x load-theme'.  (In a Lisp program, calling 'enable-theme' or
invoking 'load-theme' with NO-ENABLE argument omitted or nil has the
same effect of activating a theme whose file has been loaded.)  The
special case of the 'user' theme is an exception: it is frequently
used for ad-hoc customizations, so the settings of that theme are by
default applied immediately.

The variable 'custom--inhibit-theme-enable' controls this behavior;
its default value changed in Emacs 27.1.

thomasf avatar Jan 25 '20 19:01 thomasf

btw. I can't seem to find a chapter in the Emacs manual on how to actually write a package specification (there are no info pages written about package creation?). I think I still want to end up with something similar to this though when it's all done.

── solarized-extra-themes
│  ├── themes
│  │  ├── solarized-gruvbox-dark-theme.el
│  │  ├── solarized-gruvbox-light-theme.el
│  │  ├── solarized-wombat-dark-theme.el
│  │  └── solarized-zenburn-theme.el
│  ├── solarized-extra-themes-palettes.el
│  └── solarized-extra-themes-pkg.el
├── themes
│  ├── solarized-dark-high-contrast-theme.el
│  ├── solarized-dark-theme.el
│  ├── solarized-light-high-contrast-theme.el
│  └── solarized-light-theme.el
├── Cask
├── DEV-GUIDE.md
├── Makefile
├── README.md
├── solarized-faces.el
├── solarized-palettes.el
├── solarized-theme-utils.el
├── solarized-theme.el
├── solarized.el

thomasf avatar Jan 25 '20 19:01 thomasf

Please refer https://www.gnu.org/software/emacs/manual/html_mono/elisp.html#Packaging But the sections just claim proper headers and compress method.

I think I still want to end up with something similar to this though when it's all done. Yes, I agree with you. But I commented as below one month ago.

  1. there should probably be a themes directory or something where the default packaged themes are kept to keep load and theme paths separate.

issue 4 It's another issue. This PR suggests to sepalates child-themes and main solarized-themes. It is not a good idea to face many issues in one PR.

If we move the main solarized files, we need to modify MELPA recipe. This PR purpose is just prevent useless files from being bundled in the solarized-theme package Apparently there was a difference between me and others in the perception of waste, but ultimately it had reverted as in line with your wishes.

conao3 avatar Jan 26 '20 03:01 conao3