Created attachment 76935 [details] [review]
Refactor thrice-duplicated queue code in window.c

Attempt at doing this, as written on a Greyhound bus between Pittsburgh and Cleveland. Comments and suggestions welcome.

Overall, I think it looks good other than one big question (the first one below).  I've got lots of other questions and comments too, but they're all about tiny things.

Is someone going to come along and think it's okay to add the following code:
  meta_window_unqueue (window,
                       META_QUEUE_UPDATE_ICON | META_QUEUE_MOVE_RESIZE);
It looks perfectly innocent and similar to other constructs in the code (MetaFrameFlags and MetaMenuOp handling, for example), but would break rather badly.  Possibly the only case where we don't do something like this is with MetaClientType handling, but it simply doesn't make sense for a window to simultaneously be treated as multiple types whereas for MetaQueueType it does make sense to add or remove a window from multiple queues (particularly inside of meta_window_new_with_attrs() or meta_window_free()).


Does it make sense to move the definitions of queue_idle, queue_pending, meta_window_queue_names, window_queue_idle_priority, and window_queue_idle_handler so that they are all in one place?


+    /* if withdrawn = TRUE then unmanaging should also be TRUE,
+     * really.
+     */

Yes, window->withdrawn is only set in one place, which calls meta_window_free immediately, which sets window->unmanaging.  So let's just change the if to checking window->unmanaging.  That'll make it a bit cleaner.


+  queue_pending[queue] = g_slist_prepend (queue_pending[queue],
+                                                 window);

the second line should either be merged with the first, or the w in window should line up with the q in queue_pending following the left parenthesis.


+#if 0
 static void
 meta_window_unqueue_move_resize (MetaWindow *window)
 {
@@ -3467,7 +3488,9 @@ meta_window_unqueue_move_resize (MetaWin
       move_resize_idle = 0;
     }
 }
+#endif

Why ifdef it out instead of just removing it?


+typedef enum {
+  META_WINDOW_QUEUE_CALC_SHOWING,
+  META_WINDOW_QUEUE_MOVE_RESIZE,
+  META_WINDOW_QUEUE_UPDATE_ICON,
+  META_WINDOW_QUEUE_LAST,
+} MetaWindowQueueType;

I'd prefer to have WINDOW/Window removed from the names.  Also, since you assume the first three have values 0, 1, and 2 (in order to use them as array indices), it'd be nice to be explicit about that similar to the way it is done in MetaClientType.  Also, perhaps a name like META_QUEUE_NUM_QUEUES is more clear than META_QUEUE_LAST (which makes me think, it's the last queue?  which one is that?)


+#define META_WINDOW_IS_IN_QUEUE(window, queue) (window->is_in_queues & (1<<queue))
+#define META_WINDOW_MARK_QUEUED(window, queue) window->is_in_queues |= 1<<queue
+#define META_WINDOW_MARK_UNQUEUED(window, queue) window->is_in_queues &= ~(1<<queue)

Spacing issues.


+/* And for convenience's sake: */
+#define meta_window_queue_calc_showing(window) \
+  meta_window_queue(window, META_WINDOW_QUEUE_CALC_SHOWING)
+#define meta_window_queue_move_resize(window) \
+  meta_window_queue(window, META_WINDOW_QUEUE_MOVE_RESIZE)
+#define meta_window_queue_update_icon(window) \
+  meta_window_queue(window, META_WINDOW_QUEUE_UPDATE_ICON)

No, I think that would only serve to confuse.  It may make the patch bigger to change these, but it'd be cleaner.


+  /* Are we in the various queues? (Bitfield: see META_WINDOW_IS_IN_QUEUE) */
+  guint is_in_queues;

Shouldn't that be
  guint is_in_queues : 3;
(or even better, is "guint is_in_queues : META_QUEUE_NUM_QUEUES;" allowed)?  Also, on a related note, I think we need to pull the struts member out of the middle of the bitfields so that the bitfields can be compressed.  As another side note, it'd be interesting to find out whether these bitfields actually help performance (reducing memory costs a little bit and making it more likely things are in cache) or hurt (more code to access the relevant fields).

(In reply to comment #2)
> Is someone going to come along and think it's okay to add the following code:
>   meta_window_unqueue (window,
>                        META_QUEUE_UPDATE_ICON | META_QUEUE_MOVE_RESIZE);
> It looks perfectly innocent and similar to other constructs in the code
> (MetaFrameFlags and MetaMenuOp handling, for example), but would break rather
> badly.  Possibly the only case where we don't do something like this is with
> MetaClientType handling, but it simply doesn't make sense for a window to
> simultaneously be treated as multiple types whereas for MetaQueueType it does
> make sense to add or remove a window from multiple queues (particularly inside
> of meta_window_new_with_attrs() or meta_window_free()).

They are possibly going to do that, yes, and I hadn't really considered it. The trouble is that I don't see how these values can be both ORable bitfields *and*    array indexes, and we need them to be array indexes. Maybe renaming them will discourage people from treating them as bitfields, or maybe we just need to put a loud and obvious comment above the function prototype.

> Why ifdef it out instead of just removing it?

I think I meant to remove it (it was a while ago now).

> I'd prefer to have WINDOW/Window removed from the names.  Also, since you
> assume the first three have values 0, 1, and 2 (in order to use them as array
> indices), it'd be nice to be explicit about that similar to the way it is done
> in MetaClientType.  Also, perhaps a name like META_QUEUE_NUM_QUEUES is more
> clear than META_QUEUE_LAST (which makes me think, it's the last queue?  which
> one is that?)

All good ideas.

> Also, on a related note, I think we need to pull the struts member out of the
> middle of the bitfields so that the bitfields can be compressed.  As another
> side note, it'd be interesting to find out whether these bitfields actually
> help performance (reducing memory costs a little bit and making it more likely
> things are in cache) or hurt (more code to access the relevant fields).

I'll raise a separate bug.

Thanks for your feedback!

Separate bugs are bug 450271 and bug 450272. This was closed:

http://svn.gnome.org/viewcvs/metacity?view=revision&revision=3236

FIXED.