57799 - Mozilla needs to respond to system Appearance Theme changed events (fonts/colours)

Status: RESOLVED WONTFIX

(Core :: XUL, defect, P3)

Description

[Andrew Thompson]

When the system wide Appearence theme changes, Mac OS sends some Apple events to
inform applications of this change. These are documented here:

http://developer.apple.com/techpubs/macos8/HumanInterfaceToolbox/AppManager/ProgWithAppearanceMgr/Appearance.94.html#13915

Currently, my work on bug 1004 and bug 46174 is revealing that mozilla does not
respond to these events, hence when the appearence theme is changed Mozilla does
not recognise this change until a new window is opened.

We need to respond to these events by redrawing all open windows and discarding
any cached appearence information we might have. (I'm not sure we do cache
anything at the moment, but I believe this will be necessary for performance
reasons. More on this as I discover more)

Comment 1

[hangas]

Need to find an owner for this one.  Any suggestions?  This is not a Themes bug 
since it is not a specific issue with Modern or Classic.  I don't think this 
falls under Skinability or Event Handling either.  ccing for help assigning this 
bug...

Comment 2

[Peter Trudelle]

Sounds like XPToolit to me, taking & futuring.

Comment 3

[Andrew Thompson]

Agreed. I'd forgotten that Modern uses the system font, though naturally classic
(and potentially other themes) care far more about what the rest of the OS looks 
like.

Resummarising

Comment 4

[Peter Trudelle]

Its time this bug had a real owner, even if it stays future.  who should get it?

Comment 5

[Blake Ross]

Giving to lordpixel, who made good progress on this...

Comment 6

[Andrew Thompson]

FIXED. Code is a bit rough, but it works. Respond to event by calling
ChromeRegistry.RefreshSkins()

Will attach patches

Comment 7

[Andrew Thompson]

Attachment: Patch to AppShell + Apple Events to add appearance events

Comment 8

[Andrew Thompson]

Attachment: New class to do appearance events

Comment 9

[Andrew Thompson]

Attachment: Header for above class

Comment 10

[Andrew Thompson]

Patch is pretty rough. Breaks some new ground for me so please scrutinise. 

I think smfr should review as most of the Apple Events code seems to be his.

Comment 11

[Andrew Thompson]

Attachment: Cleaned up version of above .cpp file

Comment 12

[timeless]

* for all new files, please use consider using mpl boilerplate license unless 
you are under a contract that requires you to use another.
http://www.mozilla.org/MPL/boilerplate-1.1/
* please try to limit lines to 80 columns,
* avoid tabs
* follow the emacs modeline [23429 says 2 spaces but it looks like you're using 
tabs]

there's a general warning about using C++ exceptions due to risks of crossing 
xpcom boundaries into an object compiled by a different compiler that handles 
exceptions differently.  -- It looks like you do risk that.

Comment 13

[Simon Fraser [no longer active]]

timeless: The mac Apple Event code has been written to use exceptions (mostly 
because it was derived from another project). Because it's mac-only code, I think 
this is OK.

For some reason, timeless marked this fixed. reopening.

Comment 14

[timeless]

sorry about the resolution.

Comment 15

[Andrew Thompson]

Attachment: cpp file with cleanups as proposed by timeless

Comment 16

[Andrew Thompson]

Attachment: .h file with similar cleanups

Comment 17

[Andrew Thompson]

