Improve color handling

[swflint]

In addition to fixing #2, this PR's goal is to improve color handling by relying on face definitions as much as possible. This should help it to better fit into dark & light themes.

Additionally, it contains some work to modularize or improve maintainability of the code.

[Hugo-Heagren]

I like this idea, but I'm not sure we need to use face.

I used faces for the other colours because I wanted automatic consistency with the standard battery colours, and those are setup as faces. But here, there isn't anything obvious to inherit from. Or in other words: from the users point of view, this is isn't the sort that is usually customised by changing a face. Maybe we should just use a colour value string?

[Hugo-Heagren]

If we do use a face here, I as a user would expect anything called *-face which controls colouring to be a face, not variable pointing at a symbol for a face. So if we do use a face (which again, I'm not sure we should), it should be something more like:

(defface svg-battery-indicator-stroke '((t :inherit mode-line))
  "Face used for battery image borders."
  :group 'svg-battery-indicator)

[Hugo-Heagren]

Sorry it's taken me a while to get round to this. I like most of your changes and refactoring. My only two concerns are:

1. I'm not sure a face (or rather, a variable with symbol pointing at a face) is the best way to customize the stroke colour
2. Quite a lot of your changes are measurements/proportions. I liked the previous propotions (which is why I used them). Clearly they won't be perfect for everyone. I think the way to go here is probably a more flexible way for users to customise the proportions (more flexible than my code or yours is so far). Would you agree?

[swflint]

Sorry it's taken me a while to get round to this. I like most of your changes and refactoring. My only two concerns are:

1. I'm not sure a face (or rather, a variable with symbol pointing at a face) is the best way to customize the stroke colour

I chose to use a face here because I switch between themes (modus-vivendi and modus-operandi switched using circadian). Because the overall appearance of the mode-line changes, using the face allows the drawn image to reflect that.

That said, I will define a face (inheriting from mode-line?) and use that instead.

2. Quite a lot of your changes are measurements/proportions. I liked the previous propotions (which is why I used them). Clearly they won't be perfect for everyone. I think the way to go here is probably a more flexible way for users to customise the proportions (more flexible than my code or yours is so far). Would you agree?

I agree, making the proportions more customizable is ideal. Some of that, I definitely missed out on, and I can start modifying this PR to move proportions to customizable variables.

[swflint]

Assuming I make the proportions more customizable, what should be a proportion? And what should it be based on?

My initial thoughts are:

• frame-char-height should be the base from which all other distances are calculated.
• The length should be that multiplied by some number (default 2.2?)
• Stroke width is a fraction of the height (default 0.17?)
• The width of the lug is a multiple of the stroke width (default 1.5?)
• So is the rounding radius (default 1.5?)

This is to make sure that the indicator scales well on different displays, or based on the font configuration.

[Hugo-Heagren]

I chose to use a face here because I switch between themes (modus-vivendi and modus-operandi switched using circadian). Because the overall appearance of the mode-line changes, using the face allows the drawn image to reflect that.

Would it not be possible to achieve this with custom-theme-set-variables, if the colour was set with a variable?

[swflint]

Yes, using custom-theme-set-variables would achieve the same goal, but in a way that is more complicated for the end user.

However, by using a face, they can customize or override the color, but they can also (as done in the proposed default) easily allow their theme to manage the color(s) directly. This means less configuration and a more consistent appearance for the user, but still having the option to directly customize.

[Hugo-Heagren]

Assuming I make the proportions more customizable, what should be a proportion? And what should it be based on?

Sorry it has taken me so long to come back to this.

Having thought about it a fair bit, I think the best approach here is to make everything possible an individual customisable option, and leave considerations of proportion and size to:

• the default values of those vars (in the first instance)
• the user's own customisation (in the second)

I think this is a good idea because anything else will ultimately stop the user from doing certain kinds of customisation. Say we go with exactly your proposal above -- what if the user wants slightly different proportions? Ok, maybe we can (a) custom variable(s) for the scalar with scales everything. But what if they want different proportions in some places and not in others? Etc...

I think if the user wants certain proportionality/correspondences between things, they can set that up themselves in their init file -- the code isn't especially complex.

I don't have time this second to make the relevant changes, but I/you can at some point.

[Hugo-Heagren]

Yes, using custom-theme-set-variables would achieve the same goal, but in a way that is more complicated for the end user.

However, by using a face, they can customize or override the color, but they can also (as done in the proposed default) easily allow their theme to manage the color(s) directly. This means less configuration and a more consistent appearance for the user, but still having the option to directly customize.

I'm pretty convinced by the reasoning here -- go ahead (though as you and I seem to agree, I think a newly-defined face is best, rather than a variable pointing at an already-defined one).

[swflint]

Yes, using custom-theme-set-variables would achieve the same goal, but in a way that is more complicated for the end user.
However, by using a face, they can customize or override the color, but they can also (as done in the proposed default) easily allow their theme to manage the color(s) directly. This means less configuration and a more consistent appearance for the user, but still having the option to directly customize.

I'm pretty convinced by the reasoning here -- go ahead (though as you and I seem to agree, I think a newly-defined face is best, rather than a variable pointing at an already-defined one).

This, I believe, is already done (and works quite well for me locally).

[swflint]

Assuming I make the proportions more customizable, what should be a proportion? And what should it be based on?

Sorry it has taken me so long to come back to this.

Having thought about it a fair bit, I think the best approach here is to make everything possible an individual customisable option, and leave considerations of proportion and size to:

* the default values of those vars (in the first instance)

* the user's own customisation (in the second)

I think this is a good idea because anything else will ultimately stop the user from doing certain kinds of customisation. Say we go with exactly your proposal above -- what if the user wants slightly different proportions? Ok, maybe we can (a) custom variable(s) for the scalar with scales everything. But what if they want different proportions in some places and not in others? Etc...

Making everything possible an individual option could easily break the drawing of the battery, depending on the user's customizations.

That said, I think a possible approach would be:

• We define a customize variable svg-battery-indicator-size-function; this function takes three arguments, the frame character height, the battery percent, and the charging bool. It returns the height, length, stroke width, lug width, lug height, rounding radius, and lighting bolt placement parameters (x & y).
• We provide two functions for this variable, a default which returns the current values for these, and one that uses the parameters I've proposed.

If we go with this approach, I could pretty easily hack this in a couple of hours.

[Hugo-Heagren]

Making everything possible an individual option could easily break the drawing of the battery,

I'm not sure how you mean?

We define a customize variable svg-battery-indicator-size-function; this function takes three arguments, the frame character height, the battery percent, and the charging bool. It returns the height, length, stroke width, lug width, lug height, rounding radius, and lighting bolt placement parameters (x & y).

I take it part of the draw here is the user could -- should they wish -- write their own function. But if they did, how would that be functionally different (for the user) from just setting these values in defcustom variables? Won't the threat of breaking the display be exactly the same?

[swflint]

Making everything possible an individual option could easily break the drawing of the battery,

I'm not sure how you mean?

Realistically, it would likely be pretty hard, but it could just end up with a garbled display. My initial concern was overblown.

We define a customize variable svg-battery-indicator-size-function; this function takes three arguments, the frame character height, the battery percent, and the charging bool. It returns the height, length, stroke width, lug width, lug height, rounding radius, and lighting bolt placement parameters (x & y).

I take it part of the draw here is the user could -- should they wish -- write their own function. But if they did, how would that be functionally different (for the user) from just setting these values in defcustom variables? Won't the threat of breaking the display be exactly the same?

There's a similar concern with breaking the display, yes. But this approach would give users the ability to implement their own calculations, for example, having some values (stroke width) fixed, with others relative.

[Hugo-Heagren]

But this approach would give users the ability to implement their own calculations, for example, having some values (stroke width) fixed, with others relative.

Exactly -- that was the point of my suggestion. What I'm asking is, what is the advantage of this:

(defun my/svg-battery-indicator-size-function (char-ht percentage charging)
  (let* ((len (* 2.2 char-ht))
	 (sw (* 0.17 char-ht))
	 (lug-width (* 1.5 sw))
	 (lug-ht 8)
	 (round-rad (* 1.5 sw)))
    `(,char-ht ,len ,sw ,lug-width ,round-rad)))

over this (in init.el):

(let ((fch (frame-char-height)))
  (setq svg-battery-indicator-length (* 2.2 fch))
  (setq svg-battery-stroke-width (* 0.17 fch))
  (setq svg-battery-lug-width (* 1.5 svg-battery-stroke-width))
  (setq svg-battery-lug-height 8)
  (setq svg-battery-rounding-radius (* 1.5 svg-battery-stroke-width)))

Personally I find the former annoying to deal with. I don't see what a functoin adds over a series of variables, when all the function does is some calculations which are passed back in a list (i.e. why one function returning one list of many things, rather than simply having many variables?)

[swflint]

Ahh, okay, I see where the confusion lies.

It's unreasonable to assume that the result of (frame-char-height) is constant between frames or throughout a session, because a user may change their global font scale (C-x C-M-+, etc.), through a font change, or for some other reason. For example, I set different frames to have different font sizes based on which monitor it's being placed on (not automatically, mind).

By calculating these values dynamically, the indicator can (and will) be able to fit into any frame as needed.

I have about 2/3 of an implementation based on my above proposal written, I'll push later today.

[Hugo-Heagren]

Oh I seeeeee!

Yes, I completely agree.

Side thought: I think it might be good to define a separate colour (or face, as we're using faces for colour control) for the lightning bolt charging indicator. At the moment it's just always the same colour as the outer strokes and that's fine as a default, but might not work in some themes?

[swflint]

Apologies for not communicating that clearly previously. The whole reason I started with this was to get it to fit in with my environment, themes & changing font sizes.

I'll add that as another commit once I finish getting the rest of it in.

[swflint]

We should have a separate face for the bolt now as well. What are your thoughts?