Re: multi head porting

[Owen Taylor <otaylor redhat com>]

Hans Breuer <hans breuer org> writes:

> > * (later) Consider moving global variables into the GdkDisplay and GdkScreen
> >   structures even for ports that only support one display and one screen,
> >   for cleanliness.
> >
> This (later) is already done/required, e.g. display->queued_events
> which appears to be an indirection too much for single head 
> versions ...

Basically, what happened here is that the handling of the event
queue was mostly in the  independent code, but partially
in the backends

To keep the code shared, it was necessary to make it per-display
in the backend independent code.

So, if the code to handle something is backend-independent, then
yes, the singlehead ports need to deal with it being per-display.
But that's usually as simple as a gdk_get_default_display() call.

> </Re: Ports and multihead>
> 
> >I'm not sure that minimizing the required changes to the 
> >backend code is really the right goal. I think, instead,
> >the right goal is minimizing the total amount of backend
> >code.
> >
> I'm not sure if it is the right way to go to continously
> break other backends for 'no good reason'. At some point
> in time there even was an almost working Nano X port.
> Currently AFAIK linux-fb and direct-fb are significant
> falling behind ...
> 

I'm no happier than anybody else when the backends keep breaking,
and when I can fix up the problems, I do do so.

The trouble is, a GDK backend just isn't a simple thing; 
GDK is a complex library, with complex interactions wiht
GTK+. Someone has to really care about it.

A good portion of the difficulty does come about from the fact that
the GDK <=> GDK backend interface is an ugly mess. If I
was doing it again from scratch, I'd probably think about
changing things so that all shared function implementations
lived in GDK, and a backend author implemented:

 - gdk_open_display()
 - A whole bunch of virtual functions in subclasses
   of GdkDisplay, GdkScreen, GdkColormap, etc.

But fixing this would be a huge job, and cause a lot of intermediate
pain, and just isn't worth it for the forseeable future.

Even if we had a clean setup like that, I don't see trying
to freeze the GDK<=>backend API as being reasonable ... 
the only way to do that would be not to add features to GDK.