(no, I ain't *just* spamming you, I updated the status whiteboard too :-)
grrrr. I hate tabs as spaces. Its 2001 and we can't get editors to do something 
sensible.

I blame vi users for 8 space abominations!

Comment 18

[Brendan Eich [:brendan]]

Uh, no one uses vi these days.  vim has :set expandtabs, there is no excuse.

I see no tabs in the .cpp file, but the try body is overindented by four spaces.

Please change the Emacs modeline comments to set tab-width: 4 and
c-basic-offset: 4, to match the code.

/be

Comment 19

[Andrew Thompson]

Attachment: Brendan's cleanups applied to .cpp

Comment 20

[Andrew Thompson]

Attachment: Brendan's cleanups applied to .h

Comment 21

[Andrew Thompson]

Heh. Ok done. Perhaps I shoul have put in another smiley after that vi comment. 
Actually I use vi. I have to deal with Solaris 7 systems.

Anyway, does someone want to r= this?

Comment 22

[Brendan Eich [:brendan]]

http://www.vim.org -- it's open source.

r=[e-mail], maybe sfraser or another Mac dude should sr=?

/be

Comment 23

[Simon Fraser [no longer active]]

Why do you need the try/catch block in 
AEAppearanceSuiteHandler::HandleAppearanceSuiteEvent, when the calling routine 
will catch exceptions for you?

Comment 24

[Andrew Thompson]

I think I copied that from ?your? code in the Spyglass suite handler.

I'll look at it when I get back this weekend...

Comment 25

[Andrew Thompson]

Attachment: Latest simplified version of .cpp file. Removed uneeed exception catch

Comment 26

[Steve Dagley]

Since lordpixel seems to have a patch ready to go for this shouldn't it have a 
target of Mozilla0.9 rather than future?

Comment 27

[Patty Mac]

The component is XP Toolkit, not themes. Please change the QA contact. Thanks!

Comment 28

[Marcia Knous [:marcia]]

nominating for dogfood (from sdagley's list of bugs that are good candidates for 
our next release) 

Comment 29

[andreww]

Has there been any movement on this?  Is it ready to check in?

Comment 30

[Andrew Thompson]

Still waiting for r= from a Mac person.

Comment 31

[Simon Fraser [no longer active]]

Yeah i suck

Comment 32

[Mike Pinkerton (not reading bugmail)]

i would suggest moving 

#include "nsIChromeRegistry.h"
#include "nsCOMPtr.h"

from the .h file into the .cpp file. There's nothing in the .h file that 
references them. It's best to reduce includes to a minimum, especially in 
headers. 

Comment 33

[ajbu]

This could also be useful for Windows. (bug 6061, bug 67625 and others)

In an earlier attempt to fix this, an event NS_DISPLAYCHANGED was created (bug 
6061). At the moment only fired on Windows and currently unused.
http://lxr.mozilla.org/mozilla/source/widget/public/nsGUIEvent.h#371

Would it be possible for the Mac to fire NS_DISPLAYCHANGED (or create a new 
event type for this if you don't like that name based on the Windows event) and 
handle the Refresh code in the event handler? That way we can use it on all 
platforms.

I tried this out on Win32 and it seems to work fine. (I simply added 
RefreshSkins() code to PresShell::HandleEvent, not sure what the best place for 
this would be)

Comment 34

[Andrew Thompson]

Sure, doing this in a consistent manner would be really cool. One thing though, 
we're not really fixing the same bug... what you're responding to is bit depth 
changes etc, whereas I'm responding to theme changes, i.e. changes to the whole 
look and feel of the operating system.

This means I'll probably have to fire the event on Mac OS in 2 circumstances,

1/ when the colour depth changes
2/ when the theme changes

You might want to fire it when a windows user changes the theme (color scheme in 
the Display control panel) on Windows? And what of GTK? 

Is there code which shows how to fire this eventt? I know next to nothing about 
Moz events.

Comment 35

[ajbu]

bug 67625 is about switching colors (which happens in Windows when switching 
themes), when I'm correct this patch could  all 'os  appearance changes' when 
triggered after the change. On Windows we get a few messages like 
syscolorschange, displaychanged, settingschange when this happens.

I don't know much about NS events, here is what I scrambled together:

On Windows the event is fired with the line: DispatchStandardEvent
(NS_DISPLAYCHANGED)
In the function that listens to Windows events it does this with a case:
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#2767
         case WM_DISPLAYCHANGE:
           DispatchStandardEvent(NS_DISPLAYCHANGED);
         break;

DispatchStandardEvent is defined on Windows as (there is not a Mac definition 
for this afaict):
PRBool nsWindow::DispatchStandardEvent(PRUint32 aMsg)
{
  nsGUIEvent event;
  event.eventStructType = NS_GUI_EVENT;
  InitEvent(event, aMsg);

  PRBool result = DispatchWindowEvent(&event);
  NS_RELEASE(event.widget);
  return result;
}


DispatchWindowEvent is defined on Mac:
http://lxr.mozilla.org/seamonkey/source/widget/src/mac/nsWindow.cpp#1930

that file has a DispatchMouseEvent on line #1949 that seems to handle some Mac 
events, looks like it could be used as a template for this.

Comment 36

[Andrew Thompson]

OK, so if you can provide your patch at the end of bug 6061 in diff/patch format, 
I'll apply it then try to figure out how to fire the event on Mac OS.

That should give us a good solution... 

Comment 37

[Andrew Thompson]

Moving to 0.9.1 as per Marek's request. Need to decide how to proceed with a 
coherent story for handling theme switch and colour depth switches cross 
platform.

Comment 38

[chris hofmann]

what are the chances of this landing in the next week or so.
if not good lets unset the target milestone until we get a good
plan in place.

Comment 39

[Andrew Thompson]

I have no plan to resolve the outstanding issues at present. Needs research.

Comment 40

[Greg K.]

Perhaps superficially related to Bug 6061 and Bug 101843?

Comment 41

[Frankie]

Mozilla CFM build is dead; should this bug go with it?

Comment 42

[Simon Paquet [:sipaq]]

This bug is targeted at a Mac classic platform/OS, which is no longer supported
by mozilla.org. Please re-target it to another platform/OS if this bug applies
there as well or resolve this bug.

I will resolve this bug as WONTFIX in four weeks if no action has been taken.
To filter this and similar messages out, please filter for "mac_cla_reorg".

Comment 43

[Andrew Thompson]

Classic support long dead