Closed Bug 312778 Opened 19 years ago Closed 18 years ago

Undo after a spell check correction doesn't work, undoes previous action

Categories

(Core :: Spelling checker, defect, P1)

Product:
Core
Component:
Spelling checker
Version:
1.8 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.8.1beta1

People

(Reporter: lionelbrits, Assigned: martijn.martijn)

References

Details

(Keywords: fixed1.8.1, verified1.8.1.3)

Attachments

(3 files, 1 obsolete file)

patchv2
2.24 KB, patch
brettw
: review+
mscott
: superreview+
mscott
: approval-branch-1.8.1+
Details | Diff | Splinter Review
remove the redundant DeleteSelection call
975 bytes, patch
brettw
: review+
mscott
: superreview+
Details | Diff | Splinter Review
remove transaction
1.26 KB, patch
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051006 Firefox/1.4.1

I couldn't find this bug anywhere. Using Thunderbird version 1.6a1 (20051001)
If I type in a sentence, correct one word that has been underlined in red by
rightclicking and selecting appropriate correction, and then hit ctrl+z or
select undo from edit menu, all my text disappears.  

Reproducible: Always

Steps to Reproduce:
1. Compose new window
2. Enter "I am a doodlehead" in body
3. Rightclick on "doodlehead" and select "doodle head"
4. Hit ctrl+Z

Actual Results:  
Entire body disappears

Expected Results:  
Should have reverted to prior spelling.

Upon further research it seems that this problem stems from fundamental
broken-ness of undo, i.e. too little granularity. But for the 10 percent of
users that will ever learn about undo, only about 50 percent of those will learn
about redo, which is the obvious work-around for this bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Using Spell check underline thingie doesn't become part of undo stack → Doing undo (ctrl+z) after choosing a suggestion from the spell checker removes entire line (text disappears)
Version: unspecified → 1.5
Reproducible with SeaMonkey 1.0a and SeaMonkey 1.1a/20051013.
*** Bug 316553 has been marked as a duplicate of this bug. ***
Damn!  I swear I searched before I filed bug 316533.
In TB 1.5rc1 (20051201) this behavior is gone, but undo simply doesn't undo the replacement. Can't imagine that this is wanted.
Component: Message Compose Window → Spelling checker
Product: Thunderbird → Core
Summary: Doing undo (ctrl+z) after choosing a suggestion from the spell checker removes entire line (text disappears) → Undo after a spell check correction doesn't work, undoes previuos action
Target Milestone: --- → mozilla1.8.1beta1
Version: 1.5 → 1.8 Branch
Assignee: mscott → brettw
Blocks: SpellCheckTracking
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: Undo after a spell check correction doesn't work, undoes previuos action → Undo after a spell check correction doesn't work, undoes previous action
*** Bug 339489 has been marked as a duplicate of this bug. ***
Attached patch patch (obsolete) — — Splinter Review 
This fixes it for me.
Only after an undo, the spellcheck word gets selected. Not sure if that's a big problem. Not sure if that's a problem at all.
Attachment #223700 - Flags: review?(brettw)
If you correct multiple words in a row, and then undo, does it work? i.e. does Ctrl+Z undo one correction each time, or is every-other-one some kind of "undo selection"?
With the patch, ctrl+Z undoes one correction each time.
Comment on attachment 223700 [details] [diff] [review]
patch

GetMisspelledWord can return a NULL range, which is why the old code checked for it. You should preserve this check.
Attachment #223700 - Flags: review?(brettw) → review-
Attached patch patchv2 — — Splinter Review 
Oops, you're right. I forgot about that.
Attachment #223700 - Attachment is obsolete: true
Attachment #223703 - Flags: review?(brettw)
Attachment #223703 - Flags: superreview?(mscott)
Attachment #223703 - Flags: review?(brettw)
Attachment #223703 - Flags: review+
Attachment #223703 - Flags: approval-branch-1.8.1?(mscott)
Assignee: brettw → martijn.martijn
Comment on attachment 223703 [details] [diff] [review]
patchv2

looks good Brett.
Attachment #223703 - Flags: superreview?(mscott)
Attachment #223703 - Flags: superreview+
Attachment #223703 - Flags: approval-branch-1.8.1?(mscott)
Attachment #223703 - Flags: approval-branch-1.8.1+
I'll check this in in the next couple of days.
Whiteboard: check in
I can check it in, too, but I don't mind if you do it.
Oh, if you have priveledges, it's better if you do it so you're in the CVS log.
Checking in extensions/spellcheck/src/mozInlineSpellChecker.cpp;
/cvsroot/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp,v  <--  moz
InlineSpellChecker.cpp
new revision: 1.15; previous revision: 1.14
done

