Havoc Pennington Reporter
	Description • 23 years ago

Bug #41786 was reported that gtk_window_set_title() didn't work with twm. 
However, this bug report was bogus; someone was using an ancient/broken twm
or something. XFetchName() is ancient cruft, use XGetWMName(). The fix for
#41786 was therefore broken, introducing #43108. Direct-to-Xlib code was put in
to handle #41786, but the solution to #43108 was to change the direct-to-Xlib
code back to work just like gtk_window_set_title() and revert the #41786 fix.
Which means that you now have code that does the same thing as
gtk_window_set_title(), but is doing some unkosher behind-gtk's-back sort of
fiddling in order to do so - kind of pointless. (Note that XmbSetWMProperties()
used by GTK is the same as what Mozilla is currently doing via XStdICCTextStyle).

Complicating matters, I assume recent CVS versions of mozilla set the
_NET_WM_NAME hint in UTF-8.

As an additional bug, the fallback case of calling gtk_window_set_title() with
Latin-1 is broken; GTK does not take Latin-1 strings, it takes strings in locale
encoding.

Suggested fix:

 - GTK should always be passed locale encoding; nuke the Latin-1 case

 - always use gtk_window_set_title(), to avoid confusing GTK and probably 
   having bugs with future GTK versions; e.g. gtk_window_get_title() really 
   ought to work

I'll attach an untested patch (I'm not really set up to compile the beast ;-)

	Havoc Pennington Reporter
	Comment 2 • 23 years ago

BTW, if gtk_window_set_title() or other GTK functions don't work, please file a
GTK bug on bugzilla.gnome.org; simply inserting a hacky workaround isn't good
for long-term maintainability. Also, Owen probably could have saved you a bunch
of time trying to figure out what to do.

Granted, GTK bugzilla didn't exist at the time those older bugs were handled. ;-)

	Doron Rosenberg (IBM)
	Comment 3 • 23 years ago

cc'ing pavlov and blizzard

	Christopher Blizzard (:blizzard) Assignee
	Comment 4 • 23 years ago

hp, this is against 0.9.2, right?

	Havoc Pennington Reporter
	Comment 5 • 23 years ago

Yep, I used the rawhide SRPM

	Christian Reis
	Comment 6 • 23 years ago

I'm marking this NEW so it gets off our radar; I assume HP knows what
he's talking about, and I've verified the code hasn't been changed.
Havoc, if you have the time to do more random code reviews like this, we
would be very grateful :)

Module owners, how reasonable does this sound to you?

	Stuart Parmenter
	Comment 7 • 23 years ago

Comment on attachment 46253 [details] [diff] [review]
suggested patch

Does this patch break bugs 41786 and 43108?  the code you are removing was put in specifically for those bugs.

	Havoc Pennington Reporter
	Comment 8 • 23 years ago

Well, I explained my views on #43108 and #41786 - I think they were basically
one big wild goose chase and the current code ended up doing the same thing 
GTK does anyhow. I could be wrong, testing it isn't a terrible idea, but
plain GTK does normally work with twm. ;-)

	Christopher Blizzard (:blizzard) Assignee
	Comment 9 • 23 years ago

-> blizzard@mozilla.org

	Christopher Blizzard (:blizzard) Assignee
	Comment 10 • 23 years ago

Frank, would you or someone on your team be willing to take the time to test out
this patch and make sure that it works in some random locales?

	Shanjian Li
	Comment 11 • 23 years ago

I played with this patch on HPUX. The hang (see bug 108765) actually happens 
inside Xlib call. So gtk_window_set_title is as robust as the existing block of 
code. I am OK with this patch from i18n perspective. 

	Christopher Blizzard (:blizzard) Assignee
	Comment 12 • 23 years ago

So, just to be clear, this patch fixes your hang?

	Shanjian Li
	Comment 13 • 23 years ago

Yes, this patch fixed my hang on HPUX.

	Christopher Blizzard (:blizzard) Assignee
	Comment 14 • 23 years ago

remerged patch

	Alec Flett
	Comment 15 • 23 years ago

Comment on attachment 57890 [details] [diff] [review]
remerged patch

eh, seems ok to me :)
sr=alecf

	Brian Ryner (not reading)
	Comment 16 • 23 years ago

Comment on attachment 57890 [details] [diff] [review]
remerged patch

r=bryner

	Christopher Blizzard (:blizzard) Assignee
	Comment 17 • 23 years ago

Checked in on the tip.

	David Baron :dbaron:
	Comment 18 • 23 years ago

Comment on attachment 57890 [details] [diff] [review]
remerged patch

a=dbaron for 0.9.6

	Christopher Blizzard (:blizzard) Assignee
	Comment 19 • 23 years ago

Checked into 0.9.6, too.