106695 - Rewrite activate/update events for CarbonEvents

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Mac, defect)

Description

[Mike Pinkerton (not reading bugmail)]

subject says it all

Comment 1

[Mike Pinkerton (not reading bugmail)]

-> 098

Comment 2

[Mike Pinkerton (not reading bugmail)]

not gonna happen in 098

Comment 3

[Ari]

Attachment: attempt at a patch

This didn't seem to hard to fix so I attempted to write a patch. This patch
includes an updated patch for bug 99987.

Comment 4

[Mike Pinkerton (not reading bugmail)]

comments on the patch:

+        ::SetWindowClass(mWindowPtr, kSheetWindowClass); // we really shouldn't
call this because it is deprecated

if we should be using SetWindowGroup, let's just make that change now. it's
available in carbonlib 1.4 right?

+    OSStatus err = ::InstallWindowEventHandler ( mWindowPtr,
NewEventHandlerUPP(WindowEventHandler),
+                                                
GetEventTypeCount(windEventList), windEventList, this, NULL ); 

what is GetEventTypeCount()? I couldn't find it in apple headers.

+  if ( myWind && self) {

fix minor spacing. |if ( myWind && self ) {|

-      case kEventWindowDrawContent:

rjc put this in for sheets. should you be removing it?

+        retVal = noErr;

can you go ahead also and comment that returning noErr means we consume the
event? just makes it crystal clear and follows the pattern already set up in
that routine in the other cases. Yah, it's redundant, but with so many people
tromping through this code, i'd err on the side of avoiding confusion.

looking good otherwise. cc'ing rjc re: the sheet changes since i saw another
patch of his this morning that touches this part of the code.

Comment 5

[Ari]

If the case of this call to ::SetWindowClass(), I think we actually want to set
the class of the window, not the group, so using the window group apis would not
be appropriate. Class is supposed to be an immutable property, so the only other
way to set is to create the window using, the new window creation function --
CreateNewWindow().

GetEventTypeCount() is a macro defined in CarbonEvents.h as follows:
#define GetEventTypeCount( t )  (sizeof( (t) ) / sizeof( EventTypeSpec ))

According to my understanding of the comments CarbonEvents.h,
kEventWindowDrawContent is only used if the window has the
kWindowStandardHandlerAttribute. The two events need to be handled slightly
differently. kEventWindowUpdate requires that we set the port and call
BeginUpdate() and EndUpdate() while kEventWindowDrawContent doesn't. If we ever
switch to using the standard handler, we should replace kEventWindowUpdate with
kEventWindowDrawContent. I put a comment in that tries to explain this. 

Comment 6

[Mike Pinkerton (not reading bugmail)]

re: GetEventTypeCount. I must have missed it last time i looked, now i see it.
thanks.

ok on the SetWindowClass, but if that api has been depricated, how are we
supposed to get the same functionality?

i'll let rjc chime in on the drawing carbon events. I understand the difference,
just he knows the exact details on why he had to include them.

Comment 7

[Ari]

As far as I know, the only other way to set the window class is to do so at
creation time using the new creation function:

/*
 *  CreateNewWindow()
 *  
 *  Availability:
 *    Non-Carbon CFM:   in WindowsLib 8.5 and later
 *    CarbonLib:        in CarbonLib 1.0 and later
 *    Mac OS X:         in version 10.0 and later
 */
EXTERN_API( OSStatus )
CreateNewWindow(
  WindowClass        windowClass,
  WindowAttributes   attributes,
  const Rect *       contentBounds,
  WindowRef *        outWindow);

This makes sense because you are not supposed to be able to change the class
once the window is created.

Comment 8

[Jonathan Brecher]

Regarding SetWindowClass, it appears to me that you're already doing the right 
thing.  mIsSheet is only true for windows that were created as sheets in the 
first place:

              if (nsToolkit::OnMacOSX() && aParent && (aInitData->mBorderStyle & 
eBorderStyle_sheet))
              {
                  nsWindowType parentType;
                  aParent->GetWindowType(parentType);
                  if (parentType != eWindowType_invisible)
                  {
                      // Mac OS X sheet support
                      mIsSheet = PR_TRUE;
                      wDefProcID = kWindowSheetProc;
                  }
              }
[...]
     mWindowPtr = ::NewCWindow(nil, &wRect, "\p", false, wDefProcID, 
(WindowRef)-1, goAwayFlag, (long)nsnull);


Try taking out the SetWindowClass call entirely, and see if everything still 
works?  I'm pretty sure it will...


Regarding DrawContent vs WindowUpdate, yeah, you really want support DrawContent 
instead of WindowUpdate.  On the other hand, you really *really* want to create 
your windows with the standard handler.  That'll happen eventually, even if 
you're not planning on it yet, because the standard handler is a Good Thing.  If 
sheets work now, then great.  If they don't, you're going to have to untangle 
DrawContent/WindowUpdate, else switch over to the standard handler now and use 
DrawContent only.  If there are problems now, I'd most expect them on windows 
that magically got the standard handler installed without your realizing it.  Be 
sure to check the open/save and pagesetup/print, plus various alerts, etc.

Comment 9

[Ari]

In response to comment 8:
We know that SetWindowClass() is being used correctly in the case that we are
discussing. It's just that that function is deprecated, and we're thinking of
another way to do the same thing. We can't just remove the call to
SetWindowClass() because rjc's sheet patch uses GetFrontWindowOfClass().

As for the standard handler, I know that it is not yet being installed and
shouldn't be yet until we move farther along in the CarbonEvent transition.

Comment 10

[Robert John Churchill]

> cc'ing rjc re: the sheet changes since i saw another patch of his
> this morning that touches this part of the code.

Pink was referring to bug # 122102 (resizable sheets)


As for getting rid of ::SetWindowClass() usage, just do this:


#if TARGET_CARBON
    if (mIsSheet) {
      WindowAttributes attribs = kWindowNoAttributes;
      if (resizable) attribs |= kWindowResizableAttribute;
      ::CreateNewWindow(kSheetWindowClass, attribs, &wRect, &mWindowPtr);
    }
    else
#endif
      mWindowPtr = ::NewCWindow(nil, &wRect, "\p", false, wDefProcID,
(WindowRef)-1, goAwayFlag, (long)nsnull);



and then you can remove the ::SetWindowClass() from farther below.


> According to my understanding of the comments CarbonEvents.h,
>kEventWindowDrawContent is only used if the window has the
> kWindowStandardHandlerAttribute.


kEventWindowDrawContent IS called for sheets without our setting the standard
handler (unless its getting set by the OS behind our backs).

Comment 11

[Ari]

You're right. Apple's header docs are not quite right. We are getting
kEventWindowDrawContent for sheets but not other windows even though neither
have the kWindowStandardHandlerAttribute.

As for CreateNewWindow(), we might as well use it for all windows, not just
sheets especially since it's available in classic in WindowsLib 8.5.

Comment 12

[Robert John Churchill]

Feel free, although it'll entail reworking a chunk of nsMacWindow.cpp from the
looks of things.  (Perhaps that would be best done in a different bug, if you want.)

Comment 13

[Ari]

Attachment: revised patch

A revised patch incorperating some of the feedback. (It doesn't deal with the
SetWindowClass() issue) It incorperates the resizable sheet patch.

Comment 14

[Ari]

WEIRD: Handling kEventWindowDrawContent causes bug 121667.

Comment 15

[Robert John Churchill]

*** Bug 121667 has been marked as a duplicate of this bug. ***

Comment 16

[Robert John Churchill]

*** Bug 121671 has been marked as a duplicate of this bug. ***

Comment 17

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 66828 [details] [diff] [review]
revised patch

i played around with the patch this morning and found the following:

- it doesn't compile as-is. I needed to add another ifdef for DoUpdate in the
message pump

- with the patch applied, sheets would flash (disappeaar then reappear) before
rolling up. Turning off the activate/update ifdefs didn't solve it. Backing it
out did.

- with the patch applied, cmd keys wouldn't work when clicking a window to
activate it. I had to click _again_ to get cmd keys to work. Turning off
activate/update ifdefs didn't solve it. Backing it out did.

So there seem to be some window activation issues with this patch, but not only
that, the ifdefs don't quite work as they should.

Comment 18

[Mike Pinkerton (not reading bugmail)]

over to dagley, i probably won't have time for these by 099 with the embedding
work i'm getting pulled into.

Comment 19

[Steve Dagley]

-> mozilla1.0

Comment 20

[Steve Dagley]

ajfeldman, since you seem to have been the one working on this, any update?  Is
this something we can land for Moz1.0?

Comment 21

[Ari]

I have a patch that addresses the build problems and flashing sheets 
mentioned in comment 17. The flashing sheets seem to be caused by 
the fact that for an instant the parent window is being placed in front of the 
sheet as it is rolling up. However, I can't verify the cmd keys issue 
because I'm working from the mach-o build and cmd keys are pretty 
broken in that build to begin with. Lastly, I'm getting a lot of thread-safety 
warnings whenever a sheet rolls up.

Since bug bug 121671 and bug 122102 are not fixed by simply switching 
to carbon update events as we had previously hoped, this is probably not 
worth doing for 1.0 unless we can quickly resolve the above issues.

Comment 22

[Mike Pinkerton (not reading bugmail)]

fwiw, i saw the same kinds of "flashing" of sheets (comment 17) when closing
them in OfficeX. It could just be carbon bugs we're running into and not
something inherently wrong with the patch.

Comment 23

[saari (gone)]

may be needed for embedding, marking topembed. Needs investigation

Comment 24

[Robert John Churchill]

I agree, sheets in Mac OfficeX absolutely suck.  We don't currently have any
flashing sheet problems on Mac OS X, let's keep it that way.

Comment 25

[Robert John Churchill]

Please note that the few lines of code to support resizable sheets on Mac OS X
has been checked in (see bug # 130218)

Comment 26

[dreamless]

It's been a while since there have been any comments on these Carbon Events
bugs. Is there any work being done here?

Comment 27

[Steve Dagley]

Since I don't report into Internet Technologies anymore this bug needs a new
owner -> saari

Comment 28

[Mark Mentovai]

Attachment: Patch (1.8)

I wasn't really going to do this, until I noticed that we were doing too much during each activate event: we'd handle it once on a window-scope CE handler, and again on the application-scope WNE [transitional] handler.  So I fixed them to be handled by the window-scope handler only, finding a system foible in the process - that's what the nsWindow part of the patch is about.

As long as I was in there, I reworked activate/deactivate events to be handled by window-scope CE handlers.  The IgnoreDeactivate stuff is disappearing because HiliteWindow isn't necessary anyway.  The system does proper hilighting when you leave it alone.  The frontmost window is activated and deactivated when the application is activated and deactivated, so the suspendResumeMessage stuff to handle application-scope activation and deactivation is unnecessary.  We don't need to keep track of app-active under OS X anyway (and keeping track was buggy, since we'd assume that the app was active at launch, which isn't necessarily the case, especially with all the self-restarting...)

This is the 1.8 patch.  The trunk patch is the same except it gets rid of the ignoreDeactivate attribute from nsPIWidgetMac - can't do that on the branch because of interface rules.

Comment 29

[Mark Mentovai]

Comment on attachment 223080 [details] [diff] [review]
Patch (1.8)

Rollup not happening on app deactivate?

Comment 30

[Mark Mentovai]

Attachment: Patch v2 (1.8)

The existing rollup code for window deactivates was dysfunctional in that it had a check that prevented it from ever succeeding.  This change makes it match what we do elsewhere.  Because the rollup widget should only ever be attached to an active window, this is fine.

This change fixes a related bug: if you have two windows open and open a contextual menu in the foreground window, it doesn't roll up when you press command-~ to rotate to the background window.

Comment 31

[Mark Mentovai]

Attachment: Patch v2 (trunk)

Same as the 1.8 version, adjusted to apply to the trunk.  Also removes a dead method from the private interface.

Comment 32

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 223221 [details] [diff] [review]
Patch v2 (trunk)

sr=pink

Comment 33

[Mark Mentovai]

Checked in on trunk.

Comment 34

[Mark Mentovai]

Comment on attachment 223211 [details] [diff] [review]
Patch v2 (1.8)

Marking a? as a reminder for when the tree reopens.

Comment 35

[Mark Mentovai]

Attachment: 1.8 branch version (includes 339640)

Checked in on MOZILLA_1_8_BRANCH.

Comment 36

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix for bug 341234 (regression caused by this fix)

The fix for this bug has caused a regression at bug 341234.  Here's a
very simple patch that resolves the problem (it reverts a small part
of nsMacEventHandler.cpp to the way it was before the fix for this bug
(106695) was landed).

I don't really understand why this patch fixes bug 341234 ... but fix
it it does.

Comment 37

[Mark Mentovai]

Comment on attachment 225593 [details] [diff] [review]
Fix for bug 341234 (regression caused by this fix)

Discussion of this patch and the regression it attempts to resolve in bug 341234.