Created attachment 845 [details] [review]
PATCH: first revision of Plug/Socket for win32

[ Please attach patches unzipped. We still have 25gig or
  so of disk space free on bugzilla.gnome.org. A few 
  patches won't kill us. :-) ]

My objection here has never been to having Plug/Socket
implementations on other platforms.

My object has always been that making the details of the
XEMBED implementation cross platform makes no sense,
and I still believe this is the case.

An organization of:

#ifdef GDK_WINDOWING_X11
  XSendEvent()
#elif GDK_WINDOWING_Win32
  PostMessage()
#endif

Makes the code awfully ugly and unmaintainable, and is
just wrong. 

I understand that you want to reuse the XEMBED protocol
on Windows, and that sharing the "driver" code for this
would make maintainence of the Windows PLug/Socket code
easier, but I think long term entangling the 
windows/unix code in this way won't help us, and certainly
won't help people adding additional platforms.

What I'd suggest for code organization is that we have:

 gtkplug-private.h
 gtkplug.c
 gtkplug-local.c
 gtkplug-x11.c
 gtkplug-win32.c

 gtksocket-private.h
 gtksocket.c 
 gtksocket-local.c
 gtksocket-x11.c
 gtksocket-win32.c

Where gtkplug/socket-local.c is a stubbed implementation that
can be used whe only inprocess plug/socket embedding.

What the split is between gtkplug.c and gtkplug-x11.c is obviously
the tricky part. One guideline should be that all the code
for handling the local case should be in gtkplug.c, and 
x11/gdkx.h should only be included by gtkplug-x11.c.

IMO the simpliest way to detect the cross platform needs
is to first do an almost working #ifdefed version.

My current patch (not much different to the previous one)
appears to suggest some additional Gdk API, like:

void
gdk_event_send (GdkWindow *destination,
                GdkEvent  *event,
                guint      timeout, /* ?? */
                gboolean   synchron);

GdkNativeWindow
gdk_window_get_native (GdkWindow *window);


and suddendly most of the platform specific code
from gtk<plug|socket>.c could vanish. The biggest
remaining part would be the filter_funcs, which could
live in gtkxembed-<x11|win32>.c. Splitting the code
in many independent files seems to be a little overkill ...

But I'm not sure if I understand enough of the XWindows
protocol and obviously this would only solve part of
the 'cross-platform-ness' because only two platforms
get served :)

(updated, un-zipped patch - without the above 
 suggestions implemented - follows)
 

Created attachment 5917 [details] [review]
updated (and unzipped) patch

Not going to get to doing anything on this until after 2.0.0.

Moving non-critical or hard to fix bugs to 2.0.2

Move open bugs from milestones 2.0.[012] -- > 2.0.3, since 2.0.2 is already out.

Does the recent setting of "2.6 API freeze" mean an updated patch would be
reconsidered - or would it be again just wasting time ?

Milestones generally indicate the earliest version we could include the fix.
No guarantees, in particular for Xembed related bugs, where owen would be the
only available reviewer...

Owen and Hans, do you have any new ideas regarding this? In the evolution
porting process, I am currently looking at libbonoboui. It uses gtkplug/socket,
and I am wondering whether to just leave out those parts of libbonoboui on Win32
(with unknown consequences), or add Hans's implementation to gtk and see what
happens.

I was told that the gtkplug/socket stuff is essential to libbonoboui and the
apps that use it, so I guess I'll start working on this soonish. I'll do
something like Owen describes in comment #2. This would be for HEAD only, right?

Last time I looked it appeared much simpler to have the platform 
specific stuff in platform specific file. I didn't continue on
this for two reasons :

1) None of the programs I'm interested in did make use of GtkPlug/Socket
2) I don't follow Owen's argument of implementing based on platforms
common embedding API (which would probably mean ActiveX for win32)
cause to me the only use for GtkPlug/Socket will be between two 
programs depending on Gtk. Most ActiveX Controls I'm aware of depend 
on the MFC. Having two huge toolkits in one "application" does not 
look like anyone would be interested ;)

BTW: is there any particular reason for this bug to be visible only 
for GNOME Hackers - or was this changed by accident?

1) Ditto, until now ;-)

I'll see what I can do.

GNOME Hackers only, eh, must have been by accident.

One thing you might want to investigate, Tor, is whether Evolution actually
is using *interprocess* embedding at this point. If not, then you might
only need to implement the -local files described in my comment #2.

XEMBED really does depend upon various concepts that are, to my knowledge,
X specific, like:

 - Reparenting
 - Redirection of mapping/resizing. (The embedder in XEMBED receives
   the XResizeWindow requests from the client and can choose whether
   it obeys them or not. In fact, the embedder and client interact
   just like window manager and application.)

So, while just copying XEMBED may give you a partially working initial
solution, I don't think it is going to hold up long term. The Win32
protocol needs the freedom to evolve on its own. That's why I don't
think we can just implement the XEMBED protocol and hide the X calls
as Hans suggests in comment #3.

