The Trunk: ST80-dtl.141.mcz

aText := Text string: newString emphasis: emphasisHere.
- 	self markBlock < self pointBlock
- 		ifTrue: [self setMark: self markBlock stringIndex - 1]
- 		ifFalse: [self setPoint: self  pointBlock stringIndex - 1].
- 
  	wasComposition := true. 
+ 	self markBlock < self pointBlock
+ 		ifTrue: [ self setMark: self markBlock stringIndex - 1.
+ 				self zapSelectionWith: aText.
+ 				self setMark: self markBlock stringIndex + 1]
+ 		ifFalse: [ self setPoint: self pointBlock stringIndex - 1.
+ 				self zapSelectionWith: aText.
+ 				self setPoint: self pointBlock stringIndex + 1]
- 	self zapSelectionWith: aText.

Bob,

I tried reverting zapSelectionWithCompositionWith: to its original
implementation, and making the change that you suggest to readKeyboard.
It works exactly as you describe, including when I type rapidly in an
image with the wait2ms delay.

I will make this update to Squeak trunk. To summarize the changes
we will have at that point:

1) The wait2ms is removed
2) ParagraphEditor>>readKeyboard will always process one character at a time
3) No change to zapSelectionWithCompositionWith:

Thanks very much for your help on this!

Dave

On Sat, Feb 09, 2013 at 02:57:32PM -0500, Bob Arning wrote:

On 2/9/13 1:48 PM, David T. Lewis wrote:

Hi Bob, thanks for this.

I am hopelessly out of my depth on multilingual processing and multibyte
characters, but I'm sure others on the list will know: Are there cases
(e.g. in Japan or Korea) where keyboards provide multi-byte inputs, either
directly or through X11? If so, the "sensor keyboardPressed whileTrue:"
may be necessary in order to process those key entries.

Remember that there is another "[sensor keyboardPressed] whileTrue: " 
higher up that encompases this, so I don't think this is something to 
worry about.

Otherwise, I
suspect you are right that it is easier to just process one character at
a time. I note also that the "whileTrue:" approach dates back to at least
2002, well before introduction of multilingual support in Squeak. I don't
know if that's significant, or if that's just the way it happened to have
been done.

It was that way in 1998. My guess is that if multiple input characters 
were available, it made some sense efficiency-wise to insert them into 
the text all at once rather than piecemeal. Back the people typed just 
as fast as today, but the processors were much slower.

Just by way of explanation, here is how I found and verified the change
to zapSelectionWithCompositionWith: in ST80-dtl.141.mcz:

1) In the original method, I forced the method to always execute the
multi-character path through the code, even if the input string was one
character long (the normal case). So I changed from this:

  ((aString isEmpty or: [(beforeChar := self charBefore) isNil]) or: [
    aString size = 1 and: [(Unicode isComposition: aString first) not]]) 
    ifTrue: [
      ^ self zapSelectionWith: (Text string: aString emphasis: 
      emphasisHere)].

to this:

  ((aString isEmpty or: [(beforeChar := self charBefore) isNil]) or: [
    aString size = 1 and: [(Unicode isComposition: aString first) not]]) 
    ifTrue: [
      "^ self zapSelectionWith: (Text string: aString emphasis: 
      emphasisHere)" ].

Result: All characters typed into an MVC workspace resulted in the cursor
positioned incorrectly to the left of the current character.

2) I changed the method to reposition the cursor correctly, leaving the
above change in place to continue forcing execution of the multi-byte code
path. This fixed the cursor positioning problem in the MVC workspace.

3) I removed the change from step 1 so that single character input would
be handled through the normal path, and verified that the MVC workspace
still worked.

4) I filed the updated method into another image that still has the wait2ms
delays, and I ran that image on a Linux interpreter VM (with "2 ms" delays
that might be more like 15 ms). Result: Cursor positioning now worked 
correctly
in the MVC workspace, even though keyboard entry was slow.

