63640 - Mouse cursor are not working under BeOS

Status: VERIFIED FIXED

(Core :: XUL, defect, P2)

Description

[Yannick Koehler]

need to implement nsWindow::SetCursor.  Should be fairly easy.  Logging a bug to remind.

Comment 1

[cls]

Attachment: Implement select (I_BEAM) cursor. Does BeOS have any other default cursors or will we have to make some?

Comment 2

[:deanis74]

Looks like there are only two, B_HAND_CURSOR (B_CURSOR_SYSTEM_DEFAULT) and
B_I_BEAM_CURSOR.  See:

http://www-classic.be.com/documentation/be_book/The%20Application%20Kit/Applicat
ion.html#SetCursor()

I guess any other cursors will have to be drawn by hand...?

Comment 3

[Yannick Koehler]

There is a other, I had implemented it some time ago... I will check it back 
but I know there's more than the I BEAM.  Check the header files.

Comment 4

[Yannick Koehler]

the patch is also wrong, it should keep the switch structure not put it in 
comment.  Also the lock/unlock should be done after the switch.  A bool should 
be used to see if the cursor changed and if so then call lock/setcursor/unlock.

Comment 5

[cls]

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

Comment 6

[cls]

The switch was commented out because I didn't want to bother setting all of
those entries to SYSTEM_DEFAULT.  I checked the header files and all I saw was
SYSTEM_DEFAULT & I_BEAM.  I checked BeBook and it only had those listed as well.

Comment 7

[:deanis74]

There's another variation in the attachment to bug 65246.

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=22481

Comment 8

[Yannick Koehler]

Attachment: Patch including a lot more cursor

Comment 9

[Takashi Toyoshima]

Attachment: revised patch of 22638

Comment 10

[Takashi Toyoshima]

thanks for nice cursors!!

But the 22638 patch forgot to set the mCursor.
So sometimes cursor was not changed to default.

Other changes from 22638 might be a problem of a taste.

Comment 11

[Yannick Koehler]

Takashi,

True, I'm not an artist and not aspiring to become one.  That patch is not in 
the tree yet, if you have better cursor please do send them.  I'll fix the 
thing you're mentioning.

And don't forget that  16x16 one color is Small...

Comment 12

[Takashi Toyoshima]

I'm sorry Yannick.
I'm afraid of that my poor expression cause a misunderstanding...
Nice cursors, it's not sarcasm.

Main change of 22638 is the following additional line only.
>mCursor = aCursor;
But there are other changes in 22638.
I change BCursor constructions style from
>static const BCursor *B_CURSOR_HYPERLINK = new BCursor(cursorHyperlink);
to
>#define B_CURSOR_HYPERLINK &bCursorHyperlink
>static const BCursor bCursorHyperlink(cursorHyperlink);

But I dont't know which style is better.
This is the point I wanted to mean with the word 'taste'.
It never mean the problem of design.

Comment 13

[Yannick Koehler]

Ha ;-)  Well you still have the right to dislike my pointer ;-)  
I think your way is better as I don't have to do if(...) {}

Comment 14

[Yannick Koehler]

Attachment: corrected patch

Comment 15

[Yannick Koehler]

Marking as resolved.  patch commited.  missing cursor would be treated in other
bug if they are needed.  All essentials cursors implemented

Comment 16

[cls]

This checkin has stopped the app from working for me.  Still trying to track
down why.

Comment 17

[Makoto Hamanaka]

I heard that BApplication should be initialized before 
the excution of BCursor's constructor.
(I heard this from toyoshima..)

In current patch, cursors are created like this.
>static const BCursor bCursorAlias(cursorAlias);
So, the BCursor's constructor will be executed before
initialization of BApplication.

Isn't it the cause of error?

Comment 18

[cls]

That sounds like it.  For now, I've backed out the changes to nsWindow.cpp and you guys can hash out the proper method for fixing this problem.

Comment 19

[Yannick Koehler]

Does it crash or just doesn't start?

To my memory when I had created the cursor before the BApp it actually crash... 
That's also why I had put them in the function in the first place.  Will change 
that.  

Sorry...

Comment 20

[Yannick Koehler]

Chris,

Is this last patch ok?

Comment 21

[Yannick Koehler]

Attachment: Patch for Cls problem

Comment 22

[cls]

Before it was just crashing and printing 'Kill thread' in the terminal window. 
With the latest patch (attach 23130), it crashes and brings up the debugger. 
Here's the trace:

loading symbols
You need a valid BApplication object before interacting with the app_server
_debugger:
_debugger:
+0007  ec084853:   *            c3    retn
mozilla-bin:sc
   frame         retaddr
fcfff194   ec16256d  _BAppServerLink_::_BAppServerLink_(void) + 0000005d
fcfff1a8   ec163640  BCursor::BCursor(void const *) + 00000048
fcfff1c4   ec901081  global constructors keyed to nsWindow::nsWindow(void) +
00000025
fcfff1d8   ec907b66  __do_global_ctors_aux + 00000026
fcfff1e8   ec8ee341  ._init + 0000001c
fcfff1f4   ec083eae  call_routine_in_order + 0000012e
fd00065c   ec083feb  load_add_on + 000000eb
fd000ab0   ed96698a  #File libnspr4.so text + 0000e98a
fd001320   ed96684a  #File libnspr4.so text + 0000e84a
fd001334   ed966882  #File libnspr4.so text + 0000e882
fd00135c   ed905b80  nsDll::Load(void) + 000000c0
fd001384   ed8fe56c  nsNativeComponentLoader::GetFactory(nsID const &, char
const *, char const *, nsIFactory **) + 000000d4
fd0017b4   ed8fb243  nsComponentManagerImpl::FindFactory(nsID const &,
nsIFactory **) + 000000f3
fd001800   ed8fb601  nsComponentManagerImpl::CreateInstance(nsID const &,
nsISupports *, nsID const &, void **) + 00000061
fd001828   ed902ee9  nsComponentManager::CreateInstance(nsID const &,
nsISupports *, nsID const &, void **) + 00000045
fd00184c   edb06309  nsAppShellService::Initialize(nsICmdLineService *,
nsISupports *) + 000001a5
fd0018b0   8000916a  main1(int, char **, nsISupports *) + 00000792
fd001a08   8000991b  main + 0000011b
fd001a34   80006695  _start + 00000061
mozilla-bin:                              

Comment 23

[Takashi Toyoshima]

I suggest the move of implementation of BApplication.
It's required by splash window too.
(please see http://bugzilla.mozilla.org/show_bug.cgi?id=65425 )
With this patch, 23130 seem to work certainly.

The trace of chris say we should create BApplication instance
previous to constructors of BCursor.

Comment 24

[Yannick Koehler]

I still don't get it...  If it was the case I would have get the same crash on 
my machine... how can the BApplication be created in different order based on 2 
computers.

nsAppShell is the first thing to get created (disregard splash screen)... why 
is it that it fail...  

Comment 25

[Makoto Hamanaka]

Yannick,
maybe you shold provide revised version of nsBeOSCursors.h ?

It seems that your revised nsWindows.cpp has local BCursor's but 
also current nsBeOSCursors.h has its own definition of global BCursor's .

Comment 26

[Yannick Koehler]

Attachment: Corrected file (try again cls, please)

Comment 27

[Yannick Koehler]

Chris,
  Can you retry with attachment 23130 [details] [diff] [review] and 23239.

Comment 28

[cls]

Ok, the changes from 23130 & 23239 work for me.  Nice cursors. ;-)

Comment 29

[Paul]

I have created a new patch for this bug.  There were some comments about some of
the cursors by the BeOS community
and I thought this would be an easy project for me to start with.  But, instead
of just chaning the icons, I also animated
the spinning cursor icon I created.  The code I wrote to do this should really
be reviewed, as this is the first work that I have
done for mozilla.  I attached the AnimateBeOSCursor class I created to nsWindow,
but it seems that multiple nsWindows are
created for one "physical" BWindow.  From what I can tell, the nsWindow
represents both BWindows and BViews.  So, for each
window/view, it is possible that a spinningCursor thread may be started, even if
it is not active very long.  The thread remains 
until the nsWindow object is deleted.  There is probably a better way to
implement this, I just did not see it at my first look at
the code.  Though, this implementation is better than me trying to attach the
AnimateBeOSCursor object to the nsViewBeOS!

Anyway, that patch is below.  It is a zip file containing the patch to nsWindow,
a new nsBeOSCursors.h, and a new class, 
AnimateBeOSCursor.

Also, if you email me about this, I might not get back to you right away, as I'm
going away for a week. 

Comment 30

[Paul]

Attachment: Zip file with new cursor icons, and a class to animate the spinning cursor

Comment 31

[Andrew Thompson]

Attachment: The custom cursors Moz defines on Mac OS (PNG image)

Comment 32

[Andrew Thompson]

Timeless asked me to attach that image, I guess as a reference to how custom 
cursors look on a platform with a similar visual cursor style?

Comment 33

[Yannick Koehler]

No more working on Bezilla

Comment 34

[Paul]

The cursors I submitted are close to those on the mac.  The main difference is
that the default beos cursor
is a hand, where on a mac it is an arrow.  So, from the picture posted, the only
cursor that I don't have something
"beos" like for is 146, which I'm guessing is to indicate a drop down list
(eCursor_context_menu), which I will add and
update the patche for.  Also, I don't see a difference between 142 and 147.  I
will also make an eCursor_grab cursor,
which will be an open hand, while I already have an eCursor_grabbing, but I
think I'm currently rendering as the
eCursor_grab.  I'll fix that also.

On another note, someone mentioned putting the cursors into a resource file. 
There currently is a resource file, but 
it is also changed in another patch that has not been submitted yet.   Also,
that patch registers mozilla as a bappl.ication
at a different point in the startup process, IIRC, to be able to get to the
resources in the file.  If this is true, I don't want to 
move the cursors to the resource file, until that patch is comitted.  If I'm
wrong on this, let me know.

Comment 35

[timeless]

146 is needed for mac because macs have one button so people press a key 
(option?) which changes the behavior of clicking (popup menu).  Given BeOS 
doesn't really behave like this i don't think it's necessary.

Yannick decided to use .h instead of a resource file because it allows for 
cvsblame.  Unless there's a real need to use a resource file I think I like the 
benefits of cvsblame.

Thanks for volunteering to work on this.

Comment 36

[Paul]

Attachment: Change grab cursor to grabbing. Added grab cursor. Added Makefile.in to patch file.

Comment 37

[timeless]

some quick comments:
please use |if {| consistently (you have at least one instance of |if(|).
please don't use tabs in code files (I think 2 spaces is correct, not sure... 
-- see modeline at top of file).
generally, if (cond) return ...; should be split across two lines to aid 
debugging.

+//	commented code doesn't make sense.
probably use one of
+#if 0
+#ifdef DEBUG_baron
(whatever whoami returns)
  live code+=which-wont*get/executed;
#endif

for the final patch, don't #include <stdio.h> or fprintf...

this just doesn't look right
	   if (&view && view->LockLooper())

bool is a native type, the correct values are true and false. please be 
consistent when using non NSPR types.  

this looks like a for loop
    int32 i = 0;
    while( i < qty )
    {
      i++;
    }

contributed code should reference MPL not NPL. The fact that Yannick or someone 
else was careless when they wrote other pieces of the BeZilla code doesn't 
justify perpetuating the problem. http://www.mozilla.org/MPL/boilerplate-1.1/

Comment 38

[Paul]

Attachment: fixed tabs, license, and code consistency. let me know if there is more i should clean up

Comment 39

[Paul]

Attachment: The patch file in above zip file, also contained the "window grippy" patch. This patch does not.

Comment 40

[Paul]

Attachment: Mouse cursor patch - no animation

Same icons in previous patch, just no animation.  The animation code created
multiple threads, and could use some work, so it's been removed.
I need an r= on this to get it into the tree and closed.

Comment 41

[:deanis74]

Your patch removes the triple license in nsBeOSCursors.h.  Any reason?

http://mozilla.org/MPL/relicensing-faq.html

Comment 42

[Paul]

Oops, sorry about that.  I had just saved the file from before, and ran the diff
against that.  I would definitely put the new triple license in.  If need be, I
can do this, and post a new patch.

Comment 43

[Paul]

Attachment: Patch with triple-license reinstated.

This is a non-animate cursor patch.  Thought the novelty of an animated
spinninig cursor is nice, it used native beos threads, with a new thread
created for every view, which was not ideal.  This just gives mozilla under
BeOS the different cursors it needs.  I left the extra cursor definitions used
for the spinning cursor in the nsBeOSCursors.h file, for possible future use.

Comment 44

[Paul]

Marking Assigned

Comment 45

[Paul]

Attachment: udpated patch that applies without errors

Can I get an r= on this?  Or, do I not need one, since this is BeOS specific?

Comment 46

[Paul]

Adding Daniel to CC list, since he will be working on the widget code.

Comment 47

[Daniel Switkin]

The last patch looks good, let's get it checked in. r=[e-mail]

Comment 48

[Paul]

The last patch has been checked in.

Comment 49

[Kai Lahmann (is there, where MNG is)]

v