Actually, reparenting seems to work fine on Win32. (The docs for SetParent()
claim that "The new parent window and the child window must belong to the same
application", but that doesn't seem to be true. Sheesh.)

After more or less intense hacking for some time, testsocket basically works...
Some rough edges still, for instance the "Passive Child" plug retains its window
management decorations (and its size is thus miscalculated). And the "Local
Passive Child" plug has its geometry miscalculated or something. See attached
screenshot...

I don't even do anything special for redirection of mapping/resizing, still
things mostly seems to work, at least in testsocket. Odd.

I rearranged the source code as follows, following Owen's ideas above:

- gtk{socket,plug}.c: backend-independent code
- gtk{socket,plug}-{x11,win32}.c: backend code
- gtk{socket,plug}private.h: declarations of backend functions, and of private
functions for backend's use located in the backend-independent files.
- gtkwin32embed.{ch} - minimal Windows message -based "protocol"

How does that seem, are the file names well chosen? Would it be OK to commit
something like this to HEAD? What about 2.6? 

Will attach a diff, too, in a moment.

Created attachment 38770 [details]
Screenshot

Four embedded plugs (left to right): Passive, Local Active, Active, Local
Passive.

Created attachment 38819 [details] [review]
Current diff

For your amusement, here are my current diffs related to this. Please ignore
the debugging printouts...

Created attachment 38954 [details] [review]
Current diff

Current diff after latest changes to gtk. No extra debugging output any longer.

Created attachment 39276 [details] [review]
Current diff

Hi Owen,
at least I'd like to get this applied to HEAD instead of letting 
it rot a few years ;) What's holding it now?
Remaining issues like using __FUNCTION__ unconditionally can be
resolved from cvs than ...

Thanks,
    Hans

(About __FUNCTION__) Those are obviously just temporary debugging printouts,
didn't bother to remove before running the diff.

In general, the approach taken here looks good.

(I really find that debugging printfs left-over in a patch make it
a lot harder to review... if you have printfs you want to leave 
in permanently you can use GTK_NOTE(PLUGSOCKET, ... that at least
has the benefit of being able to skip it quickly visually)

General comments:

 - I think it would be cleaner if *all* public functions went
   into gtkplug/socket.[ch], so make gtk_socket_get_id() a wrapper.

 - I'd like to see a naming convention for the backend-specific
   plug/socket functions. Perhaps _gtk_plug_windowing_<blah>
   in analogy to _gtk_windowing_window_get_offsets, and so forth.
   Right now it's hard to guess what file something lives in
   by reading the code.

 - We need doc comments for all the _gtk_plug_* in gtkplug.c.
   If you write them to the best of your understanding, I can
   fix up.
   
 - Having doc comments for the _gtk_plug_windowing_* would also be
   nice. I'd suggest putting the doc comments for them in
   gtkplugprivate.h. 

 - 

Specific comments:

 - I'd rather not see scanf() usage in testsocket.c even in 
   getenv() block. In general, I dont' really understand the testsocket.c
   changes.

 - Have we thought about win64? Do window handles stay 32-bits there?
  
 - These changes look to break the linux-fb/direct-fb backends. 
   How hard would a stubbed out gtkplugstub.c be? Would it work
   for local-only?

 - I don't think the translation of XEMBED_MAPPED into an event is
   going to work ... the point of making it a property is that it
   can be set at any point, before or after the embedding starts.
 
 - Don't export _gtk_plug_symbols, just emit events by name

 - Why did GrabbedKey moeve to gtkplugprivate.h? (If it does
   move, it would need to be renamed to GtkGrabbedKey)

 - The implementation of _gtk_socket_size_request for Win32 
   looks wrong. What we want is the plug's minimum size,
   not it's current size. If you can't get that information 
   from the Win32 API, you'll have to pass it via the protocol.

 - _gtk_socket_active_change() is misnamed. I'd probably prefer
   it just as a pair of functions, though if you keep the
   current signature, I'd say _gtk_socket_change_activity().

 - gtk_socket_send_configure_notify() is used to implement particular
   requirements of the ICCCM for window-manager/client interaction.
   It's strange to me to use the same logic on Windows.

 - The push_message/pop_message stuff is just crying out for 
   documentation. 

I am now working on this again. Expect a new version modified according to your
comments today or tomorrow. I assume you mean signals when you talk about
"_gtk_plug_symbols" and "emit events"?

Created attachment 45579 [details] [review]
Current diff

Here is my current diff. It should address most of your issues. I didn't
include any patch to testsocket, will have to look more closely at it. I
commented out the  _gtk_socket_windowing_send_configure_event in
gtksocket-win32.c, and indeed it didn't seem to make any difference. Will
remove altogether.

I removed the debugging output, but will probably regret that as soon as I try
to solve the remaining issues... (for instance, the "blink" test in testsocket
behaves oddly sometimes).

I did not yet:
- consider Win64.
- try to implement the XEMBED_MAPPED X11 property some other way than messages.
Windows windows don't have X11-like properties, or at least not anything
comparable that one could receive change notification from. In what cases would
the property be set before the embedding starts?
- document the push/pop message stuff as I don't understand it at all.
- implement local-only stubs for the linux-fb or direct-db backends. Probably
would be trivial.

 * The main large-scale change I'd like to see now, is to see all
   the gtk_{plug,socket}_windowing functions changed to take a
   GtkPlug or GtkSocket as their first parameter rather than a
   random collection of functions. In most cases, this would allow
   getting rid of one or more of the current parameters.

 * I would also really like to see documentation of the protoocl
   you are using and how it differs from XEMBED. (You could put
   a text file into gtk+/docs/ or put it in a source code comment.)
   It's not clear, for example, to me what 
   GTK_WIN32_REQUEST_EVENTS does.

 * I would suggest that stripping out the version number part of the
   XEMBED protocol is probably not the best idea. Obviously it
   has to be done a little differently without the XEMBED_INFO
   property, but an initial version number handshake likely isn't
   hard.

 * The documentation and names of the inter-file private functions
   need a lot of work from my perspective, but it's probably easier
   to just do that myself once this lands rather than explaining
   it here.

 * A very simple way to handle map state is to say that if the 
   client (plug) is mapped when embedding begins, the client must send 
   a map message to the embedder.

 * There's reasonable documentation of the push/pop message stuff 
   in gtkxembed.c that you could just copy. 

   XEMBED_FOCUS_WRAPAROUND is explained in:

   http://lists.freedesktop.org/archives/xdg/2003-August/002216.html

   (Apparently never committed to CVS)

 * Could you revert plug_signals (socket_signals) => _gtk_plug_signals
   name change and the gtk_plug_set_is_child() ... set_is_child() 
   name change?

 * Instead of exporting _gtk_plug_add_grabbed_key_always() across
   file boundaries, you should put the g_hash_table_foreach() call
   into gtkplug.c

 * The handling of getting the minimum size looks better, though
   I don't see the point in calling GetClientRect(). But what you 
   aren't handling is changes in the minimum size. You may have
   to add a new virtual function to GtkWindow to allow a subclass
   to hoook in when it calls gdk_window_set_geometry_hints()
   [ There is padding in the class structure ]

> all the gtk_{plug,socket}_windowing functions changed to take a
> GtkPlug or GtkSocket as their first parameter rather than a
> random collection of functions.

OK, done.

> documentation of the protoocl you are using and how it differs from XEMBED.

This is the hard part ;-)