I put your change in an otherwise unchanged 4.4 and mashed keys very 
quickly. Everything got entered, but the cursor got farther and farther 
behind the end of the text. This is on a Mac where 2ms seems to be 2ms. 
I then changed wait2ms to wait 20ms and things got worse - it took long 
pauses for characters to appear and when they did, the cursor did not 
keep pace with the end of the text. What might be interesting to know 
how often (if ever) your tests actually had more that one byte to 
process in the zap methods. I think a single character works fine either 
way, multiple characters work fine in the original zap.. but multiple 
characters in the new zap are the problem.

Conclusion: The patch in ST80-dtl.141.mcz does address a real bug in cursor
positioning that occured only in cases where sluggish key entry resulted
in multi-byte strings (presumably two characters) being handled in the
zapSelectionWithCompositionWith: method.

The problem that you identified with your "z123" test does not seem to be
occurring in practice,

But it did and that's what got me started here - I was trying to see 
sluggish input in MVC, so I mashed keys as fast as I could and they all 
made  it to the screen, just not always at the end of the text.

but it certainly looks as though it *could* occur.
I tested only by typing into a workspace from my US ascii keyboard, so I
certainly would not rule out the possibility of further problems.

I do not know if it is safe to make the change you suggest to process one
character at a time. It looks to me like it would be good to do that, but
I don't know if it might cause problems for non-US keyboards or locales.

If it was safe to make that change, then the entire bit of code for 
handling
multi-byte strings in zapSelectionWithCompositionWith: would become 
irrelevant,
and we could just get rid of it.

I don't think so. That code has a purpose - the UnicodeCompositionStream 
is used to combined multiple characters (like a letter followed by a 
separate accent character) into a single character. It is basically 
saying: if there are 1 or more characters in the typeahead and the first 
is one of these combining characters (accent) and there is a character 
in the text before the insertion point, then back up 1 space so we can 
process that character and the accent together. I suspect it works fine 
when there is just once character in the typeahead and that character is 
an accent. I don't think it works well when there are more than one 
character in the input. Hence my suggestion to do it a char at a time.

Cheers,
Bob

Hopefully someone with experience in this area can help?

Dave


On Sat, Feb 09, 2013 at 06:52:43AM -0500, Bob Arning wrote:

I fear that doesn't even work with ordinary USASCII input. I tweaked
readKeyboard to simulate additional characters arriving in close 
succession:

        self hasSelection ifTrue: "save highlighted characters"
            [UndoSelection := self selection].
*((model isKindOf: Workspace) and: [char = $z]) ifTrue: [typeAhead
nextPutAll: '123'].*

        self zapSelectionWithCompositionWith: typeAhead contents.

and what I get by typing some z's in a Workspace is:

z123zzzzzz123123123123123123

I think the safest way to guard against multiple characters arriving in
quick succession is to simply process each one completely before
proceeding to the next. This, after all, is the likeliest case by far
for modern computers and ordinary humans typing:

        self deselect.
*sensor keyboardPressed ifTrue: "was a whileTrue:"*
            [char := sensor keyboardPeek.
            (self dispatchOnCharacter: char with: typeAhead) ifTrue:
                [self doneTyping.
                self setEmphasisHere.
                ^self selectAndScroll; updateMarker].
            self openTypeIn].

Cheers,
Bob

On 2/9/13 1:01 AM, [hidden email] wrote:

aText := Text string: newString emphasis: emphasisHere.
- 	self markBlock < self pointBlock
- 		ifTrue: [self setMark: self markBlock stringIndex - 1]
- 		ifFalse: [self setPoint: self  pointBlock stringIndex - 1].
-
  	wasComposition := true.
+ 	self markBlock < self pointBlock
+ 		ifTrue: [ self setMark: self markBlock stringIndex - 1.
+ 				self zapSelectionWith: aText.
+ 				self setMark: self markBlock stringIndex + 1]
+ 		ifFalse: [ self setPoint: self pointBlock stringIndex - 1.
+ 				self zapSelectionWith: aText.
+ 				self setPoint: self pointBlock stringIndex +
1]
- 	self zapSelectionWith: aText.