Re: EWMH package, please review.

[Kim F. Storm]

"Jan D." <address@hidden> writes:

> Hello.
> 
> I've made a small package to manipulate extended window manager hints.
> Since Elisp is not my biggest strength, I'd like some comments.  

Ok, you asked for it :-)

>                                                                  I
> don't know if this is useful enough to be included in Emacs, so I'd
> like your views on that also.

I don't know.  Are (all of) those hints something an ordinary user
would typically toggle interactively?


> ;;; Code:
> 
> (defun x-ewmh-send (arg hint frame &optional hint2)
>   "Send a client message to change an extended window manager hint.
> 
> FRAME may be nil to use the selected frame.
> If ARG is poitive, add HINT.
> If ARG is negative, remove HINT.
> Otherwise toggle HINT."
> 
>   (let* ((eff-arg (if (null arg) 0 (prefix-numeric-value arg)))

This is simpler:

  (setq arg (if (null arg) 0 (prefix-numeric-value arg)))
  (let ((action ...use ARG instead of EFF-ARG...))

IMO, it is somewhat unusual to let a generic, non-interactive function
assume that it is always invoked by an interactive function.  I would
normally let the callers do that (possibly using an auxiliary
function), but in your case I would just change the DOC string to say
that ARG is supposed to be a raw prefix argument.


>        (action (cond ((= eff-arg 0) 2)        ;; Toggle
>                      ((> eff-arg 0) 1)        ;; Add
>                      (t 0))))                 ;; Remove
>     (x-send-client-message nil 0 frame "_NET_WM_STATE" 32
>                          (list action
>                                hint 
>                                (if hint2 hint2 0)

simpler:  (or hint2 0)

>                                0))))
> 
> 
> (defun x-ewmh-fullscreen (&optional arg frame)
>   "Toggle FRAME fullscreen hint using extended window manager hints (EWMH).

Since this is an interactive function, you should describe the normal
interactive function in the "one-liner", which in your case would mean
not referring to the FRAME argument.

> 
> If FRAME is not given, the selected frame is used.
> If ARG is poitive, add fullscreen hint.
> If ARG is negative, remove fullscreen hint.

Since this is an interactive function, you would normally say that
using a positive command prefix would add the hint and a negative
prefix would remove the hint, while no prefix toggles the hint.

But it might be clearer if you say that a C-u prefix (write
\\[universal-argument] in the doc string) adds the hint, while
a M-- prefix (write \\[negative-argument] removes the hint.

> Otherwise toggle fullscreen hint.

I suggest you write something like this in the doc string:

  "Toggle EWMH fullscreen hint of selected frame.

With \\[universal-argument] prefix arg, add fullscreen hint, and
with \\[negative-argument] prefix arg, remove fullscreen hint.

When called from a Lisp program, optional second arg FRAME specifies the
frame on which to apply the fullscreen hint."

> 
> If fullscreen doesn't work with your window manager, try
> `x-ewmh-horz-and-vert'.
> 
> NOTE:  If the window manager does not support EWMH, this does nothing."
>   (interactive "P")
>   (x-ewmh-send arg "_NET_WM_STATE_FULLSCREEN" frame))
> 


> 
> (defun x-ewmh-maximized_vert (&optional arg frame)

x-ewmh-maximized-vertical  (don't use underscore and write name in full)

> 
> (defun x-ewmh-maximized_horz (&optional arg frame)

x-ewmh-maximized-horizontal  (ditto)

> 
> (defun x-ewmh-horz-and-vert (&optional arg frame)

x-ewmh-horizontal-and-vertical  (ditto)


Maybe in general, the functions should be named

        x-ewmh-toggle-...

to emphasize that their default operation is to toggle the hint.

-- 
Kim F. Storm <address@hidden> http://www.cua.dk