[gtk2] windows not activating correctly

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: bryner, Assigned: bryner)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

This seems to be gtk2-only.  If I load a page in a browser window, then open a
mail compose window, then switch back to the browser window, I cannot use
pageup/pagedown to scroll the page - focus memory is not getting restored.
Hitting tab several times does allow you to recover focus in the window.
Yay for focus problems.
Blocks: gtk2
I *think* this may be a metacity bug. I've upgraded to cvs of metacity and can't
say i see this problem. Reporter do you use metacity as your window damager ?
Ok, I found out what's going on.  When the mail compose window is deactivated,
we update some commands which cause data to be fetched from the clipboard.  This
can cause another iteration of the gtk mainloop, which means that we can get the
focus-in event for the browser window while we're still processing the
deactivate for the compose window.  This causes gFocusWindow to incorrectly end
up set to null.

Still thinking about what the right fix is here -- I can get gFocusWindow to be
correct easily enough, but this is still likely to leave things in a bad state
as we assume the events can be handled atomically (I'm thinking of ESM and focus
controller).  What I think I'd like to do is skip over events (without changing
their order in the event queue) if we're in the middle of a call to
gtk_clipboard_wait_for_*.
... but I don't see a way to make gtk, or X, do that.  Which makes sense, as
being a queue means you can only take the event off the front of it.

Here are a few other ideas:

1. Make Mozilla's clipboard API be asynchronous so that it maps better to GTK's
functionality.

2. Could we could queue up the events ourselves during execution of the nested
mainloop, and re-insert them into the event queue after the clipboard function
returns?  That's getting kind of messy.

3. Make Mozilla's event system able to deal with reentrancy between deactivate
and activate events.
I'd like to make the clipboard API async, if we can.  It would make things soooo
much easier.

( I'd also like to do the same for the DND api while we're at it, since drop
events are async as well. )
Yet another idea is to reinstate some version of the gtk1
nsClipboard::FindSelectionNotifyEvent, which waits for the data without running
the gtk event loop.  This might require no longer using GtkClipboard, though.
Posted patch patch (obsolete) — Splinter Review
Ignore my earlier comment about not being able to manipulate the event queue. 
This patch ports some code from the gtk1 source to wait for the SelectionNotify
event and send it to the clipboard, without letting any other events be
processed.
Attachment #130398 - Flags: superreview?(blizzard)
Attachment #130398 - Flags: review?(pavlov)
Making all the callers of the clipboard code, including embedders use async
clipboard APIs I suspect would be far more difficult than making the gtk2 code
sync.  That said, I wish we had made the clipboard APIs async 4 years ago :/
Comment on attachment 130398 [details] [diff] [review]
patch

let's not do this.  It introduces bug 56219 to the gtk port.  I'll come up with
something better.
Attachment #130398 - Attachment is obsolete: true
Attachment #130398 - Flags: superreview?(blizzard)
Attachment #130398 - Flags: review?(pavlov)
You know what, let's just make the clipboard API async.  I'll hook up some
dependent bugs.
Depends on: 218040
Posted patch new patch (obsolete) — Splinter Review
I think there's actually a way to do this using my original approach (with a
little tweaking).  I discovered XCheckIfEvent() and can now pull events out of
the queue in the correct order.  I tested it with pasting a very large amount
of text, so I'm sure I didn't introduce bug 56219.

This also fixes an unnecessary temporary string assignment in GetData(), which
should save a string copy.
Attachment #130804 - Flags: superreview?(blizzard)
Attachment #130804 - Flags: review?(pavlov)
Comment on attachment 130804 [details] [diff] [review]
new patch

Couple of things: use select() on the X connection (much to the chagrin of VMS)
not sleep.

Also, please declare all functions at the top of the file instead of just
defining them inline.
Comment on attachment 130804 [details] [diff] [review]
new patch

> use select() on the X connection (much to the chagrin of VMS) not sleep.

Are you saying it should be ifdef'd for VMS to still use sleep?
Posted patch new patchSplinter Review
Fixed blizzard's comments (I took the VMS fu from gtk2xtbin.c, hope it's
right).  This also fixes it to not wait for the timeout if the request
completed synchronously (i.e. copying from the same Mozilla instance).
Attachment #130804 - Attachment is obsolete: true
Attachment #130879 - Flags: superreview?(blizzard)
Attachment #130879 - Flags: review?(pavlov)
Attachment #130879 - Flags: superreview?(blizzard) → superreview+
Attachment #130804 - Flags: superreview?(blizzard)
This patch avoids using API's that are not present in GTK+ 2.0.x.
Attachment #131613 - Flags: review?(blizzard)
What, you don't just want to make the APIs async? :)
Comment on attachment 131613 [details] [diff] [review]
patch that works on gtk+ 2.0.x

Not right now, no :)
Attachment #131613 - Flags: superreview?(dbaron)
Comment on attachment 131613 [details] [diff] [review]
patch that works on gtk+ 2.0.x

r=blizzard

My only comment is that WaitForRetrieval is StudlyCaps while other static
functions use_underscores, which I prefer.
Attachment #131613 - Flags: review?(blizzard) → review+
Attachment #130804 - Flags: review?(pavlov)
Attachment #130879 - Flags: review?(pavlov)
Comment on attachment 131613 [details] [diff] [review]
patch that works on gtk+ 2.0.x

I'm just going to use blizzard's review since this isn't part of the default
build.
Attachment #131613 - Flags: superreview?(dbaron)
checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
No longer depends on: 218040
i think this may have caused bug 226405
You need to log in before you can comment on or make changes to this bug.