> It's not clear, for example, to me what GTK_WIN32_REQUEST_EVENTS does.

Hmm, looking at it again now, not to me either. I guess I must have confused myself.

That message is sent in _gtk_socket_windowing_select_plug_window_input(), which
on X11 calls XSelectInput() on the *plug* window to get StructureNotify and
PropertyChange events of the socket window. Nothing like that is possible on
Windows, and I guess my idea was that that message would request the socket's
code to send information about such events explicitly  to the plug code. But
then I lost my train of thought and what the handler for that message actually
does in gtkplug-win32.c is a reworked copy of the ReparentNotify handler in
gtkplug-x11.c. I.e. some confusing mixup here.

> stripping out the version number part of the XEMBED protocol is probably
> not the best idea.

OK, will add a version negotiation.

> revert plug_signals (socket_signals) => _gtk_plug_signals name change

OK, done.

> Instead of exporting _gtk_plug_add_grabbed_key_always()

OK, done.

Will continue to look at the other points. I won't attach fresh diffs yet, I
wrote this comment mostly so that I keep track myself of that remains to be done
in case I get sidetracked to other stuff inbetween ;-)

BTW, once this in good shape, do you think it can go into the stable branch,
too, or just HEAD?

Created attachment 48832 [details] [review]
Current diff

So, after some intense debugging and headscratching, I now have it working much
better on Win32. Now the resizing of embedded windows (the "Add" button in
testsocket) is noticed by the socket, and it resizes accordingly. Some
superfluous GTK_NOTE() calls are still present in this patch, will remove
before committing.

Why don't you go ahead and commit what you have. I think a lot of my
comments above still stand; e.g:

 - I don't think the current patch will even compile for non-Win32,
   non-X11 backends. A stubbed local-only version would be a nice
   addition.

 - It doesn't look like changes in the minimum size are handled

 - I'd really like to see some attempt at documenting the Win32 
   protocol

 - There are places where the win32 protocol should probably 
   deviate more from the X11 protocol ... if win32 doesn't have
   a ReparentNotify, then it would be simpler to use a client
   message rather than guessing from WM_WINDOWPOSCHANGED

 - You still dont' have a "version handshake", just a debug check
   for matching versions.

But let's get it into CVS. I think it's unlikely to regress the
X11 code, and then I can go ahead and fix up the docs for the
inter-file functions.

Committed now. I also added stub implementations for the non-X11, non-Win32
case. I'll have a look at your other points later.

Is more work on this required for 2.8 ?

What needs to be done is handling Owen's points in comment #28. 

Is there still work remainging to be done or should this bug be closed?