Add recipe for darkman.el

[grtcdr]

Brief summary of what the package does

darkman.el is a package that provides seamless integration between Darkman and Emacs. Darkman is usually configured manually, via shell scripts, to do specific things (mostly theme changes) according to the time of day. This package on the other hand directly interacts with the Darkman D-Bus service to automate this process.

Direct link to the package repository

https://github.com/grtcdr/darkman.el

Your association with the package

I'm the author.

Checklist

• The package is released under a GPL-Compatible Free Software License
• I've read CONTRIBUTING.org
• I've used the latest version of package-lint to check for packaging issues, and addressed its feedback
• My elisp byte-compiles cleanly
• M-x checkdoc is happy with my docstrings
• I've built and installed the package using the instructions in CONTRIBUTING.org

[progfolio]

Some notes:

(defcustom darkman-switch-themes-silently t
  "Do not print the new mode and theme to the echo area."

This docstring should mention the general condition under which the behavior applies.
e.g.

(defcustom darkman-switch-themes-silently t
  "When non-nil, suppress theme loading messages."

Consider a better name/docstring for darkman-get.
I suggest naming the function after what it returns.
Checking for interactive calls via called-interatively-p is also discouraged (see its docstring for an explanation). It's generally better to accept a parameter which the interactive spec can make use of. e.g.

(defun darkman-current-mode (&optional message)
  "Return the Darkman service's mode.
When MESSAGE is non-nil, etc..."
  (interactive (list t))
  ;;etc

The same naming advice applies to darkman-set (perhaps darkman-set-mode).
Instead of using a cond, you can use if or or in your mode variable guard:

(defun darkman-set-mode (mode)
  "Set the mode of the Darkman service to MODE.
MODE can be ‘light’ or ‘dark’."
  (dbus-set-property :session
                     darkman--dbus-service
                     darkman--dbus-path
                     darkman--dbus-interface
                     "Mode" (if (member mode '(dark light))
                                (symbol-name mode)
                              (darkman--invalid-mode-error mode))))

Instead of writing a custom lookup function (darkman--lookup-theme), it may be better to
change the data structure which configure the modes. For example, using a simple alist of the form: ((MODE . theme)...) would allow you to use (alist-get mode darkman-themes).

Instead of autoloading the code which adds to server-after-make-frame-hook, it would be more polite to wrap that configuration in a function which users can call if they so choose.

[grtcdr]

Hi @progfolio!

Thanks for the suggestions and code examples.

I've implemented every one of them except for changing the current data structure as that will break compatibility and cause more trouble than it's worth.

[progfolio]

I've implemented every one of them except for changing the current data structure as that will break compatibility and cause more trouble than it's worth.

How about using plist-get directly? e.g.

(defun darkman--event-handler (interface property value)
  "Callback function for handling a change in mode.
INTERFACE is the name of the interface that is the target of the event.
PROPERTY is the property that is modified by the event.
VALUE is the new value of PROPERTY."
  (when-let (((string-equal interface darkman--dbus-service))
             ((string-equal property "Mode"))
             (mode (car value))
             (theme (plist-get (intern (concat ":" mode)) darkman-themes)))
    (unless darkman-switch-themes-silently
      (message (format "Darkman switched to %s mode, switching to %s theme."
                       mode theme)))
    (load-theme theme)))

[grtcdr]

darkman--lookup-theme is referenced more than once ¹, we'd be repeating ourselves if we don't place that logic in a separate function, though I did refactor it to use when-let*.

Footnotes

1. Here and here ↩

[riscy]

Thanks both. This is looking solid and I'm not seeing any blockers. The below are just for your reference:

- darkman.el#L98: No `format` required; `error` takes an f-string
- darkman.el#L102: No `format` required; `error` takes an f-string
- darkman.el#L121: No `format` required; `message` takes an f-string

[grtcdr]

Thank you @riscy! I wasn't aware of those warnings (neither the byte compiler nor package-lint show me such a message) but they should be gone now.

And thank you @progfolio for the detailed and helpful review.