The default bug view has changed. See this FAQ.

Patch to enable inline input in Japanese on BeOS

RESOLVED FIXED

Status

()

Core
Internationalization
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: Jorge G. Mare (a.k.a. Koki), Assigned: Sergei Dolgov)

Tracking

({fixed1.8.1, intl})

Trunk
x86
BeOS
fixed1.8.1, intl
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 10 obsolete attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.7b) Gecko/20040412 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.7b) Gecko/20040412 Firefox/0.8.0+

One of the main inconveniences of using Mozilla/Firefox in a Japanese
environment in BeOS is that it is not possible to enter text inline. This means
that when you switch to Japanese input, text is not entered directly into the
Mozilla window, but instead through a little pop-up window. Although this may
arguably be not a critical shortcoming, it is a quite annoying one.

The good news is that Rihatsu-san at JPBE.net has come up with a way to fix
this, and he has asked me to share it with the Bezilla development team in the
hope that his proposal is merged into the source tree of Mozilla.

Attachment ime.txt contains a rough translation of the solution that Rihatsu-san
posted at JPBE.net (http://jpbe.net/forum/viewtopic.php?p=5867#5867).

Attachment ime.zip has the sources (believe this is from the 1.6 trunk). The
added lines are identified by the comment "// Ryeha2" at the end of the line.

I hope you can understand and can make use of this. If not, feel free to ask and
I will convey your questions to Rihatsu-san. Also, if you need any testing of
binaries in BeOS, just let me know; I have a full team eager to test this
further and report problems.



Reproducible: Always
Steps to Reproduce:
N/A
Actual Results:  
Japanese input is done through an IME popup window.

Expected Results:  
With this patch, hopefully, it will be possible to enter Japanese directly into
the Mozilla window in BeOS.
(Reporter)

Comment 1

13 years ago
Created attachment 147479 [details]
English rough translation Rihatsu-san's solution

English rough translation Rihatsu-san's solution posted at
http://jpbe.net/forum/viewtopic.php?p=5867#5867 (this is in Japanese).
(Reporter)

Comment 2

13 years ago
Created attachment 147480 [details]
Sources containing patch to enable Japanese inline input

Attachment ime.zip has the sources (believe this is from the 1.6 trunk). The
added lines are identified by the comment "// Ryeha2" at the end of the line.
jshin, could you take a look at this?  If not, who'd be the right person to?

Comment 4

13 years ago
Adding a few people who might be able to help. 
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: intl
(Reporter)

Comment 5

13 years ago
Created attachment 147596 [details] [diff] [review]
Diff file

Rihatsu-san created this patch by loging in to
anonymous@cvs-mirror.mozilla.org:/cvsroot and using the following command:

cvs diff -uwp nsWindow.cpp nsWindow.h > nsWindow.cpp.patch

Thought this diff would be better than the full files previously posted for
this bug.

Koki

Comment 6

13 years ago
Thanks for the diff, but it would be even better to remove commented lines in
the patch and 'diff -u8 -p' would be better
(Reporter)

Comment 7

13 years ago
Created attachment 147628 [details] [diff] [review]
 'diff -u8 -p' patch with all comments & printf removed

As requested, posting an edited 'diff -u8 -p' patch with all comments removed.
Per the author (Rihatsu-san), all unnecessary printf have also been removed.

Comment 8

13 years ago
Comment on attachment 147628 [details] [diff] [review]
 'diff -u8 -p' patch with all comments & printf removed

> +void nsViewBeOS::DoIME(BMessage *msg)
...
> +uint32 *args = new uint32[argc];
that allocation could fail
> +msg->Flatten((char*)args, size);
then this would crash
or at least, that's the theory

> +BezillaIME::RunIME(uint32 *args, nsWindow *target, BView *view)
...
> +mText  = new PRUnichar[mCount+1];
  ^^ not checked
> +cutf16(src, &srcLen, mText, &dstLen);
would crash if mText allocation failed

while utf8_str_size / utf8_str_len / cutf16 look ok to me, i want someone else
to concur (smontagu, jshin, ...). Also, the name |cutf16| is a bit strange,
something more verbose might be better. as an English speaker, i first parsed
it as |cut,f,16| and got confused.
(In reply to comment #8)

> while utf8_str_size / utf8_str_len / cutf16 look ok to me, i want someone else
> to concur (smontagu, jshin, ...). 

Before looking in detail I would like to understand why you need to roll your
own routines instead of using the ones in xpcom/string/
momoi-san, can you help with this? If you could contact Rihatsu-san directly
about the UTF-8 conversion, it would be great.
(Reporter)

Comment 11

13 years ago
This is an attempt to convey Rihatsu-san's responses (at
http://jpbe.net/forum/viewtopic.php?p=6149#6149).

*********
> Also, the name |cutf16| is a bit strange...
I am a bit at a loss as to what would be acceptable/appropriate for an English
speaker. Perhaps someone could recommend a good name here?

> why you need to roll your own routines instead of using the ones in xpcom/string/
I simply did not know how to use xpcom/string.

In the setion cutf16, change to mText = ToNewUnicode(NS_ConvertUTF8toUCS2(src));
delete[] mText can be changed to nsMemory::Free(mText);

Not sure what I will do with utf8_str_len and utf8_str_size yet, so this is
on-hold (until I figure it out?).

***********

I hope this rough "translation" makes some sense...
It definitely does.  I'd actually recommend using an nsString for mText.  Then
the code can become:

  CopyUTF8toUTF16(src, mText);

where mText is currently allocated, mText.get() for places where we need a
PRUnichar*, mText.Truncate() when we want to clear the string, et.

Alternately, you could keep mText as-is and instead of

  mText = ToNewUnicode(NS_ConvertUTF8toUCS2(src));

use

  mText = UTF8ToNewUnicode(src);
(In reply to comment #11)

> Not sure what I will do with utf8_str_len and utf8_str_size yet, so this is
> on-hold (until I figure it out?).

For utf8_str_len I think you can #include "nsUTF8Utils.h"and use
CalculateUTF8Length, like at
http://lxr.mozilla.org/seamonkey/source/xpcom/string/src/nsReadableUtils.cpp#230
(Boris or Jungshik, please correct me if I am wrong).
utf8_str_size is anyway only used internally by cutf16 and utf8_str_len, right?
(Reporter)

Comment 14

13 years ago
(In reply to comment #13)
> For utf8_str_len I think you can #include "nsUTF8Utils.h"and use
> CalculateUTF8Length, like at

Reply from Rihatsu-san (at http://www.jpbe.net/forum/viewtopic.php?p=6170#6170).

*********
I was able to verify that CalculateUTF8Length works, so I plan to use it.
However, I always get a "always_inline' attribute directive ignored" warning: is
this attribute required?

Also, in the code for [CalculateUTF8Length] the following appears:

else if ( UTF8traits::is4byte(*p) ) {
p += 4;
++mLength;
}

I cannot understand this, so I am a bit reluctant (to use it?).
********
(Assignee)

Comment 15

13 years ago
in mozilla/widget/gfx/beos/nsRenderingContextBeOS.cpp we use macros and inlines
for utf-8 processing for speed purpose. Borrowed from old BeNewsLetters.
You can look there, though, i don't think that code in widget has some speed
requirements, so probably better is to use regular mozilla tools
(Assignee)

Comment 16

13 years ago
Sorry, wrong path in previous mesage. Must be:
mozilla/gfx/src/beos/nsRenderingContextBeOS.cpp
(In reply to comment #14)

> Also, in the code for [CalculateUTF8Length] the following appears:
> 
> else if ( UTF8traits::is4byte(*p) ) {
> p += 4;
> ++mLength;
> }
> 
> I cannot understand this, so I am a bit reluctant (to use it?).

It's very bad that this isn't documented properly in the file. The "length of a
UTF-8 string" calculated by CalculateUTF8Length is not the number of codepoints
in the string but the length of the UTF-16 string with the same codepoints.
Because a UTF-8 sequence of 4 bytes represents a codepoint greater than 0xFFFF,
it will become a surrogate pair in the UTF-16 string, hence |++mLength|.

This doesn't happen with is5byte and is6byte because these are illegal UTF-8
sequences (greater than 0x10FFFF) so get converted to a single replacement
character.

I hope this is clear.
(Assignee)

Comment 18

13 years ago
I have some questions to Rihatsu-san.
1)What for is B_NAVIGABLE flag in our case?
2)Why not to set both fags B_NAVIGABLE|B_INPUT_METHOD_AWARE at nsViewBeOS
creation but in MakeFocus? Does setting those falgs at creation affects badly
some other aspects?
3)What happens if you just set _B_INPUT_METHOD_AWARE flag, but don't add any
code, and try to type japanese characters? Some screenshots, pls, is there is
any efect.
4)BeNewsletter says that with B_INPUT_METHOD_AWARE flag events receive BView on
one of two ways - via B_INPUT_EVENT message and via BView:KeyDown(). What happens 
 in KeyDown() in your code? Does it receive anything too? Or getting
B_INPUT_EVENT message in MessageReceives prevents call of that KeyDown() hook?

5)Did you estimate possibility to implement IME in way like it is done with that
little proxy window but inside BeOS widget code? I mean - process whole message
inside nsViewBeOS method and call there BView::KeyDown/KeyUp functions (or post
BMessages  which will result in calling KeyUp/Down()).

P.S. That's very sad Rihatsu-san don't speak nor English neither Russian. Ehheh,

P.P.S Do you know any article about BeOS input method usage besides only article
i found in Volume III, Issue 7, February 17, 1999 of Be Newsletter?
(Reporter)

Comment 19

13 years ago
(In reply to comment #18)
> I have some questions to Rihatsu-san.

I translated the questions into Japanese and posted it on the JPBE.net forum for
Rihatsu-san to see and hopefully answer.

http://www.jpbe.net/forum/viewtopic.php?p=6861#6861

If he replies, I will post his answers in English here.

Koki
(Reporter)

Comment 20

13 years ago
(In reply to comment #19)
> (In reply to comment #18)
> > I have some questions to Rihatsu-san.
> I translated the questions into Japanese and posted it on the JPBE.net forum 
for
> Rihatsu-san to see and hopefully answer.
> http://www.jpbe.net/forum/viewtopic.php?p=6861#6861
> If he replies, I will post his answers in English here.
> Koki

Here is a translation of Rihatsu-san's reply. I am afraid that this 
translation may not be very acurrate, as honestly the content is very 
technical for me. I am posting it nevertheles in the hope that it could be of 
some use. Here I go.

*******
1) I added it since, according to the Developer's Guide Vol. 1, this is a flag 
that indicates that BView can be the focus view for a keyboard operation, and 
therefore a view for key input was needed. It is possible to enter text 
without this flag, so maybe it is not required.

2) This is to have the least effect on the original (code?). With this, 
MakeFocus is a false view as in the original (code?). If added during 
creation, everything will make this flag constantly in an ON status. Setting 
the flag during creation does not affect other aspects. (Note: these last two 
sentences sound contradicting, so I am not sure if they make sense.)

3) Nothing would happen. So, I cannot send you a screenshot of "nothing 
happening". :-)

Raising the B_INPUT_METHOD_AWARE is just to indicate that the following 
messages will be processed. If you just indicate but do not do anything, 
nothing would happen. In that case, the BMessage would be discarded.

4) It is not in one of to ways, but actually from both ways. It comes from 
both B_INPUT_EVENT as well as from BView:KeyDown(), so it is actually telling 
you to process both without failure. Of course, the content is different. What 
this means is that the receiving side cannot decide whether to process only 
B_INPUT_EVENT or to have everything sent via BView:KeyDown(). Which way it 
comes is decided by the module (InputMethod) originating from 
BInputServerMethod. 
 
