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 ability to setq, and statically define the particle color #9

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

fxkrait
Copy link

@fxkrait fxkrait commented May 28, 2023

Implementation for #8.

  • (Uses an if statement for early-termination and return if the requisite setq exists).
  • (Note: And adds an additional, missing terminating parenthesis to power-mode--foreground-color-before-point to make it balanced)

Behavior look to be OK, from my testing.


Behavior in 4 cases:

1: No setq

  • uses foreground text color (the original/prior behavior)

2: Setq, valid color

  • Uses valid setq color
    • (setq power-mode-streak-particle-color "medium orchid")

3: setq, nil

  • Uses foreground text color (original behavior)

4: setq, invalid color

  • Undefined Color error, which makes sense
    image

@fxkrait
Copy link
Author

fxkrait commented May 28, 2023

2 things:

  1. I don't know if this static color should be customizable (user-settable) via customize-face (defface) instead of customize-variable as it currently stands.
  2. Currently, the color is initialized to nil, which while technically works for this purpose, has the side effect of breaking the Choose menu until a valid color is later set - either setqd or via customize-variable.
    image
    image
    nil is not a valid color value, thus the (mismatch) displayed in green (see the image above). Per here, I guess colors don't have a nil, but they do have unspecified, but I can't get that to work.
    Per that link, I tried (unsuccessfully) initializing the defcustom with:
  • default
  • '(:inherit default)
  • "unspecified"
  • 'unspecified
  • unspecified

However, if you initialize it to a valid color in the defcustom, or setq a valid color later, the drop down interface works fine again (Choose).
image

@fxkrait
Copy link
Author

fxkrait commented May 29, 2023

Alright, I took a look at faces (defface). I believe the current PR, using a variable with nil defcustom is better than a nil defface, because the customize-variable actually shows a nil value, which the user can update, whereas defface (customize-face) will list no attributes (no foreground, and no nil), so someone would have to go and add a foreground attribute, and then a color, which is worse.

I can't figure out how to set a non-color default value, that leaves a blank foreground attribute, and retains the customize Choose menu available.

a) nil (no foreground attribute, no Choose):

(defface power-mode-streak-particle-color
  '((t :foreground nil
       ))
  "Streak particle color. If nil it uses the foreground text color."
  :group 'power)

image

  • Also tried some default stuff, doesn't work.

b) init to valid color (foreground attribute, and Choose menu shows)

(defface power-mode-streak-particle-color
  '((t :foreground "medium orchid"
       ))
  "Streak particle color. If nil it uses the foreground text color."
  :group 'power)

image

Tried to see, referencing here, if I can then get rid of the foreground value after the fact, but it doesn't work. Foreground color is the same.

(face-remap-add-relative 'power-mode-streak-particle-color '(:inherit default))
(face-remap-add-relative 'power-mode-streak-particle-color `(:foreground ,(face-foreground 'default)))

You can get the foreground value and use it in an if statement, and it would work the same as the variable form in the current PR (nil foreground would go to the else case):

  (if (face-foreground  'power-mode-streak-particle-color )
   (face-foreground  'power-mode-streak-particle-color ) ;; would output something like: medium orchid
   "else"
)

And then instead of setq to set the variable, you have to do the following to set the face:

(set-face-attribute 'power-mode-streak-particle-color nil :foreground "medium orchid")
  • This is pretty ugly. I like the setq better with a regular variable, as the setq is simpler (no nil :foreground), and more commonplace.

@fxkrait
Copy link
Author

fxkrait commented May 29, 2023

So yeah, I'm down with the current PR, where you stick the static color in a variable (not a face), defcustom it, init it to nil, and then they can customize-variable it by hand if needed without a Choose menu (they'll figure it out) or they can setq it.

@elizagamedev
Copy link
Owner

Thanks for the PR! I think that there's a way to specify color or nil as a type using a composite type. I think it's done with choice, like this:

(choice (const :tag "Foreground color" nil) color)

But I'm not 100% sure.

@fxkrait
Copy link
Author

fxkrait commented Oct 10, 2023

Thanks for the response, and for the pointer towards using composite types. The composite types do indeed seem to be the way forward here. Apologies for the delay on my end in terms of a response.


Composite-Type Behavior (looks good):

1) nil init:

  • So because a color cannot be set/init-ed to nil while also allowing the color picker to work, with the composite type, it can be init-ed to nil, andcustomize-variable will display the nil sub-type per the composite type's definition:

image

2) color selection:

  • And under the Choice dropdown, they can later select Color, and use the color picker to pick a color, and switch to begin using the color sub-type:

image

🙋 However, let me know what you think about the label for the nil sub-type that is shown under customize-variable. 🙋 (click here to expand)

