Provide UI to show alternative strings of dictated text.

• Component: New Bugs
• Status: CLOSED WONTFIX

[Jia Pu]

When user uses speech recognition as input method, sometimes the recognizer is not very confident about the recognized text, and provides alternatives texts. On OSX, AppKit provides an UI to show alternative text when user move caret to end of a dictated word which has alternatives. The behavior is similar to spelling suggestion UI. We'd like to hook up the same UI to WebKit on OSX.

<rdar://problem/11033898>

[Jia Pu]

Created attachment 134430 [details]
Patch (v1)

[Mark Rowe (bdash)]

Comment on attachment 134430 [details]
Patch (v1)

View in context: https://bugs.webkit.org/attachment.cgi?id=134430&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:27747
> +				COPY_PHASE_STRIP = NO;
> +				GCC_TREAT_WARNINGS_AS_ERRORS = NO;

Don't do this.

[Jia Pu]

(In reply to comment #2)
> (From update of attachment 134430 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134430&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:27747
> > +				COPY_PHASE_STRIP = NO;
> > +				GCC_TREAT_WARNINGS_AS_ERRORS = NO;
> 
> Don't do this.

Oops. We will for sure have some revision to this patch. I will revert this in next patch. Accidental change when I struggled to set up a debug environment.

[Jia Pu]

(In reply to comment #2)
> (From update of attachment 134430 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134430&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:27747
> > +				COPY_PHASE_STRIP = NO;
> > +				GCC_TREAT_WARNINGS_AS_ERRORS = NO;
> 
> Don't do this.

Oops. We will for sure have some revision to this patch. I will revert this in next patch. Accidental change when I struggled to set up a debug environment.

[Alexey Proskuryakov]

Comment on attachment 134430 [details]
Patch (v1)

View in context: https://bugs.webkit.org/attachment.cgi?id=134430&action=review

> Source/WebCore/editing/DictationAlternativeController.h:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY

This is an old version of the license - the company is called differently now.

[Jia Pu]

Created attachment 134518 [details]
Patch (v2)

[Jia Pu]

Patch (v2):
Updated patch to TOT.
Reverted unintended xcode project file change.
Fixed entity name in license term.

[WebKit Review Bot]

Attachment 134518 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/editing/SpellingCorrectionController.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/editing/Editor.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/editing/DictationAlternativeController.h:37:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 3 in 78 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 134518 [details]
Patch (v2)

Attachment 134518 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12172219

[WebKit Review Bot]

Comment on attachment 134518 [details]
Patch (v2)

Attachment 134518 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12172218

[Early Warning System Bot]

Comment on attachment 134518 [details]
Patch (v2)

Attachment 134518 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12204151

[Build Bot]

Comment on attachment 134518 [details]
Patch (v2)

Attachment 134518 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12172220

[Build Bot]

Comment on attachment 134518 [details]
Patch (v2)

Attachment 134518 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12202146

[Early Warning System Bot]

Comment on attachment 134518 [details]
Patch (v2)

Attachment 134518 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12184258

[Build Bot]

Comment on attachment 134518 [details]
Patch (v2)

Attachment 134518 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12192178

[WebKit Review Bot]

Comment on attachment 134518 [details]
Patch (v2)

Attachment 134518 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12173231

[Early Warning System Bot]

Comment on attachment 134518 [details]
Patch (v2)

Attachment 134518 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12192181

[Gustavo Noronha (kov)]

Comment on attachment 134518 [details]
Patch (v2)

Attachment 134518 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12173239

[Gyuyoung Kim]

Comment on attachment 134518 [details]
Patch (v2)

Attachment 134518 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12172314

[Jia Pu]

Created attachment 134612 [details]
Patch (v3)

[Jia Pu]

Patch (v3): Attempt to fix build failures on various platforms.

[WebKit Review Bot]

Attachment 134612 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/editing/DictationAlternativeController.h:37:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Source/WebCore/WebCore.vcproj/WebCore.vcproj:54006:  not well-formed (invalid token)  [xml/syntax] [5]
Total errors found: 2 in 89 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jia Pu]

Created attachment 134616 [details]
Patch (v4)

[WebKit Review Bot]

Attachment 134616 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebCore/editing/DictationAlternativeController.h:37:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 89 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Comment on attachment 134616 [details]
Patch (v4)

Attachment 134616 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12192369

[Jia Pu]

(In reply to comment #24)
> Attachment 134616 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
> Source/WebCore/editing/DictationAlternativeController.h:37:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
> Total errors found: 1 in 89 files
> 
> 

I not quite sure about this style violation. The offending line just follows what's already on SpellingCorrectionController.h:69.

[Build Bot]

Comment on attachment 134616 [details]
Patch (v4)

Attachment 134616 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12192374

[Ryosuke Niwa]

Comment on attachment 134616 [details]
Patch (v4)

View in context: https://bugs.webkit.org/attachment.cgi?id=134616&action=review

r- due to style and build EWS failures. You should also restructure this code so that it doesn't add so much code.

> Source/WebCore/ChangeLog:16
> +        3. Introduced DictationCommand which will add DictationAlternative markers to text
> +           inserted by the command.

It seems like this class is unnecessary. We probably want to add a new flag or something to InsertTextCommand instead.

> Source/WebCore/editing/AlternativeTextController.cpp:76
> +    VisibleSelection currentSelection(m_frame->selection()->selection());
> +    if (!currentSelection.isCaret() || currentSelection == oldSelection)
> +        return;
> +    
> +    VisiblePosition selectionPosition = currentSelection.start();
> +    // Creating a Visible position triggers a layout and there is no
> +    // guarantee that the selection is still valid.
> +    if (selectionPosition.isNull())
> +        return;
> +    
> +    VisiblePosition endPositionOfWord = endOfWord(selectionPosition, LeftWordIfOnBoundary);
> +    if (selectionPosition != endPositionOfWord)
> +        return;
> +    
> +    Position position = endPositionOfWord.deepEquivalent();
> +    if (position.anchorType() != Position::PositionIsOffsetInAnchor)
> +        return;

This chunk of code is duplicated in SpellingCorrectionController::respondToChangedSelection.
We should at least share some code with SpellingCorrectionController.
I'm not convinced at all that we need this new class for just supporting dictation alternatives.
Can't we generalize SpellingCorrectionController to support dictations as well?

> Source/WebCore/editing/AlternativeTextController.cpp:129
> +PassRefPtr<Range> replaceTextInRange(const String& newText, Range* range, Frame* frame, bool forSpellingCorrection)
> +{

This function probably belongs in Editor.cpp

> Source/WebKit/mac/WebView/WebHTMLView.mm:5975
> +#if USE(DICTATION_ALTERNATIVES)
> +        if (dictationAlternatives.size())
> +            eventHandled = [[self _webView] _insertDictatedText:eventText alternatives:dictationAlternatives triggeringEvent:event];
> +        else
> +#endif
> +            eventHandled = coreFrame->editor()->insertText(eventText, event);

You should obtain the list of dictation alternatives here, and then add some optional argument to insertText.

[Ryosuke Niwa]

(In reply to comment #26)
> (In reply to comment #24)
> > Attachment 134616 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
> > Source/WebCore/editing/DictationAlternativeController.h:37:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
> > Total errors found: 1 in 89 files
> > 
> > 
> 
> I not quite sure about this style violation. The offending line just follows what's already on SpellingCorrectionController.h:69.

A lot of existing code violates coding style but we try to fix any style violations in new code so that we can slowly migrate the entire code base to be style-guide compliant :)

[Jia Pu]

(In reply to comment #28)
> (From update of attachment 134616 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134616&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        3. Introduced DictationCommand which will add DictationAlternative markers to text
> > +           inserted by the command.
> 
> It seems like this class is unnecessary. We probably want to add a new flag or something to InsertTextCommand instead.

DictationCommand is the counter part of TypingCommand when the input is done by dictation rather than typing. They both call into InsertTextCommand eventually. I could try to merge DictationCommand into TypingCommand, but then the name "TypingCommand" won't be accurate anymore. Do we want to keep them separated to make the distinction clear, or do we want to merge them, and maybe change "TypingCommand" to "TextInputCommand" ?

> 
> > Source/WebCore/editing/AlternativeTextController.cpp:76
> > +    VisibleSelection currentSelection(m_frame->selection()->selection());
> > +    if (!currentSelection.isCaret() || currentSelection == oldSelection)
> > +        return;
> > +    
> > +    VisiblePosition selectionPosition = currentSelection.start();
> > +    // Creating a Visible position triggers a layout and there is no
> > +    // guarantee that the selection is still valid.
> > +    if (selectionPosition.isNull())
> > +        return;
> > +    
> > +    VisiblePosition endPositionOfWord = endOfWord(selectionPosition, LeftWordIfOnBoundary);
> > +    if (selectionPosition != endPositionOfWord)
> > +        return;
> > +    
> > +    Position position = endPositionOfWord.deepEquivalent();
> > +    if (position.anchorType() != Position::PositionIsOffsetInAnchor)
> > +        return;
> 
> This chunk of code is duplicated in SpellingCorrectionController::respondToChangedSelection.
> We should at least share some code with SpellingCorrectionController.

I use AlternativeTextController to hold code shared by SpellingCorrectionController and DictationAlternativeController. I will try to factor this piece of code out as well.

> I'm not convinced at all that we need this new class for just supporting dictation alternatives.
> Can't we generalize SpellingCorrectionController to support dictations as well?

OK, I will merge DictationAlternativeController into SpellingCorrectionController.

[Jia Pu]

Created attachment 134925 [details]
Patch (v5)

[Jia Pu]

Following Ryosuke's suggestion, Patch (v5) avoided adding new classes DictationAlternativeController and AlternativeTextController, in addition to existing SpellingCorrectionController. Instead, this patch renamed SpellingCorrectionController to AlternativeTextController, which now contains logic for both spelling correction and alternative dictated text. But this does result in much larger patch size.

This patch still contains new class DictationCommand. Since InsertTextCommand assumes that the input doesn't contain line separator, it's not very easy to fit dictation input into InsertTextCommand. It's possible to merge dictation command into existing TypingCommand class. However, TypingCommand is quite complex already, and most of its functionalities are irrelevant to dictated text.

[Early Warning System Bot]

Comment on attachment 134925 [details]
Patch (v5)

Attachment 134925 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12288547

[Build Bot]

Comment on attachment 134925 [details]
Patch (v5)

Attachment 134925 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12267558

[Early Warning System Bot]

Comment on attachment 134925 [details]
Patch (v5)

Attachment 134925 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12266634

[WebKit Review Bot]

Comment on attachment 134925 [details]
Patch (v5)

Attachment 134925 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12291540

[Philippe Normand]

Comment on attachment 134925 [details]
Patch (v5)

Attachment 134925 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12291551

[Build Bot]

Comment on attachment 134925 [details]
Patch (v5)

Attachment 134925 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12247617

[Gustavo Noronha (kov)]

Comment on attachment 134925 [details]
Patch (v5)

Attachment 134925 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12266626

[Build Bot]

Comment on attachment 134925 [details]
Patch (v5)

Attachment 134925 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12266657

[Gyuyoung Kim]

Comment on attachment 134925 [details]
Patch (v5)

Attachment 134925 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12262561

[Ryosuke Niwa]

Comment on attachment 134925 [details]
Patch (v5)

View in context: https://bugs.webkit.org/attachment.cgi?id=134925&action=review

I would be interested in hearing morrita's comments here since he has done a lot of refactoring in this area.
I want to make sure we're heading to the right direction with this new feature.

> Source/WebCore/editing/AlternativeTextController.h:119
> +class AlternativeTextController {

Did you use svn mv for this rename? patch usually shows diff if you had used svn mv.

> Source/WebCore/editing/DictationCommand.cpp:102
> +void DictationCommand::doApply()
> +{
> +    unsigned offset = 0;
> +    size_t newline;
> +    while ((newline = m_textToInsert.find('\n', offset)) != notFound) {
> +        if (newline != offset)
> +            insertTextRunWithoutNewlines(offset, newline - offset);
> +        insertParagraphSeparator();
> +        offset = newline + 1;
> +    }
> +    if (!offset)
> +        insertTextRunWithoutNewlines(0, m_textToInsert.length());
> +    else {
> +        unsigned length = m_textToInsert.length();
> +        if (length != offset)
> +            insertTextRunWithoutNewlines(offset, length - offset);
> +    }
> +}

This looks like a duplicate of TypingCommand::insertText(const String &text, bool selectInsertedText) at http://trac.webkit.org/browser/trunk/Source/WebCore/editing/TypingCommand.cpp?rev=106646#L354
Why do we need to duplicate this code?

[Jia Pu]

(In reply to comment #42)
> (From update of attachment 134925 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134925&action=review
> 
> I would be interested in hearing morrita's comments here since he has done a lot of refactoring in this area.
> I want to make sure we're heading to the right direction with this new feature.
> 
> > Source/WebCore/editing/AlternativeTextController.h:119
> > +class AlternativeTextController {
> 
> Did you use svn mv for this rename? patch usually shows diff if you had used svn mv.

No, I use git-svn, and renamed it in Xcode. I think xcode uses "git mv" under the hood. 

> 
> > Source/WebCore/editing/DictationCommand.cpp:102
> > +void DictationCommand::doApply()
> > +{
> > +    unsigned offset = 0;
> > +    size_t newline;
> > +    while ((newline = m_textToInsert.find('\n', offset)) != notFound) {
> > +        if (newline != offset)
> > +            insertTextRunWithoutNewlines(offset, newline - offset);
> > +        insertParagraphSeparator();
> > +        offset = newline + 1;
> > +    }
> > +    if (!offset)
> > +        insertTextRunWithoutNewlines(0, m_textToInsert.length());
> > +    else {
> > +        unsigned length = m_textToInsert.length();
> > +        if (length != offset)
> > +            insertTextRunWithoutNewlines(offset, length - offset);
> > +    }
> > +}
> 
> This looks like a duplicate of TypingCommand::insertText(const String &text, bool selectInsertedText) at http://trac.webkit.org/browser/trunk/Source/WebCore/editing/TypingCommand.cpp?rev=106646#L354
> Why do we need to duplicate this code?

I am hesitant to put DictationCommand code into TypingCommand. How about move this function into CompoteEditCommand, so that it can be used by both DictationCommand and TypingCommand?

[Ryosuke Niwa]

(In reply to comment #43)
> (In reply to comment #42)
> > (From update of attachment 134925 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=134925&action=review
> > 
> > I would be interested in hearing morrita's comments here since he has done a lot of refactoring in this area.
> > I want to make sure we're heading to the right direction with this new feature.
> > 
> > > Source/WebCore/editing/AlternativeTextController.h:119
> > > +class AlternativeTextController {
> > 
> > Did you use svn mv for this rename? patch usually shows diff if you had used svn mv.
> 
> No, I use git-svn, and renamed it in Xcode. I think xcode uses "git mv" under the hood. 

The last time I checked, git mv doesn't work with WebKit repository. You'll probably need to do this manually on some svn checkout.

> > This looks like a duplicate of TypingCommand::insertText(const String &text, bool selectInsertedText) at http://trac.webkit.org/browser/trunk/Source/WebCore/editing/TypingCommand.cpp?rev=106646#L354
> > Why do we need to duplicate this code?
> 
> I am hesitant to put DictationCommand code into TypingCommand. How about move this function into CompoteEditCommand, so that it can be used by both DictationCommand and TypingCommand?

But so much of DictationCommand looks like TypingCommand. TypingCommand is a very weird editing command (it has a concept of being open, etc...). Does DictationCommand want a similar behavior? e.g. if a user types some word by a dictation, followed by another word by a dictation, should undo remove both of those words? Or should it revert one by one? If the answer to this question is yes, then I don't want to introduce another composite edit command that behaves like TypingCommand. If anything, we should be extracting a common super class aside from CompositeEditCommand that this peculiar behavior.

[Jia Pu]

(In reply to comment #44)
> (In reply to comment #43)
> > (In reply to comment #42)
> > > (From update of attachment 134925 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=134925&action=review
> > > 
> > > I would be interested in hearing morrita's comments here since he has done a lot of refactoring in this area.
> > > I want to make sure we're heading to the right direction with this new feature.
> > > 
> > > > Source/WebCore/editing/AlternativeTextController.h:119
> > > > +class AlternativeTextController {
> > > 
> > > Did you use svn mv for this rename? patch usually shows diff if you had used svn mv.
> > 
> > No, I use git-svn, and renamed it in Xcode. I think xcode uses "git mv" under the hood. 
> 
> The last time I checked, git mv doesn't work with WebKit repository. You'll probably need to do this manually on some svn checkout.

Ok, I will try to work from a svn checkout. 

> 
> > > This looks like a duplicate of TypingCommand::insertText(const String &text, bool selectInsertedText) at http://trac.webkit.org/browser/trunk/Source/WebCore/editing/TypingCommand.cpp?rev=106646#L354
> > > Why do we need to duplicate this code?
> > 
> > I am hesitant to put DictationCommand code into TypingCommand. How about move this function into CompoteEditCommand, so that it can be used by both DictationCommand and TypingCommand?
> 
> But so much of DictationCommand looks like TypingCommand. TypingCommand is a very weird editing command (it has a concept of being open, etc...). 

DictationCommand has much simpler logic than TypingCommand. It is really just some sort of "insert text" command that supports multi-line input and allows passing in markers attached to the input text. How about using multiple inheritance? We would create a CommonTextInserationBehavior class that contains that shared insertText() function. DictationCommand and TypingCommand would derive from both CompsiteEditCommand and CommonTextInserationBehavior. This way, we can share the code without moving it into CompositeEditCommand to muddy the relatively pure base class.

[Ryosuke Niwa]

(In reply to comment #45)
> DictationCommand has much simpler logic than TypingCommand. It is really just some sort of "insert text" command that supports multi-line input and allows passing in markers attached to the input text. How about using multiple inheritance? We would create a CommonTextInserationBehavior class that contains that shared insertText() function. DictationCommand and TypingCommand would derive from both CompsiteEditCommand and CommonTextInserationBehavior. This way, we can share the code without moving it into CompositeEditCommand to muddy the relatively pure base class.

Why can't this shared class inherit from CompositeEditCommand? That's the benefit of using multiple inheritance here?

[Hajime Morrita]

Hi Jia,

This patch looks to large to review at once. How about to split this into

- Simple naming
   - from SpellingCorrectionController to AlternativeTextController
   - related names like Editor::m_spellingCorrector
   which would be trivial and easy to get reviewed. Some small style change
   like bool-to-enum conversion could be part of this.

- Adding Clients. I hope spelling correction and dictation related client API to have
   separate object, like AlternativeTextClient. tons of ifdefs are strong signal for new interface.
   Also, we might be able to apply Supplement<T> pattern for Editor.

- Extending TextEvent to add dictation related data and types.
   I thinks change on EventHandler and Editor related this dictation stuff
   could stay inside AlternativeTextController since it doesn't affect Editor/EventHandler state.
   Adding DictationCommand could be also done here.
   We can write tests which don't care markers.

- Adding marker state and rendering. test is easy and you can use window.internals object for this.
   It now inspect marker types on the node.

This splitting is just an idea. The point here is that splitting trivial change and
landing stuff in stage manner both help us a lot.
It would be appreciated if you consider approaches like this.

[Jia Pu]

(In reply to comment #47)
> Hi Jia,
> 
> This patch looks to large to review at once. How about to split this into
> 

Thanks for the suggestion, I will work toward something like you suggested.

[Jia Pu]

(In reply to comment #46)
> (In reply to comment #45)
> > DictationCommand has much simpler logic than TypingCommand. It is really just some sort of "insert text" command that supports multi-line input and allows passing in markers attached to the input text. How about using multiple inheritance? We would create a CommonTextInserationBehavior class that contains that shared insertText() function. DictationCommand and TypingCommand would derive from both CompsiteEditCommand and CommonTextInserationBehavior. This way, we can share the code without moving it into CompositeEditCommand to muddy the relatively pure base class.
> 
> Why can't this shared class inherit from CompositeEditCommand? That's the benefit of using multiple inheritance here?

We can certainly do that. My thought is that CompositeEditCommand is base class of many concrete command types, but this little function is relevant to only these two commands.

[Jia Pu]

(In reply to comment #49)
> (In reply to comment #46)
> > (In reply to comment #45)
> > > DictationCommand has much simpler logic than TypingCommand. It is really just some sort of "insert text" command that supports multi-line input and allows passing in markers attached to the input text. How about using multiple inheritance? We would create a CommonTextInserationBehavior class that contains that shared insertText() function. DictationCommand and TypingCommand would derive from both CompsiteEditCommand and CommonTextInserationBehavior. This way, we can share the code without moving it into CompositeEditCommand to muddy the relatively pure base class.
> > 
> > Why can't this shared class inherit from CompositeEditCommand? That's the benefit of using multiple inheritance here?
> 
> We can certainly do that. My thought is that CompositeEditCommand is base class of many concrete command types, but this little function is relevant to only these two commands.

Sorry, mis-read your comment. My previous reply is directly relevant. 

I'm often more inlined toward multiple inheritance for this sort of small shared functions, Because it's more flexible to add on shared code without significantly alter existing primary class hierarchy. But as I said, I can go either way.

[Ryosuke Niwa]

(In reply to comment #50)
> I'm often more inlined toward multiple inheritance for this sort of small shared functions, Because it's more flexible to add on shared code without significantly alter existing primary class hierarchy. But as I said, I can go either way.

While multiple-inheritace is a handy way to modularize classes (we use it for ScriptElement class for example), I don't think we need that kind of modularity for these two classes so I'd rather use a single inheritance to keep the inheritance hierarchy simple here.

[Hajime Morrita]

Comment on attachment 134925 [details]
Patch (v5)

Marking this obsolete since Jia is submitting the staged version.

[Jia Pu]

Closing this bug, since I will address this with staged changes in other bugs.