5) (Note: I may not have translated your question properly, so it would not be 
surprising if his answer is a bit off). The inline implementation is in the 
View side. In other words, all messages are processed in nsViewBeOS, and 
BView::KeyDown/KeyUP functions are called (or  BMessages are used to call 
KeyUp/KeyDown()).

I am not sure what you are trying to ask, but I will try to answer anyway.

IME is a mechanism to implement inline (input). This displays the text that 
has been entered or converted, but it is still not considered to be an input.

For example, let's say that the letter A is received via B_INPUT_EVENT. This 
is still not an input, so it cannot be processed using BView::KeyDown/KeyUp. 
But it still has to be displayed. If you do not display the letter, the person 
typing will (mistakenly) think that the letter that he/she typed was not 
received. So, can we display the letter A in insert mode in the current cursor 
position from nsViewBeOS? I am sure you understand this better than me, but to 
the extent of my knowledge, the operation and information for displaying is in 
nsWindow.cpp. You may be able to take that information and operate it in 
nsViewBeOS, I do not think there is any advantage in doing so. WIN32, OS2 and 
GTK they all process IME in nsWindow. I cannot think of any reason that would 
merit processing INE in nsViewBeOS.

The converted entry can be processed via BView::KeyDown/KeyUp. However, having 
BView::KeyDown/KeyUp recieve again what has already been received does not 
make much sense to me.
***********

