fwiw, I think it's pretty ugly and non-object-oriented to make the methods on display not take a display object.

Replacing meta_displays_list() with meta_display_get() probably does make sense, assuming we'll never want to do #1, though afaik #1 would work fine if some means of telling metacity to connect to multiple displays were added. Not that I can come up with what those means would be, or why you would want to do this.
It is in fact kind of useless.

I do think though that meta_displays_list(), or a hypothetical meta_display_get(), should only be used in contexts where the display is unknown (e.g. when handling a notification from gconf), and should never be used e.g. inside a method on a display. Think about in Java:

  class Display {
    private static Display theDisplay;

    private List<Window> windows;

    static List<Window> listWindows() {
       return theDisplay.windows;
    }
  }

that is just bizarre, it should be:

  class Display {
     private List<Window> windows;

     List<Window> listWindows() {
       return windows;
     }
  }

(In reply to comment #1)
> fwiw, I think it's pretty ugly and non-object-oriented to make the methods on
> display not take a display object.

It does mean that in a number of places we call meta_display_list (or meta_display_get in happy fun singleton world) only in order to be able to pass the result in as the first parameter of various meta_display_* functions later in the function; this is useless extra complexity and extra code if the value is predictable. (I agree it's not terribly OO, but singleton objects rather break the OO pattern anyway.)

There are only 10 calls to meta_displays_list(), all in places where there really is no known display (i.e. in truly global contexts).

I don't know. All my programming experience would say "don't make this change"

It is perhaps a bit of an indentation-style issue though, challenging to build an irrefutable argument either way.

I guess I would say the only reason to do it your way is that it saves a (small) amount of typing and work up front, but since the work is already done to be completely OO and even multiple-display-safe, why undo that? Seems like a downgrade.

It is a downgrade, insofar as it removes abilities we don't use and have no plans to. I'm not trying to save us work we've already done-- and new patches, like the one Alex posted recently, sometimes need to add new calls to meta_display_list() after all, so not *all* the work is already done.

Rather, I'm trying to reduce the complexity of the existing code, giving us less to maintain in future: if we replace meta_displays_list() with meta_display_get(), we save one outermost for loop in every one of those ten places; if we also then stop passing MetaDisplay*s around (which is really a second possible refactoring made possible by the first) we save ourselves one parameter every time we call meta_display_*. We could do both of those, or just the first, or neither.

On the other hand, perhaps neither would be good: it's mature code, so probably it's best not to mess with it even though I think it's pretty clear that the paths through it would be identical with and without this change.

I'm fine with marking this WONTFIX if your spidey-sense is tingling against it, though: I've been looking for places to tidy up the code recently, and this was something that jumped out at me, but if you'd rather keep it as it is that's fine.

I don't have a strong view on get vs. list, that seems toss-up-ish. I think calling get() (or just using a global var directly) in the display methods, instead of passing in the display object, is pretty fugly though.

Oh, okay, yay. I know I said I didn't mind either way, but I didn't mind either way more about that part. :) I'll see about a patch and we'll see how it goes.

I think the current handling of displays is probably more similar to how gtk+ and X are coded than changing it to a singleton.  I don't think gtk+ deals with multiple X servers in most cases either, but I think keeping the style to be familiar to other coders (there's a lot more people familiar with gtk+ than metacity, and gtk+-hackers have often been known to help out with metacity issues here and there) is more of a win.

Also even when I do singletons in C++, there's some global function to get 'the instance' and then I call the functions on that instance...thus meaning the object is passed to the function.  So, I think I agree with Havoc here as far as passing the display objects goes.  Now, if you wanted to change meta_display_list() to meta_display_get() and added some other cleanups to make it clear that there would ever only be one display, I probably wouldn't have an opinion.  But I agree with Havoc that it seems to make more sense to pass the display object to various display functions.

Anyway, that's my $0.02.

Created attachment 107506 [details] [review]
Replace meta_displays_list with meta_display_get (first draft)

Here's a patch doing that, then.

There is actually one place where it's not clear that the code paths before and after are identical: it turns out that the old display list didn't always have exactly one element in it.  When Metacity's starting up, there was nothing in the list.  This is the same now as meta_display_get() returning NULL.  Of course before we'd iterate over the list, and if there was nothing in it we'd do nothing, whereas now if we just charge ahead we'll end up dereffing a null pointer.  So in the cases where we call meta_display_get() where we might be called before the display is initialised, we need to check whether meta_display_get() returns null and not use the result if so.  After some checking I believe there is only one such case.

Oops, I left in a FIXME towards the end which I meant to clear up.  The current Metacity trunk is ambiguous on casual reading whether it terminates in meta_display_close() at

  if (all_displays == NULL)
    {
      meta_verbose ("Last display closed, exiting\n");
      meta_quit (META_EXIT_SUCCESS);
    }

or by reaching the end of main(), and why.  (I think it reaches the end of main(), otherwise session management would be broken, but I need to check and remove the one which isn't in use.)

Oh, my bad.  meta_quit() tells the main loop to quit and returns: it's not some kind of exit() type affair.  So this should be all good.

Created attachment 107968 [details] [review]
Second draft

I think this is ready to be checked in, and I will later, but I want to go to bed.

Yay.  FIXED.

http://svn.gnome.org/viewvc/metacity?rev=3663&view=rev