Checked into trunk. Sorry for the delay.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Can you check it into branch soon?
Flags: blocking1.8.1?
Pff, I finally have a working 1.8.1 branch build.

Checking in extensions/spellcheck/src/mozInlineSpellChecker.cpp;
/cvsroot/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp,v  <--  moz
InlineSpellChecker.cpp
new revision: 1.6.4.5; previous revision: 1.6.4.4
done

Checked into 1.8.1 branch.
Flags: blocking1.8.1?
Keywords: fixed1.8.1
Whiteboard: check in
Comment on attachment 223703 [details] [diff] [review]
patchv2

>+    editor->BeginTransaction();
>+  
>+    NS_ENSURE_SUCCESS(res, res);
>+    selection->RemoveAllRanges();
>+    selection->AddRange(range);
>+    editor->DeleteSelection(nsIEditor::eNone);
>+    nsCOMPtr<nsIPlaintextEditor> textEditor(do_QueryReferent(mEditor));
>+    textEditor->InsertText(newword);
>+    editor->EndTransaction();
InsertText automatically deletes the selection (have you never typed over a selection?) so you don't need to delete it yourself or need fiddle around with transactions.
Ok, thanks, you're right. This can safely be removed.
Attachment #226263 - Flags: superreview?(neil)
Attachment #226263 - Flags: review?(brettw)
Comment on attachment 226263 [details] [diff] [review]
remove the redundant DeleteSelection call

Should also be fixed on branch, then, I think.
Attachment #226263 - Flags: superreview?(neil)
Attachment #226263 - Flags: superreview?(mscott)
Attachment #226263 - Flags: approval-branch-1.8.1?(mscott)
Attachment #226263 - Flags: superreview?(mscott) → superreview+
Attachment #226263 - Flags: review?(brettw) → review+
*** Bug 342141 has been marked as a duplicate of this bug. ***
Attachment #226263 - Flags: approval-branch-1.8.1?(mscott) → approval1.8.1?
Checking in extensions/spellcheck/src/mozInlineSpellChecker.cpp;
/cvsroot/mozilla/extensions/spellcheck/src/mozInlineSpellChecker.cpp,v  <--  moz
InlineSpellChecker.cpp
new revision: 1.16; previous revision: 1.15
done

Checked the "remove the redundant DeleteSelection call" patch in on trunk.
Comment on attachment 226263 [details] [diff] [review]
remove the redundant DeleteSelection call

You misunderstood part of comment 18 which was that you don't need the transaction either, surely?
(In reply to comment #23)
> (From update of attachment 226263 [details] [diff] [review] [edit])
> You misunderstood part of comment 18 which was that you don't need the
> transaction either, surely?

No, sorry, I completely missed that part :(

Again my apologies for all the patches.
Attachment #226698 - Flags: superreview?(neil)
Attachment #226698 - Flags: review?(brettw)
Comment on attachment 226698 [details] [diff] [review]
remove transaction

I don't understand why this works. Perhaps you can find somebody else who does?
Attachment #226698 - Flags: review?(brettw)
Attachment #226698 - Flags: superreview?(neil)
Depends on: 342511
Comment on attachment 226263 [details] [diff] [review]
remove the redundant DeleteSelection call

This patch caused bug 342511.
Attachment #226263 - Flags: approval1.8.1?
Depends on: 342536
No longer depends on: 342511
I've backed out the "remove the redundant DeleteSelection call" patch, because of the regression in bug 342536. Apparently, it wasn't that redundant at all.
I'm not going to bother with this seemingly unnecessary code. There is still risk involved, as I noticed and the benefit is minimal.
Depends on: 342748
*** Bug 350682 has been marked as a duplicate of this bug. ***
*** Bug 353127 has been marked as a duplicate of this bug. ***
>  
it should be noted that the problem still exists in release version   1.5.0.7 (upgrade date 18-Sept-2006) 
verified fixed 1.8.1.3 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.3) Gecko/20070326 Thunderbird/2.0.0.0 Mnenhy/0.7.5.0 ID:2007032620 and the steps to reproduce from comment #0
Keywords: verified1.8.1.3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: