Re: [STUMP] removed duplicate code from group, screen and window.lisp

[Shawn]

"Patrick Pippen" <address@hidden> writes:

Hi Patrick,

Your patch was not quite what I meant. I left some comments below.

> +(defmacro set-list (l &body body)
> +  `(setf ,l ,@body))

This doesn't actually do anything. Why use set-list over setf?

> +(defmacro push-list (l &body body)
> +  `(push ,l ,@body))

Same here. Push already pushes an element to a list. Why use push-list
instead of push?

> +(defun move-to-head (&key screen group window)
> +  "Moves a screen, group, or window to head based 
> +on condition of keyword arguments :screen :group :window and it defaults to 
> nil if not supplied."
> +  (if (and (not (null screen)) (null group) (null window))

(not (null screen)) can be simplified to just screen.

> +      (progn
> +     (set-list *screen-list* (remove screen *screen-list*))
> +     (push-list screen *screen-list*))
> +    (cond
> +     ((and (not (null screen)) (not (null group)) (null window))
> +      (set-list (screen-groups screen) (delete group (screen-groups screen)))
> +      (push-list group (screen-groups screen)))
> +     ((and (null screen) (not (null group)) (not (null window)))
> +      (set-list (group-windows group) (delete window (group-windows group)))
> +      (push-list window (group-windows group))))))

The problem with this function is that it actually makes things more
complicated. Instead of having 3 functions that use similar code to do
a very similar thing, you have 1 function with 3 copies of of similar
code that do similar things.

What I was thinking was either a macro like this:

(defmacro move-to-head (list elt)
  "Move the specified element in in LIST to the head of the list."
  `(progn
     (setf ,list (remove ,elt ,list))
     (push ,elt ,list)))

which you could use this way: (move-to-head *screen-list* screen) But
maybe you'd want something more specialized such as a macro that
generates those the move-*-to-head functions so you could replace them
with:

(define-move-to-head screen *screen-list*)

However, having looked at those three functions they're actually
different enough that it doesn't really make much sense to have such a
macro. I only included it to help you see the direction I was thinking
in.

-Shawn