78363 - Beachball in Mac `spinning' cursor should actually spin

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P5)

Description

[Matthew T (active 1999-2002)]

Build: 2001043008, Mac OS 9.1

To reproduce:
*   Go from one Web page to another.
*   Look at the combination arrow-and-beachball cursor.

What you should see:
*   The beachball spins round and round.

What you actually see:
*   It doesn't.

Comment 1

[Mike Pinkerton (not reading bugmail)]

--> pchen.

Comment 2

[Andrew Thompson]

OK, Apple "helpfully" removed RotateCursor and SpinCursor from Carbon. As it 
happens they were always part of the MPW toolkit, not a core part of the 
Quickdraw API. That's the reason...

However, since all Spin/RotateCursor actually does is advance to the next frame 
in a cursor sequence (as defined in an acur resource) for you, they're not 
actually necessary. Consider: the hard part was always deciding when to call 
SpinCursor - you still had to call it every time you wanted to advance a frame, 
it does no automatic animation for you.

So, at mpt's suggestion, I've just implemented this by calling SetCursor 
repeatedly, and we maintain the state which knows which frame to draw. This is 
all neatly wrapped up in nsDocShell::OnProgress. Patches to follow...

Comment 3

[Andrew Thompson]

oh, this will fix 78362 also..

Comment 4

[Andrew Thompson]

Attachment: Patch to make cursor spin as documents load

Comment 5

[Andrew Thompson]

Also take Matthew's cursors from bug 78362 and paste them into nsmacWidget.rsrc 
as numbers 149, 150, 151 and 152

Comment 6

[Simon Fraser [no longer active]]

You can't #ifdef XP_MAC in nsDocShell. Sorry. You'll have to use timers or 
something.

Comment 7

[sairuh (rarely reading bugmail)]

marking p5, future

Comment 8

[Andrew Thompson]

Taking (goes with a bunch of other polish bugs I want to get to)

Comment 9

[Andrew Thompson]

Note to self: EventStateManager (as in nsIEventStateManager)

Comment 10

[Frankie]

Still occurs in OSX, but this bug probably does not affect LinuxPPC.

Comment 11

[Andrew Thompson]

Attachment: macWidget.rsrc with MPTs additional cursors added

This is nsMacWidget.rsrc with MPTs 3 additional cursor frames added.
This file is MacBinary encoded, to preserve the resource fork.
Currently the file in CVS is in AppleSingle format, so we'll need to unpack
this file to a regular 2 fork Mac file then check it in using something like
MacCVSPro that can AppleSingle encode it.

I'll work on some code to use these frames shortly.

Comment 12

[Andrew Thompson]

Attachment: patch uses a carbon event loop timer to spin the cursor

Comment 13

[Andrew Thompson]

Attachment: Apple Single encoded resource file with 4 frame cursor animation

To try this:

Put this "nsMacWidget.rsrc" file into /mozilla/widget/src/mac/
Apply the patch
Build & run

Comment 14

[Andrew Thompson]

Comment on attachment 121042 [details] [diff] [review]
patch uses a carbon event loop timer to spin the cursor

Reviewers: Any threading issues strike you as troublesome?

Comment 15

[Simon Fraser [no longer active]]

Comment on attachment 121042 [details] [diff] [review]
patch uses a carbon event loop timer to spin the cursor

>Index: nsWindow.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/mac/nsWindow.cpp,v
>retrieving revision 1.198
>diff -u -2 -r1.198 nsWindow.cpp
>--- nsWindow.cpp	25 Mar 2003 02:58:00 -0000	1.198
>+++ nsWindow.cpp	19 Apr 2003 03:53:34 -0000
>@@ -85,4 +85,7 @@
> static Boolean	gNotificationInstalled = false;
> 
>+//used to animate the Arrow + Beachball cursor
>+static CursorSpinner *gCursorSpinner = new CursorSpinner();
>+

Best not to rely on running code at library load time. Allocate the thing when
it's first required.

> // Routines for iterating over the rects of a region. Carbon and pre-Carbon
> // do this differently so provide a way to do both.
>@@ -106,5 +109,5 @@
> #endif
> 
>-
>+#if defined(INVALIDATE_DEBUGGING) || defined(PAINT_DEBUGGING) || defined (PINK_PROFILING)
> static Boolean KeyDown(const UInt8 theKey)
> {
>@@ -113,4 +116,5 @@
> 	return ((*((UInt8 *)map + (theKey >> 3)) >> (theKey & 7)) & 1) != 0;
> }
>+#endif
> 
> #if defined(INVALIDATE_DEBUGGING) || defined(PAINT_DEBUGGING)
>@@ -699,117 +703,52 @@
>   nsBaseWidget::SetCursor(aCursor);
> 	
>-	// allow the cursor to be set internally if we're in the bg, but
>-	// don't actually set it.
>-  if ( !nsToolkit::IsAppInForeground() )
>+  // allow the cursor to be set internally if we're in the bg, but
>+  // don't actually set it.
>+  if ( !nsToolkit::IsAppInForeground() ) {
>     return NS_OK;
>-
>-  // mac specific cursor manipulation
>-	if (nsToolkit::HasAppearanceManager())
>-	{
>-		short cursor = -1;
>-	  switch (aCursor)
>-	  {
>-	    case eCursor_standard:				cursor = kThemeArrowCursor;						break;
>-	    case eCursor_wait:						cursor = kThemeWatchCursor;						break;
>-	    case eCursor_select:					cursor = kThemeIBeamCursor;						break;
>-	    case eCursor_hyperlink:				cursor = kThemePointingHandCursor;		break;
>-			case eCursor_sizeWE:					cursor = kThemeResizeLeftRightCursor;	break;
>-			case eCursor_sizeNS:					cursor = 129; 	break;
>-			case eCursor_sizeNW:					cursor = 130; 	break;
>-			case eCursor_sizeSE:					cursor = 131; 	break;
>-			case eCursor_sizeNE:					cursor = 132; 	break;
>-			case eCursor_sizeSW:					cursor = 133; 	break;
>-			case eCursor_arrow_north:			cursor = 134; 	break;
>-			case eCursor_arrow_north_plus:cursor = 135; 	break;
>-			case eCursor_arrow_south:			cursor = 136; 	break;
>-			case eCursor_arrow_south_plus:cursor = 137; 	break;
>-			case eCursor_arrow_west:			cursor = 138; 	break;
>-			case eCursor_arrow_west_plus:	cursor = 139; 	break;
>-			case eCursor_arrow_east:			cursor = 140; 	break;
>-			case eCursor_arrow_east_plus:	cursor = 141; 	break;
>-			case eCursor_crosshair:				cursor = kThemeCrossCursor; 					break;
>-			case eCursor_move:						cursor = kThemeOpenHandCursor; 				break;
>-			case eCursor_help:						cursor = 143; 	break;
>-			case eCursor_copy:						cursor = 144; 	break; // CSS3
>-			case eCursor_alias:						cursor = 145; 	break;
>-			case eCursor_context_menu:		cursor = 146; 	break;
>-			case eCursor_cell:						cursor = kThemePlusCursor; 			 			break;
>-			case eCursor_grab:						cursor = kThemeOpenHandCursor; 				break;
>-			case eCursor_grabbing:				cursor = kThemeClosedHandCursor; 			break;
>-			case eCursor_spinning:				cursor = 149; break;  // better than kThemeSpinningCursor
>-			case eCursor_count_up:				cursor = kThemeCountingUpHandCursor; 					break;
>-			case eCursor_count_down:			cursor = kThemeCountingDownHandCursor;				break;
>-			case eCursor_count_up_down:		cursor = kThemeCountingUpAndDownHandCursor; 	break;
>-
>-	  }
>-	  if (cursor >= 0)
>-	  {
>-	  	if (cursor >= 128)
>-	  	{
>-        nsMacResources::OpenLocalResourceFile();
>-        CursHandle cursHandle = ::GetCursor(cursor);
>-        NS_ASSERTION ( cursHandle, "Can't load cursor, is the resource file installed correctly?" );
>-        if ( cursHandle )
>-          ::SetCursor(*cursHandle);
>-        nsMacResources::CloseLocalResourceFile();
>-			}
>-			else
>-	  		::SetThemeCursor(cursor);
>-	  }
>   }
>-  else


Erk, diff hard to read here. diff -uw please?

>@@ -817,4 +756,69 @@
>   
> } // nsWindow::SetCursor
>+
>+void nsWindow::SetCursor(short aCursor) {

Brace on new line pls, here and below.

>+    if (aCursor >= 0)
>+    {
>+        if (aCursor >= 128)
>+        {
>+            nsMacResources::OpenLocalResourceFile();
>+            CursHandle cursHandle = ::GetCursor(aCursor);
>+            NS_ASSERTION ( cursHandle, "Can't load cursor, is the resource file installed correctly?");
>+            if ( cursHandle )
>+            {
>+                ::SetCursor(*cursHandle);
>+            }
>+            nsMacResources::CloseLocalResourceFile();            
>+        }
>+        else
>+        {
>+            ::SetThemeCursor(aCursor);
>+        }
>+    }    
>+}
>+
>+CursorSpinner::CursorSpinner() :
>+    mSpinCursorFrame(0), mTimerUPP(nsnull), mTimerRef(nsnull)
>+{
>+   mTimerUPP = NewEventLoopTimerUPP(SpinCursor);
>+}
>+
>+CursorSpinner::~CursorSpinner()
>+{
>+    if (mTimerUPP) ::DisposeEventLoopTimerUPP(mTimerUPP);
>+    if (mTimerRef) ::RemoveEventLoopTimer(mTimerRef);
>+}
>+
>+short CursorSpinner::GetNextCursorFrame() {
>+    int result = 149 + mSpinCursorFrame;
>+    mSpinCursorFrame = (mSpinCursorFrame + 1) % 4;
>+    return (short) result;
>+}
>+
>+void CursorSpinner::StartSpinCursor() {
>+    OSStatus result = noErr;
>+    if (mTimerRef == nsnull)
>+    {
>+        result = ::InstallEventLoopTimer(::GetMainEventLoop(), 0, 0.25 * kEventDurationSecond,
>+                                         mTimerUPP, this, &mTimerRef);
>+        if (result != noErr) {
>+            mTimerRef = nsnull;
>+            nsWindow::SetCursor(149);
>+        }
>+    }
>+}
>+
>+void CursorSpinner::StopSpinCursor() {
>+    if (mTimerRef)
>+    {
>+        ::RemoveEventLoopTimer(mTimerRef);
>+        mTimerRef = nsnull;
>+    }
>+}
>+
>+pascal void CursorSpinner::SpinCursor(EventLoopTimerRef inTimer, void *inUserData) {
>+    CursorSpinner* cs = reinterpret_cast<CursorSpinner*>(inUserData);
>+    nsWindow::SetCursor(cs->GetNextCursorFrame());
>+}
> 
> #pragma mark -
>Index: nsWindow.h
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/mac/nsWindow.h,v
>retrieving revision 1.94
>diff -u -2 -r1.94 nsWindow.h
>--- nsWindow.h	27 Feb 2003 23:31:54 -0000	1.94
>+++ nsWindow.h	19 Apr 2003 03:53:34 -0000
>@@ -75,4 +75,18 @@
> } TRectArray;
> 
>+class CursorSpinner {
>+public:
>+    CursorSpinner();
>+    virtual ~CursorSpinner();

No need for virtual dtor.

>+    void        	StartSpinCursor();
>+    void        	StopSpinCursor();    
>+private:
>+    short mSpinCursorFrame;
>+    EventLoopTimerUPP 	mTimerUPP;
>+    EventLoopTimerRef   mTimerRef;
>+    
>+    short				GetNextCursorFrame();
>+    static pascal void  SpinCursor(EventLoopTimerRef inTimer, void *inUserData);
>+};

Spacing looks odd. Tabs?

> //-------------------------------------------------------------------------
>@@ -181,5 +195,6 @@
>     
>     NS_IMETHOD        SetCursor(nsCursor aCursor);
>-    
>+    static void       SetCursor(short aCursor);
>+

I'd prefer a different method name, clarifying the difference.

>     NS_IMETHOD        CaptureRollupEvents(nsIRollupListener * aListener, PRBool aDoCapture, PRBool aConsumeRollupEvent);
>     NS_IMETHOD        SetTitle(const nsString& title);

Comment 16

[Andrew Thompson]

Attachment: Changes to patch to cover Simon's comments

I tried adding -w to the cvs diff, but it made a much worse, all sorts of
unrelated code changes got conflated into a real mess :)

I think the bit that's confusing you in the patch is the part where I removed a
chunk of code which was designed to do the cursor setting on
pre-AppearanceManager systems (ie, System 7!) I figured we didn't need that any
more. Take a look at nsWindow::SetCursor() as it stands now and you'll soon
figure out which bit I took out.

Comment 17

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 121067 [details] [diff] [review]
Changes to patch to cover Simon's comments

+    int result = 149 + mSpinCursorFrame;

a constant would be nice. i know the original code didn't have one, but
still...

how about doing this feature (and the code cleanup) for the cocoa ChildView
class as well? we don't want the two to get any more out of sync in terms of
functionality than they need to.

r=pink

Comment 18

[Andrew Thompson]

>(From update of attachment 121067 [details] [diff] [review])
>+    int result = 149 + mSpinCursorFrame;

>a constant would be nice. i know the original code didn't have one, but
>still...

OK. I'll add one before I checkin.


>how about doing this feature (and the code cleanup) for the cocoa ChildView
>class as well? we don't want the two to get any more out of sync in terms of
>functionality than they need to.

Sure. Planned to do that anyway. I need to learn to use NSTimer in any case.
I've never built Chimera however. Do I need a seperate tree or can I build ontop
of a Mozilla debug tree?

Comment 19

[Andrew Thompson]

Attachment: Version of spin cursor patch for Camino (widget/src/cocoa/)

Comment 20

[Mike Pinkerton (not reading bugmail)]

+- (id) init;
+- (void) dealloc;

you don't need to list inherited methods in the decl.

+  if ( gCursorSpinner == nsnull)
+  {
+    gCursorSpinner = [[CursorSpinner alloc] init];
+  }

in an embedding context, this will never go away, will it? how will the embeddor
clean up if they don't want gecko around anymore?

+- (id) init
+{
+  self = [super init];
+  return self;
+}

un-necessary.

+  if (mTimer != nsnull)

if (mTimer)

+  if ( mTimer == nsnull)

if (!mTimer)

+    mTimer = nsnull;  

should probably stick with cocoa stuff using |nil| rather than nsnull. doubt it
matters, but...

Comment 21

[Simon Fraser [no longer active]]

I think we should eliminate the use of resources in Cocoa, and load the cursors
from images instead.

Comment 22

[Andrew Thompson]

Switch to NSCursor you mean?

Yeah, I hadn't noticed that. That'd be cool.
I'll file a seperate bug since this is really doing double time as it is. Can
you sr= the patch for Mozilla so I can check in the Carbon version please?

Comment 23

[Simon Fraser [no longer active]]

Comment on attachment 121067 [details] [diff] [review]
Changes to patch to cover Simon's comments

We open the resource file ever time we switch the cursor. I think with an
animating cursor, this will be too expensive; it will hit the disk each time.

Comment 24

[Simon Fraser [no longer active]]

Comment on attachment 121067 [details] [diff] [review]
Changes to patch to cover Simon's comments

OK, I was mislead by the nsMacResources method names, grumble.

Comment 25

[Andrew Thompson]

Fix checked into Mozilla Trunk for 1.5/Firebird
Fixed in Nightly 2003052703

Comment 26

[Chris Petersen]

This looks like it may have can a regression in the Camino build.

http://bugzilla.mozilla.org/show_bug.cgi?id=207301

Comment 27

[timeless]

Comment on attachment 121067 [details] [diff] [review]
Changes to patch to cover Simon's comments

I think this would be a good thing to land on the long lived 1.4 branch. The
landing would include the changes in bug 207301.

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 121067 [details] [diff] [review]
Changes to patch to cover Simon's comments

a=asa (on behalf of drivers) for checkin to the 1.4 branch. We'll also need to
get the fix at 207301 with this as well.

Comment 29

[sairuh (rarely reading bugmail)]

looks good with the 2003.05.30.08 trunk build (OS X 10.2.6).

when checked into the 1.4 branch, pls add the fixed1.4 keyword. thanks!

Comment 30

[sairuh (rarely reading bugmail)]

checked into 1.4 at 07:19 today.

Comment 31

[sairuh (rarely reading bugmail)]

weird, this doesn't seem to be working with the 2003.06.09.05-1.4 (commercial
branch) build on OS X (10.2.6).

is this working for anyone using today's *branch* mozilla build?

Comment 32

[Andrew Thompson]

I see the problem. Is should have checked in nsMacWidget.r too. Will do ASAP

Comment 33

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2003.06.10.05-1.4 comm bits, and carrying over trunk vrfy from
comment 29.

Comment 34

[Kathleen :Brade]

it appears that this bug caused the regression for bug 211462 (Crash turning on
JA IME on Mac 10.1)

lordpixel--can you please look at bug 211462 and this patch and see if you can
fix it?