Comment 21

13 years ago
Adding Nakano-san to Cc who may or may not have BeOS around. Even if he doesn't,
 he may be of help in moving this forward.
Sorry, my system and my knowledge of IME programming are Windows only.

Comment 23

13 years ago
Created attachment 163538 [details] [diff] [review]
Updated patch

This is an updated patch. It should apply cleanly to AVIARY, and it will do
that for HEAD as well. I have made one change though: nsCompositionEvent
doesn't have a 'compositionMessage'  (anymore). Will it work this way as well?

Niels

Updated

13 years ago
Blocks: 266252

Comment 24

13 years ago
Created attachment 163748 [details] [diff] [review]
Newest patch as Koki pointed to me

Newest patch which will be included in build 1.0.0-d3 .

BTW, could someone either make me QA contact or assign this bug to me, or
alternatively simply obsolete the previous patches.

Thanks.

Updated

13 years ago
Attachment #163538 - Attachment is obsolete: true

Comment 25

13 years ago
Regarding comment 18 , Sergei are all your concerns addressed at this point?

Comment 26

13 years ago
Created attachment 163757 [details] [diff] [review]
Properly whitespaced patch

Sorry to bug you (yet again). This time I attached a properly whitespaced patch
(in my opinion). The only thing I like to see changed is the name of the class
(BeZillaIME), which should become nsIMEBeOS, IMO. Your opinions? I'll be doing
a build tomorrow with this patch included for testing.

Updated

13 years ago
Attachment #163748 - Attachment is obsolete: true
(Assignee)

Comment 27

13 years ago
Niels, in this file we use (at least try to use consistently) GNU style for
btackets.
foo1
{
 foo2
 {
 }
}

Agree about names. "Bezilla" looks like argo.

Comment 28

13 years ago
Okay, I am changing all the BezillaIME into nsIMEBeOS (which isn't particularly
esthetically pleasing, but it should do). 

Another thing I'm not sure about is the global variable be_IME. Shouldn't this
become a static member accessible by a static function of the nsIMEBeOS class?

For the more knowledgeble among us, is the patch code-wise correct?
(Reporter)

Comment 29

13 years ago
(In reply to comment #28)
> Okay, I am changing all the BezillaIME into nsIMEBeOS (which isn't particularly
> esthetically pleasing, but it should do). 
> 
> Another thing I'm not sure about is the global variable be_IME. Shouldn't this
> become a static member accessible by a static function of the nsIMEBeOS class?
> 
> For the more knowledgeble among us, is the patch code-wise correct?

Here is a message from Rihatsu-san...

********
I have no objections about changing BeZillaIME to nsIMEBeOS.
I chose to use the BeZillaIME class as a way to reduce as much as possible the
number of changes in nsWindow. In reality, it would be better to make it a
member of nsWindow like in the other platforms. Plus, since BeZillaIME is a
class, it means that it duplicates functions from nsWindows, which is a waste.

BeZillaIME is a global class. It is the only one in Mozilla. If it were to be
made a member of nsWindow, then it would exist for every nsWindow. The way that
BeOS handles input methods currently does not require this; however, looking at
how the mozilla devs are testing other new input methods in Windows, it may be
wise to keep the class on a session basis.
*******

I hope that my translation makes sense. Feel free to ask if it does not. :-)
(Reporter)