> With almost the same amount of work I could have added
> the GdkDisplay* / GdkScreen* parameters [I even have done
> it somewhere - where I don't think it's right - to not 
> break the common gdk api (and the X11 backend, which 
> I can't compile)]

If you know of a single place where the GDK API/ABI broke, or
if the X11 backend doesn't compile for you, please file
a bug report.

> >If we take your approach of:
> >
> >==
> >void
> >gdk_display_beep (GdkDisplay * display)
> >{
> >  g_return_if_fail (GDK_IS_DISPLAY (display));
> >
> >  gdk_beep ();
> >}
> >===
> >
> >Then we need to implement:
> >
> > gdk_display_beep:  in gdk/x11/ and gdk/
> > gdk_beep:          in gdk/x11/ and gdk/win32
> >
> Not really. All the working ports allready _have_
> gdk_beep (). The could be compiled with minimized
> changes after applying my patch. For future addition
> your suggestions is obviously the right way to go.

The last thing that the GDK needs is more inconsistency between the
way different things are done.

We are talking about a few dozen functions. I could do the fixups for
all three backends in a couple of hours of work. Compared to harmful
leaving excess duplication all over the code, this is a tiny amount of
work.
 
> >> With this goal in mind there 
> >> are some places where it could get huge improvements if the
> >> multi-head api isn't inscribed in stone yet :
> >> 
> >> gdk_colormap_get_screen()
> >> Only the multi-head version needs to store a screen with
> >> every colormap. Single Head version would return the 
> >> default screen. [The GdkScreen* field from the GdkColormap 
> >> struct could be removed and the multi-head version would
> >> maintain it's own hashlist to remember the Colormap/Screen
> >> relations.]
> >> 
> >> gdk_visual_get_screen()
> >> Same logic as for gdk_colormap_get_screen(), removing
> >> GdkScreen* from _GdkVisual struct, whiches content needs to be 
> >> maintained by the backend.
> >
> >We could move the GdkScreen structure in to the X11 private
> >part ... I don't really see much of a difference though.
> >
> The point: Currently every backend needs to maintain information 
> for the common case, which isn't at all relevant at single-head.
> See: gdk/win32/gdkcolor-win32.c gdk/win32/gdkvisual-win32.c

The number of colormaps and visuals just isn't big enough to 
worry about 4 bytes extra per object... might be a waste of
50 bytes for the app.

> >> gdk_event_* ()
> >> Removing the display parameter from the gdk_event_* () function
> >> again and determining it from the window given in _GdkEventAny
> >> would allow for multple event queues but did not enforce
> >> them. [They appear unnecessary on win32 even if it would support
> >> multi-head. And they are obviously unnecessary for any gdk port
> >> which is bound to single-head through real backend 'limitations'
> >> (linux-fb, directfb ?).]
> >
> >The hierarchy we have here is:
> >
> > * GdkDisplay ... a set of monitors on one system with an attached 
> >   keyboard and mouse.
> >
> > * GdkScreen - a separate virtual area of one or more monitors,
> >   with it's own set of colormaps and visuals.
> >
> > * gdk_screen_get_n_monitors() [etc]... find out how many monitors
> >   a screen is made
> >
> >To my knowledge, win32 can "only" do multiple monitors, so I
> >don't think there is ever going to be multiple screens or
> >multiple displays for win32.
>
> Thanks for the clarification. Could I have read it anywhere in the
> docs or on the mailing-list ?

 http://developer.gnome.org/doc/API/2.0/gdk/multihead.html

Has some of it. (but could still use more work)

> Anyway: If different Displays allow different depths, but
> different Monitors on a single Screen do not, the current
> model does not fit the windoze multiple-head definition 
> at all [as long as I've understood the MSDN docs].
> http://msdn.microsoft.com/library/default.asp?url=/library/en-us/gdi/monitor
> _82yb.asp

Well, it seems that you can paint onto all monitors as if
they were the color properties of the primary monitor, so
we don't *have* to expose the different depths.

And the idea that different parts of a single window have
different depths or that a window changes depths over time
isn't really representable in the GDK API. 

We can hide some magic behind gdk_draw_pixbuf(), etc, if we
want to improve some specific rendering operations.

> >But I do think that it  makes sense that if you have a separate
> >display ... a separate keyboard and mouse ... then you have a
> >separate event queue.
> >
> Yes. But we appear to agree that this is only possible with
> X Windows at the moment ?
> If so: removing the Display* parameter from the common 
> _gdk_event_*() API would at least save all the single headed
> backends an additional indirection.

It would force all the single headed backends to duplicate
the event queuing code that is currently shared; I don't
see this as an improvement.

> >I'm not sure what you mean by 'optimize' here. 
> > 
> > - Optimize work for backend authors. As I said above, I think
> >   the most important thing is to optimize on-going maintainence,
> >   rather than to optimize the amount of work that we have to
> >   do right now.
> >
> As noted above: every change required for every backend may break 
> it forever (or at least for a long time). I currently can't 
> imagine the next change which breaks all backends but having 
> some statement that we actually do care about least possible 
> impact on backend authors would be nice.

The only major structural change I could see doing would be
completely redoing the architecture to make ports easier to
maintain. But I don't think it would worth the effort in the
remotely near future.

I'm not going to say I care about the *least* possible impact
on backend author. I certainly do care about possible impact
in backend authors. But the priorization goes something like:

 * ABI and API compatibility
 * Providing a nice clean API for app devlopers.
 * Providing new features as needed
 * Keeping the code clean and maintainable
 * Minimizing code changes for backend authors
 * ...
 
[...]

> >> What do you think ?
> >> And if you think all the above doenn't make much sense: Is
> >> the patch in its current form ok to commit ?
> >
> >I'd really like to see something more like what I outlined
> >in my mail... move the non-multihead variants into the common 
> >code and only duplicate the multihead variants. 
> >(With non-multihead backends ignoring the screen/display 
> >arguments.)
> >
> Ignoring the screen/display arguments isn't at all possible
> at the moment, i.e. see gdk/gdkevents.c :
> 
> GList*
> _gdk_event_queue_find_first (GdkDisplay *display)
> {
>   GList *tmp_list = display->queued_events;
> ...
> 
> GList *
> _gdk_event_queue_append (GdkDisplay *display,
> 			 GdkEvent   *event)
> {
>   display->queued_tail = g_list_append (display->queued_tail, event);
> ...
> 
> 
> So there should be no commonly reusable <GdkScreen/GdkDisplay>Single
> implementation ?
> And there needs to be an event queue indirection for all the
> backends though only one does sometimes need it ?

If a few extra indirections per event is going to slow down the Win32
port in any measurable way...  please tell me what you used on it,
because I need some of that stuff to speed up the X version :-)

I do have some concerns about the internals of the X backend where we
end up calling gdk_drawable_get_screen() a lot, which is a virtualized
function call... I suspect that there might be a fraction of a percent
of slowdown from that. 

But that's not going to be a concern for the win32 backend.
 
> If the above arguments still don't make my point clear do you
> think a patch implementing the goals (but possibly breaking 
> the X11 backend) could help ?

I don't want to come across here as being inflexible, but I'm not
sure I can avoid it ... I'm not going to accept a patch that doesn't
move gdk_beep() to gdk/ and make it a wrapper for gdk_display_beep().

Within that constraint, I'm happy to look at patches that break the
X11 backend, though I wouldn't want to commit one without having a
plan for getting X11 compiling again within a short period of time.

Regards, 
                                        Owen