Summary:

- I'm thinking of going with option 3, with a rename of Power Mode Streak Particle Color to Power Mode Streak Custom Particle Color: see: https://github.com/elizagamedev/power-mode.el/compare/main...fxkrait:power-mode.el:setParticleColor

Expositions:

  • I listed the example you gave under the header 1, and added two more cases under 2, and 3. A lot of text lies below, it's as short as I can make it, with saying what I want to say. 🙀
  • I'm thinking regardless, rename Power Mode Streak Particle Color to something indicating more that the variable is for a optional custom color such as Power Mode Streak Custom Particle Color to further clarify the existence of nil, in that it means no custom color has been set, and thus there is a default color being used by definition. Having the color set to nil, yet the particle colors appear per the foreground color, doesn't seem to be too correct per the previous variable name I put.
  • I think 3 seems best (especially after variable rename per above). It's simple, fits, and builds off of the existing type description ("If nil it uses the foreground text color."), which I would probably also rename to "If nil the foreground text color is used". I think having a explicit choice called "nil" sort of makes sense per the type description, and per the context of the "Choice" dropdown. So you're "choosing" nil as the "choice", and what nil does is listed in the type description below.
  • 2 also seems alright, as like 3, it states clearly that you are still selecting the explicit choice of nil, and then it's followed by a further description of what effect that choice causes, if they don't want to read the type description to see what the effect of choosing nil does. But the label is slightly redundant, as compared to 3, if we assume people will read the type description.
  • I don't like that for 1, it doesn't explicitly call out nil, which is listed in the type description, and the value that is being set. But I can see how 1 does make sense in a way, it's just abstracting out the nil part. This option does make more sense without the variable rename.
  • (Note: A space is added to the end of tags so that the text fits correctly: ")

      Example for: no space added, border very close to last letter: (:tag "Foreground color" nil

1: :tag "Foreground color " nil (example given)

2: :tag "nil: Use foreground text color " nil

3: No tag, just a nil: const nil

(defcustom power-mode-streak-particle-color nil "Streak particle color. If nil it uses the foreground text color." :type '(choice (const :tag "Foreground color" nil) color) :group 'power)

nil init:

image

Choice dropdown:
image

(defcustom power-mode-streak-particle-color nil "Streak particle color. If nil it uses the foreground text color." :type '(choice (const :tag "nil: Use foreground text color" nil) color) :group 'power)

nil init:
image

Choice dropdown:

image

(defcustom power-mode-streak-custom-particle-color nil "User-defined streak particle color. If nil the foreground text color is used." :type '(choice color (const nil)) :group 'power)

nil init:

image

Choice dropdown:

image

FXKrait added 3 commits October 12, 2023 00:27
…g-babel code block incorrectly told me to add an additional parenthesis to balance, in the .el it indicates parethesis balancing correctly. Undoing what babel told me, because it's false, breaks things.
@fxkrait
Copy link
Author

fxkrait commented Oct 13, 2023

This is ready for review:

Tested with

(use-package power-mode
  :ensure t
  :straight  (:host github :repo "elizagamedev/power-mode.el"  :fork (:host github :repo "fxkrait/power-mode.el" :branch "setParticleColor")))

And then a:

#+begin_src emacs-lisp :tangle yes
(power-mode)
(setq power-mode-streak-static-particle-color "turquoise")
#+end_src

And it works:

image


I removed the 2nd, additional, incorrectly added parenthesis I had put in from 47e0b5b, because I'm 99.99% sure there's a bug in elisp org-mode source blocks parenthesis highlighting.

With current main branch, it says a parenthesis is missing:

image

If you remove this balanced portion:

      (when (< 0 (current-column))
        (backward-char))

It then highlights things correctly, and doesn't tell you to incorrectly add an additional parenthesis.

image

In the .el I didn't see anything like the above, it's just in the org-source-blocks, which is what I was working in, it can have issues highlighting, thus why I incorrectly added an additional parenthesis in that prior commit.

@fxkrait
Copy link
Author

fxkrait commented Jan 26, 2024

@elizagamedev bump for review

Input:

(use-package power-mode
  :ensure t
  :straight  (:host github :repo "elizagamedev/power-mode.el"  :fork (:host github :repo "fxkrait/power-mode.el" :branch "setParticleColor")))

(setq power-mode-streak-static-particle-color "turquoise")
(setq power-mode-streak-shake-threshold nil)
(setq power-mode-streak-combo-threshold 1)
(setq power-mode-streak-particle-threshold 1)
(power-mode)

Output (turquoise):
2024-01-25_18-01-1706235250

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.

2 participants