Comment 30

13 years ago
(In reply to comment #28)
> Another thing I'm not sure about is the global variable be_IME. Shouldn't this
> become a static member accessible by a static function of the nsIMEBeOS class?

More more reply from Rihatsu-san...

******
If you make it global, the relationaship between nsWindow, be_IME and Input
Method becomes n:1:1. If you make it a static member, then the relationship
becomes n:n:1.

The problem is that each nsIMEBeOS holds the information related to Input
Method, but Input Method does not. Which means that if you change focus in the
middle of a session, then you may have a problem.

So, I think it is necessary to synchronize between the nsIMEBeOS of the nsWindow
that looses focus and the nsIMEBeOS of the nsWindow that gains focus.
********

Comment 31

13 years ago
Well, I don't actually mean a functional change. I rather mean a stylistic
change. Let's give the nsIMEBeOS class the following members:

public:
    static nsIMEBeOS *GetIME() 
    {
        if(mIME == 0)
        {
            mIME = new nsIMEBeOS();
        }
        return mIME;
    } 
private:
    static nsIMEBeOS *mIME = 0;


This would opt rather for a stylistical change as the be_IME global could be
deleted. It would in no way mean that the workings of the patch should be changed.

I just wonder what is stylistically preferable.
(Reporter)

Comment 32

13 years ago
(In reply to comment #31)
> Well, I don't actually mean a functional change. I rather mean a stylistic
> change. Let's give the nsIMEBeOS class the following members:
> 
> This would opt rather for a stylistical change as the be_IME global could be
> deleted. It would in no way mean that the workings of the patch should be changed.
> 
> I just wonder what is stylistically preferable.

Comment from Rihatsu-san...

********
I now understand that you meant to make it a static member of nsWindow. 
Yes, this is better than making it global.
********

Comment from Koki: it may be that my translation to Rihatsu-san was not clear
enough, in which case I apologize. I hope this moves us one step closer to
committing this patch. :-)

Koki

Comment 33

13 years ago
(In reply to comment #32)
> > I just wonder what is stylistically preferable.
> 
> Comment from Rihatsu-san...
> 
> ********
> I now understand that you meant to make it a static member of nsWindow. 
> Yes, this is better than making it global.
> ********

About the functional part: I think it works perfectly like this. If we only need
one communication object, why bother moving that into BWindow? Of course I don't
 have anything to say about this, but we'll feel the general consensus once it
is on for review.

> Comment from Koki: it may be that my translation to Rihatsu-san was not clear
> enough, in which case I apologize. I hope this moves us one step closer to
> committing this patch. :-)

Don't worry about it. I will have to see whether I have some time to fix it
today, otherwise it will be tomorrow morning. I'll put out a build as soon as
possible then. 
(Assignee)

Comment 34

13 years ago
setting Neils as owner. 
Assignee: smontagu → Niels.Reedijk

Updated

13 years ago
Status: NEW → ASSIGNED

Comment 35

13 years ago
Created attachment 164232 [details] [diff] [review]
Input patch without the global

Here's the input patch that is included in the development release of Firefox
1.0.0 - d3. If this one works, it will also be flagged for review...
Attachment #163757 - Attachment is obsolete: true

Comment 36

13 years ago
Comment on attachment 164232 [details] [diff] [review]
Input patch without the global

Please review this bug. It appears to work.
Attachment #164232 - Flags: review?(sergei_d)

Comment 37

13 years ago
Comment on attachment 164232 [details] [diff] [review]
Input patch without the global

Please review this bug. It appears to work.
(Assignee)

Comment 38

13 years ago
Comment on attachment 164232 [details] [diff] [review]
Input patch without the global

Starting polishing.
1)BTNCLCK: case in CallMethod - access to arguments must be performed AFTER
test for number of arguments

2)nsViewBeOS::MakeFocus()
Inspite reports say that patch is working, it is unclear for me, is it ok to
rely on "focused" argument BEFORE we set actual focus state

3)Question rather about style - for all other classes we've put decalration in
*.h file, so IME here appears to be exception - both decalration and
implementation are in *.cpp.
Is there any serious reason behind it?
Attachment #164232 - Flags: review-
(Assignee)

Comment 39

13 years ago
Comment on attachment 164232 [details] [diff] [review]
Input patch without the global

see previous comment
Attachment #164232 - Flags: review?(sergei_d)

Comment 40

13 years ago
(In reply to comment #38)
> (From update of attachment 164232 [details] [diff] [review])
> Starting polishing.
> 1)BTNCLCK: case in CallMethod - access to arguments must be performed AFTER
> test for number of arguments

Hmmm, definately true, I will change that.

> 2)nsViewBeOS::MakeFocus()
> Inspite reports say that patch is working, it is unclear for me, is it ok to
> rely on "focused" argument BEFORE we set actual focus state

I'll look to that. 

> 3)Question rather about style - for all other classes we've put decalration in
> *.h file, so IME here appears to be exception - both decalration and
> implementation are in *.cpp.
> Is there any serious reason behind it?

I think the main reason is that the class is only used in that cpp file. It
would be of no use to actually put it in a header. At the other hand, other
classes used in one file are also in headers, so it would be a question of
policy. As in this case, I don't think it would be needed to put the class
definition in a header. It's fine this way, and it doesn't polute headers.

I'll give it a swing when I have more time.

Niels
(Assignee)

Comment 41

13 years ago
3)class declaration inside *.cpp
If there isn't any special idea behind it, i think we should follow MS Win and
GTK example in that aspect. Look pls there.

Also, bit off-topic -looking at your patch i noticed my own old mistake in
CallMethod() code. There is ONWORKSPACE: case in switch, but it is wrong - must
be something like nsWindow::ONWORKSPACE  there. Can you file new bug on that?
I will quickly review it, and if tree is open, will try for check patch in
before i leave my home (will be in Sweden from 7-th to 21-th October). So new
IME patch will be maid against new code with that stupid bug fixed.

Comment 42

13 years ago
(In reply to comment #41)
> 3)class declaration inside *.cpp
> If there isn't any special idea behind it, i think we should follow MS Win and
> GTK example in that aspect. Look pls there.

The windows port has private classes as well. Check
/mozilla/widget/src/windows/nsWindow.cpp line 549 (nsAttentionTimerMonitor). So
we are following conventions here.

Comment 43

13 years ago
(In reply to comment #40)
> > 2)nsViewBeOS::MakeFocus()
> > Inspite reports say that patch is working, it is unclear for me, is it ok to
> > rely on "focused" argument BEFORE we set actual focus state

I don't think I really understand the problem here. You mean that the:

	if (focused)
		SetFlags(Flags()|B_NAVIGABLE|B_INPUT_METHOD_AWARE);
	else
		SetFlags(Flags() & ~(B_NAVIGABLE|B_INPUT_METHOD_AWARE));

should be after:
	if (!IsFocus() && focused)
		BView::MakeFocus(focused); ?
(Assignee)

Comment 44

13 years ago
>should be after:
I looked at code again.
Actually it may be where it is now, i confused it with nsWindow::SetFocus()
method, where we may reject request to set focus.

Though, i have now questions to Rihatsu-san again (sorry for my slowness:).

Fact:B_NAVIGABLE flag allows to set focus to given view by TAB key. 

Problem #1: We set that flag when we already got focus, and unset when focus is
lost. Is there any sense in that?

Problem #2:  We haven't that flag set in BView constructor, but switching focus
by TAB works anyway - Mozilla handles it by self. Do we really need it?

Problem #3:
 	case nsWindow::BTNCLICK :
+		if (((int32 *)info->args)[3] == 1) 
+			nsIMEBeOS::GetIME()->DispatchCancelIME();

Is it really proper trigger-event for DispatchCancelIME ? 
As BTNCLCK with clicks==1 may result in very various events - getting focus,
loosing focus, moving text caret, nothing.

Koki, are you watching this thread?


Comment 45

13 years ago
Created attachment 164930 [details] [diff] [review]
Patch for 1.0.0-d4
Attachment #164232 - Attachment is obsolete: true

Updated

13 years ago
Attachment #164930 - Attachment description: Rejected patch, but updated to latest cvs → Patch for 1.0.0-d4
(Reporter)

Comment 46

12 years ago
(In reply to comment #44)
> Koki, are you watching this thread?

Sorry fyysik. I dropped the ball on this one. Rihatsu-san had replied this long
back... Here is a translation of his reply. I hope this is useful.

*********
> Fact:B_NAVIGABLE flag allows to set focus to given view by TAB key. 

B_NAVIGABLE also indicates that BView can become a focus view for a keyboard
operation. In other words, if there is no B_NAVIGABLE, it means that it is not
possible to obtain focus view from a keyboard operation. That is how I am using
it. For the program, moving the focus with TAB is not important.

> Problem #1: We set that flag when we already got focus, and unset when focus is
> lost. Is there any sense in that?

It may not have any meaning, but it does not create a problem either. I do not
understand, however, why you ask this question. Is it about the SetFlags()
portion of MakeFocus()? Expressions such as "got focus" and "unset when focus is
lost" impply things happening in past tense, but note that when entering
MakeFocus() the focus itself has not changed yet. However, this may depend on
whether by "focus" you refer to the focus on Mozilla or that on BeOS.

> Problem #2:  We haven't that flag set in BView constructor, but switching focus
> by TAB works anyway - Mozilla handles it by self. Do we really need it?

I guess you are referring to B_NAVIGABLE. As mentioned in my reply to Problem
#1, if there is no B_NAVIGABLE, it means that it is not possible to obtain focus
view from a keyboard operation. Getting rid of this in Mozilla may not pose any
problems.

Problem #3:
 	case nsWindow::BTNCLICK :
+		if (((int32 *)info->args)[3] == 1) 
+			nsIMEBeOS::GetIME()->DispatchCancelIME();

Is it really proper trigger-event for DispatchCancelIME ? 
As BTNCLCK with clicks==1 may result in very various events - getting focus,
loosing focus, moving text caret, nothing.

I do not understand the purpose of your question? Are you suggesting that more
code be added to handle other (possible) events from BTNCLCK? nsIMEBeOS needs to
catch any focus moves. When you click, it is expected that the focus will move.
The purpose here is to catch the loss of focus. The number "1" has no particular
meaning. That is because a left click is the one used to move the focus. As long
as it is possible to catch the loss of focus, it is OK to have it in another
part (of the code).
*************

Updated

12 years ago
Attachment #147596 - Attachment is obsolete: true

Updated

12 years ago
Attachment #147480 - Attachment is obsolete: true

Updated

12 years ago
Attachment #147628 - Attachment is obsolete: true

Comment 47

12 years ago
Created attachment 196286 [details] [diff] [review]
updated patch for 1.6.1a

changed code to reflect changes in nsGUIEvent (bug 289940 and 296036).	Needs
eye of seasoned dev to see if patch correctly reflects nsGUIEvent updates.

Comment 48

12 years ago
Comment on attachment 164930 [details] [diff] [review]
Patch for 1.0.0-d4

Newer patch required.
Attachment #164930 - Attachment is obsolete: true

Comment 49

12 years ago
neilx,   With all the work touching nsWindow, it seems easier to let the
work-in-progress settle first, then update this patch.  At this point, I believe
only tqh's nsWindow cleanup under bug 296856 is still actively in process.  If
he's almost done, I'll wait until that's committed.  If you want something
sooner, I can update but make this dependent on 296856.  Let me know which
method is better for the community.

Updated

12 years ago
No longer blocks: 266252

Updated

12 years ago
Blocks: 311032
(Assignee)

Comment 50

12 years ago
Created attachment 199759 [details] [diff] [review]
patch against current tree

bit refactored (class declaration moved in *.h) and ifdefed patch.
Far from final, more refactoring coming, mostly due code in BTNCLICK and
MakeFocus. Maybe something more.
Put here for reference and for tigerdog and nielx convinience.
Assignee: Niels.Reedijk → sergei_d
Attachment #196286 - Attachment is obsolete: true
(Reporter)

Comment 51

12 years ago
Created attachment 200223 [details] [diff] [review]
One more try at this patch
Attachment #200223 - Flags: review?(sergei_d)

Updated

12 years ago
Summary: Patch to enable inline input in Japanese → Patch to enable inline input in Japanese on BeOS
(Assignee)

Comment 52

12 years ago
Comment on attachment 200223 [details] [diff] [review]
One more try at this patch

r=sergei_d
Attachment #200223 - Flags: review?(sergei_d) → review+
Koki-san:

The patch doesn't have nsTextFrame.cpp. I think that we need it.
(Assignee)

Comment 54

12 years ago
landed in trunk
Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.104; previous revision: 1.103
done
Checking in mozilla/widget/src/beos/nsWindow.h;
/cvsroot/mozilla/widget/src/beos/nsWindow.h,v  <--  nsWindow.h
new revision: 1.41; previous revision: 1.40
done

Will close after 1.8* checkin
You should close this bug by fixed on trunk.
And if fixed on 1.8 branch, you should use fixed1.8 and verified1.8 keywords.

See http://developer.mozilla.org/devnews/index.php/2005/08/18/branch-keywords/
(Assignee)

Comment 56

12 years ago
Masayuki-san:
nsTextFrame.cpp addition should be separate bug and patch IMHO.
I will be glad if you point me at proper component in CORE to submit it, and
maybe i will ask you for review, if you are allowed to do reviews on this topic.

Truth is it's very easy for us at moment to do patches in BeOS-only folders, but
quite hard (due formal reasons) in such components as layout/toolkit/gkview etc
(Assignee)

Comment 57

12 years ago
Comment on attachment 199759 [details] [diff] [review]
patch against current tree

obsoleting
Attachment #199759 - Attachment is obsolete: true
(Assignee)

Comment 58

12 years ago
closing bug
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 59

12 years ago
Comment on attachment 200223 [details] [diff] [review]
One more try at this patch

asking approval for 1.8 branch
Attachment #200223 - Flags: approval1.8rc1?
If this patch goes to 1.8 branch, we need to change nsTextFrame.cpp on 1.8
branch too. Because we may not get feedback for composition string. This is
serious regression.

Comment 61

12 years ago
AT this point I would advice against applying this patch in the 1.8 branch. I'm
still working out the kinks caused by the DnD patch, I can't work on this one as
well. 

Updated

12 years ago
Attachment #200223 - Flags: approval1.8rc1? → approval1.8rc1+
Depends on: 313174
I filed bug 313174 for rendering of composition string.

Comment 63

12 years ago
They way you use GetMouse doesn't work properly, it's to bad you sneak things in
instead of discussing them. Even if GetMouse remove the last message it may be a
MouseUp event. This makes me disappointed.
(Assignee)

Comment 64

12 years ago
tqh, i think your notice is about another bug, isn' it?
(Assignee)

Comment 65

12 years ago
Sorry, tqh, now I understand your disppointment.
Probably Koki got (i'm really guilty) diff against my version of nsWindow, not CVS.
So unreviewed (and without plans to review that) patch from Bug 312755 landed
here, which wasn't my intention at all.

Comment 66

11 years ago
Comment on attachment 200223 [details] [diff] [review]
One more try at this patch

Requesting approval for the MOZILLA_1_8_BRANCH

This is a BeOS-only change and will not affect any other platform.
Attachment #200223 - Flags: approval1.8.1?

Comment 67

11 years ago
Comment on attachment 200223 [details] [diff] [review]
One more try at this patch

a=darin on behalf of drivers
Attachment #200223 - Flags: approval1.8.1? → approval1.8.1+

Comment 68

11 years ago
Sergei, could you commit this to the branch after bug 312660?

(It's this one instead of bug 314687)
(Assignee)

Comment 69

11 years ago
Checking in mozilla/widget/src/beos/nsWindow.cpp;
/cvsroot/mozilla/widget/src/beos/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 1.91.4.14; previous revision: 1.91.4.13
done
Checking in mozilla/widget/src/beos/nsWindow.h;
/cvsroot/mozilla/widget/src/beos/nsWindow.h,v  <--  nsWindow.h
new revision: 1.35.4.9; previous revision: 1.35.4.8
done 
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.