Closed Bug 352037 Opened 18 years ago Closed 13 years ago

Undo Add To Dictionary

Categories

(Core :: Spelling checker, enhancement)

Product:
Core
Component:
Spelling checker
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla12

People

(Reporter: elharo, Assigned: qheaden)

References

(Blocks 2 open bugs)

Details

(Keywords: ue)

Attachments

(1 file, 3 obsolete files)

Patch v3.1
21.18 KB, patch
ehsan.akhgari
: review+
ehsan.akhgari
: checkin+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b2) Gecko/20060821 Firefox/2.0b2
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1b2) Gecko/20060821 Firefox/2.0b2

Undo (Cmnd-Z, Edit/Undo) does not Undo adding a word to the dictionary. Instead it undoes the last edit. 

Reproducible: Always

Steps to Reproduce:
1. Type something misspelled such as "Foog"
2. Right click the word and select "Add to Dictionary" from the context menu
3. Go the edit menu and select Undo. 

Actual Results:  
The last typed text is deleted. 

Expected Results:  
The new word is removed from the dictionary. No text is deleted. The word is once again marked as misspelled.  

Probably related to 312778, but this may be an additional means to trigger the same bug (different context menu item); and does not appear fixed as of 2.0b2. The undo action that's needed here is also different, since the dictionary change must be backed out.
Assignee: nobody → mscott
Status: UNCONFIRMED → NEW
Component: General → Spelling checker
Ever confirmed: true
OS: Mac OS X 10.4 → All
Product: Firefox → Core
QA Contact: general → spelling-checker
Hardware: Macintosh → All
Version: unspecified → 1.8 Branch
Severity: normal → enhancement
Keywords: ue
o.k, I'd just like to add to this bug 415779 has a good point of moving "add to dictionary" further away from the words, thus causing less problems.
Assignee: mscott → nobody
Nominating this bug for being wanted, it seems somewhat in limbo atm.
Flags: wanted1.9.1?
Version: 1.8 Branch → Trunk
Seconded

In the same vain, having the GUI enhancement to move the "Add to Dictionary" button moved further down the menu to prevent accidental clicking would be great.

It's a real pain in the backside to have to manually edit persdict and then re-open Firefox when I had 20+ tabs open at work.

I have had to go through this process three times now in a fortnight. (What can I say, I was really busy, there was only one suggestion in the list because "bacisally" can only be spelt one way and I clicked the wrong button - just.)

That said, I cannot leave it and have the chance of a client get a typo from me, it's just not cricket.

Thank you for your consideration.

James
I like the idea of adding the undo feature, but for now can we simply move the add to dictionary command next to the undo command so there is at least a spacer between the suggestions and add to dictionary?  Then somebody can add the undo feature another day.
I agree with James. That sounds practical.
What about presenting the fact that the "add to dictionary" action has occured in the yellow warning bar (the one that gets presented for things like flash/plugin installation) along with an "undo" button?

Mabye an "edit -> dictionary" menu item would also be useful.

Or maybe the right-click "add to dictionary" menu should be renamed to "dictionary", which would take the user to a more comprehensive dictionary that has an option to add the mispelled word.  This would prevent errant clicks while still allowing the "add to" action if the user deliberately wants to do so.
Note that the last suggestion would also reduce some menu clutter -- the "languages" entry in the right-click menu could be moved into the new "dictionary" dialog.
This might be easier than it looks, and the UI could be pretty simple too (no need for a dictionary editor).

The Personal Dictionary check is actually done in mozHunspell.cpp. It wouldn't be too hard to record whether a "correct" word was found in the main Hunspell dictionary or the Personal Dictionary. 

If it was in the Personal Dictionary, then add a menu item for "Remove from Dictionary" where otherwise there would be an item for "Add to Dictionary."

Not volunteering to fix it yet, just saying.
Why in the world hasn't this been fixed? The raw undo functionality alone would be enough, but the other suggestions are awesome too. Yet it's two years old and the problem persists. I'm even looking for a plugin or greasemonkey script to do it since apparently mozilla won't.

This kind of **** and unresponsiveness is why people go to chrome.
CCing some UX folks to see if they have ideas on how the user interaction should work here...
Keywords: uiwanted
As far as I can tell, Chrome doesn't have "undo add to dictionary" functionality.
A simple way would be to include an extra item in the context menu “Undo adding ‘XYZ’ to the dictionary” and make it only appear when there's something that was just added — ie. the “oops” case.
Keywords: uiwanted
(In reply to Brandon from comment #14)
> This kind of **** and unresponsiveness is why people go to chrome.

Hi Brandon, I know you're new to Bugzilla. We try to keep it civil around here.

Rude remarks like that are not appreciated. There are thousands of things that are more important to fix than this particular issue, so please keep some perspective when commenting. :)
And that's exactly what I'm talking about. Thanks for proving my point Alex. 

That same fallacious old song and dance. 

I don't care what you consider to be more important. I am the end user and my demands are not structured around developer limitations or opinions, quite the opposite. You exist to serve Us. Your usefulness is defined by what we consider to be problems. 

You remind me of spoiled Corporations convinced that their service is a privilege for which customers should be obsequiously grateful.

No.

It is Your perspective that is in need of revision.

This complaint is as old as punch cards and open source software itself, and the answer has always been the same.

Either you are being paid, in which case its your job to solve the problems We, the users, deem important, or you are not in which case you're a volunteer and your complaint is a bit like punching a brick wall and then complaining about your hand. 

If you don't like it, tough, that's why you're compensated with pay, or If you don't like it, stop, no one's forcing you to volunteer (or even answer).

This kind of "you'll take what we give you and any general complaint is rudeness" attitude is exactly why people reject closed source software in the first place.

I guess the bloody history of Africa, the Stanford prison experiment, Gaddafi, all the way down to the attitudes of privileged developers like yourself, show the same consistent human trend, when the revolutionary becomes the autocrat, (when the rouge web browser becomes the de facto standard) the regime acts the same.

I think Frank Herbert explained it best speaking through Leto II when he said... 

"All rebellions are ordinary and an ultimate bore. They are copied out of the same pattern, one much like another. The driving force is adrenalin addiction and the desire to gain personal power. All rebels are closet aristocrats."~God Emperor of Dune

Specifically, there are Not "thousands of things" (hyperbole if there ever was any) "more important" to Me, if at all. In fact I'm quite pleased with firefox other than with this Long standing, and now I see intentionally preserved annoyance.

Again, what you consider to be important phases me not one whit. I don't care how hard you job is just like you don't care how hard mine is. I will complain when I feel the need.

A tiny hyper annoying problem that persists for years because of sheer neglect Is ****, and if characterizing it thus makes one rude then propriety becomes a massive Nirvana Fallacy, wherein an infinite progression of more important things prevents anything from getting done as easily fixable problems are constantly shoved to the side in favor of impossibly difficult ones.

Even if your snide condescending statements (and tone) were justified, (which neither are) the fact would remain that importance does not dictate priority. The trade off between action and result dictates priority. In this case, sure there may be more "important" (defined by who? apparently not consensus) problems but the solution to this one is obvious and judging from your own dismissal of it as trivial, apparently quite easy to fix.

This error, judging from this report alone, is at least 5 Years old. I and others have been patient enough. After 5 years a small problem being unsolved becomes a big problem.

Searching for "firefox, remove from dictionary, undo add" in Google produces 24 million results. It's a constant support question that has earned square miles of forum real estate. I doubt there's hardly any avid firefox users who haven't hit this wall.

To ignore that for a minimum of 5 years is by definition unresponsive.

I've said nothing rude. Only truth.

Feel free to kick me for it. I'm used to that behavior from autocrats.
Just edit persdict.dat or write an add-on to do so, rather than expect others to fix it.
@Scott M. Sanders

If we're expected to fix it ourselves then why bother having a bug forum?
Quentin, is this something that you would be interested to work on?  :-)
@Ehsan

This looks like an interesting bug. I'll start looking into it, so you can assign me if you want.

Just to be clear, you want the undo button to remove the word from the dictionary?
(In reply to Quentin Headen from comment #23)
> @Ehsan
> 
> This looks like an interesting bug. I'll start looking into it, so you can
> assign me if you want.
> 
> Just to be clear, you want the undo button to remove the word from the
> dictionary?

Yes. It should be a separate menu item from the Undo edit menu item, though.
Great!  I'm assigning this to you then.  :-)

Let me know if you needed some pointers on how this should be implemented.  I won't attempt to spoil the fun for you, so for now I'll just mention that you need to look at the mozIPersonalDictionary interface as a starting point.  :-)
Assignee: nobody → qheaden
I have to admit I kind of liked the God Emperor of Dune quote, but nonetheless we try to keep bugs focused on specific technical details of fixing the problem at hand, as opposed to a more informal forum of advocacy.

>If we're expected to fix it ourselves then why bother having a bug forum?

it's an effective way to recruit our core developers.
>If we're expected to fix it ourselves then why bother having a bug forum?

It's not a forum, where we discuss our taste in literature, reference dictatorships and try to impress women on the internet. It's a bug tracker — we report bugs, they get prioritized, and eventually fixed. 

I do think that people that report a bug in general don't need to get an email every time there's a small change in the bug, but should rather get notified when it's fixed or when another state change happens. In general, Bugzilla is way too noisy.
I have another question about desired behavior. How far back should the undo operation be available? In other words, if you add 16 words to the dictionary, for example, does the "undo add" function have to be able to remove all 16 words, or just the last word added?
Status: NEW → ASSIGNED
Just the last one. We're trying to fix the "oops" case here.
(In reply to Alex Limi (:limi) — Firefox UX Team from comment #29)
> Just the last one. We're trying to fix the "oops" case here.

For the implementation, I think we should use a stack though.  We can set the maximum stack size limit to 1 for now...
Where is the code for the context menu that pops up when you right click a misspelled word? I need to add the undo button there, but I don't know where to add the code for it.
I have a question. I am following the context element "spell-add-to-dictionary" as a guide on where to place my "spell-remove-from-dictionary" element. 

Why is the "spell-add-to-dictionary" context element defined in two different places? First, it is defined in the toolkit/content/widgets/textbox.xml file, then it is defined in the browser/base/content/browser-context.inc file.

Please let me know.

Thanks
(In reply to Quentin Headen from comment #34)
> Why is the "spell-add-to-dictionary" context element defined in two
> different places? First, it is defined in the
> toolkit/content/widgets/textbox.xml file, then it is defined in the
> browser/base/content/browser-context.inc file.

The former is used for <xul:textbox> generically (text input boxes in the Firefox user interface), while the latter is used for the Firefox content menu specifically (text fields in web pages). We probably want to make the same functionality changes to both (they share much of the code via InlineSpellChecker.jsm), but if you'd like you can focus on the browser/ version to start with (since it's a little easier to test).
Hey guys. Sorry I've been so slow on this bug. College assignments had me busy for a while. :)

So I will start back on this bug again.
This is a functionality patch. It does not contain any new tests, and it does not contain fixes for broken tests. I will submit it to the try server and fix any tests that are broken.

To try the enhancement, type a misspelled word, add it to the dictionary, then right click again to find a button that says "Undo Add To Dictionary". Click that, and retype the word. The word should be highlighted again.
Attachment #574106 - Flags: review?(ehsan)
Amazingly, my patch did not break any tests. Here is the TBPL report: https://tbpl.mozilla.org/?tree=Try&rev=5a6c01907165

I still have to write new tests for my patch, which I am working on now.
I need some help on where I should add tests at for this enhancement.
Can anyone apply my patch just to make sure I am going down the right track with this? Just verify if the functionality I implemented is correct.
Comment on attachment 574106 [details] [diff] [review]
Functionality Patch - (Tests Not Fixed)

Review of attachment 574106 [details] [diff] [review]:
-----------------------------------------------------------------

Your general approach looks good to me!  Please find my comments below.  Sorry that it took me so long to get back to you.  Please feel free to ping me on IRC if you need anything from me, because for the past few weeks I have been unable to keep up with the volume of my bugmail.  :-)

::: editor/txtsvc/public/nsIInlineSpellChecker.idl
@@ +67,5 @@
>  
>    nsIDOMRange getMisspelledWord(in nsIDOMNode aNode, in long aOffset);
>    void replaceWord(in nsIDOMNode aNode, in long aOffset, in AString aNewword);
>    void addWordToDictionary(in AString aWord);
> +  void removeWordFromDictionary(in AString aWord);

Please change the IID of nsInlineSpellChecker too.

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp
@@ +880,5 @@
> +  nsresult rv = mSpellCheck->RemoveWordFromDictionary(wordstr.get());
> +  NS_ENSURE_SUCCESS(rv, rv); 
> +
> +  mozInlineSpellStatus status(this);
> +  rv = status.InitForSelection();

Hmm, I'm not sure why you're only spell checking for the active selection...  Removing a suggested word could potentially change the result of spell checking on the entire document, right?

::: toolkit/content/InlineSpellChecker.jsm
@@ +40,5 @@
>  var gRegionBundle;
>  
>  function InlineSpellChecker(aEditor) {
>    this.init(aEditor);
> +  this.mAddedWordStack = []; //This must be persistant, thus initialized once

Nit: please add a trailing space after //.

@@ +283,5 @@
>  
>    // callback for adding the current misspelling to the user-defined dictionary
>    addToDictionary: function()
>    {
> +    this.mAddedWordStack.push(this.mMisspelling);

So, I think we want to limit the stack depth to 1 for now.  You could probably define a const at the top of the file named something like MAX_UNDO_STACK_DEPTH, and check for it here before pushing something on to the stack (and remove 1 element from the bottom of the stack before pushing it when you hit that limit.)

@@ +297,5 @@
> +    }
> +  },
> +  canUndo : function()
> +  {
> +    //Return true if we have words on the stack

Nit: same spacing nit as above.  :-)
(In reply to Quentin Headen from comment #39)
> I need some help on where I should add tests at for this enhancement.

I think your best bet is to add your tests to <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/test_contextmenu.html?force=1>, because it already has most of the infrastructure that you need.  Also, please remember to add the context menu entry to textbox.xml as well, as Gavin pointed out in comment 35.

Thanks!
Thanks a lot for the feedback Ehsan, and no problem with the late response. I've been ultra busy lately too, so I understand. :)

I'll get to work on this right away.
Attached patch Patch V2 (Almost Complete) (obsolete) — — Splinter Review 
Here is the second version of my patch. I made all of the changes suggested in the feedback from version 1 of my patch. I also modified the context menu mochitest to test for the "Undo Add to Dictionary" menu element.

I still haven't implemented the code needed for the browser's XUL textbox element. That will be in my next patch. The next patch will be the feature complete one.
Attachment #574106 - Attachment is obsolete: true
Attachment #583068 - Flags: review?(ehsan)
Attachment #574106 - Flags: review?(ehsan)
Comment on attachment 583068 [details] [diff] [review]
Patch V2 (Almost Complete)

Review of attachment 583068 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments fixed.  Please feel free to submit a new patch for the rest of the bug if you don't need me to look at this part again.  Thanks!

::: browser/base/content/test/test_contextmenu.html
@@ +467,5 @@
>          closeContextMenu();
>          openContextMenuFor(image_in_iframe); // Invoke context menu for next test.
>          break;
>  
> +    case 13:          

Nit: trailing spaces.

@@ +517,5 @@
>                           ].concat(inspectItems));
> +                        
> +        
> +        // Simulate a click on the "Add to Dictionary" button
> +        contextMenu.ownerDocument.getElementById("spell-add-to-dictionary").doCommand();

I wonder if you could simply use goDoCommand here...

::: toolkit/content/InlineSpellChecker.jsm
@@ +42,4 @@
>  
>  function InlineSpellChecker(aEditor) {
>    this.init(aEditor);
> +  this.mAddedWordStack = []; // This must be persistent, thus initialized once  

Please move this to the uninit function.  This way it would also be called as part of the init call above.

Also, I think the comment here is incorrect and needs to be removed.

@@ +285,5 @@
>    // callback for adding the current misspelling to the user-defined dictionary
>    addToDictionary: function()
>    {
> +    // Prevent the undo stack from growing over the max depth
> +    if(this.mAddedWordStack.length == MAX_UNDO_STACK_DEPTH)

Nit: space before parenthesis.

@@ +294,5 @@
>    },
> +  // callback for removing the last added word to the dictionary LIFO fashion
> +  undoAddToDictionary: function()
> +  {
> +    if(this.mAddedWordStack.length > 0)

Ditto.
Attachment #583068 - Flags: review?(ehsan) → review+
Attached patch Patch v3 (obsolete) — — Splinter Review 
This is the final, feature-complete patch. This version added the changes and tests to the XUL textbox element so it can also have the ability to undo words from the personal spell check dictionary.
Attachment #583068 - Attachment is obsolete: true
Attachment #583666 - Flags: review?(ehsan)
Attached patch Patch v3.1 — — Splinter Review 
I had to come out with a fix to the last patch. I discovered that moving the initialization of the mAddedWordStack array inside of the InineSpellChecker.uninit() method completely broke the undo add functionality. This is because uninit seems to be called every time the textbox is right clicked, thus clearing the stack before you can undo a word. This had to be initialized outside.
Attachment #583666 - Attachment is obsolete: true
Attachment #583711 - Flags: review?(ehsan)
Attachment #583666 - Flags: review?(ehsan)
Comment on attachment 583711 [details] [diff] [review]
Patch v3.1

Oh you're right, sorry about the wrong tip!

This looks great, r=me.  If you have run this through try server, please mark it as checkin-needed.  Thanks!
Attachment #583711 - Flags: review?(ehsan) → review+
Attachment #583711 - Flags: checkin?(ehsan)
I guess green means go. Here are the try server results https://tbpl.mozilla.org/?tree=Try&rev=25cbc985905d
Attachment #583711 - Flags: checkin?(ehsan) → checkin+
https://hg.mozilla.org/integration/mozilla-inbound/rev/314f940ca86e

Thanks a lot, Quentin!
Flags: in-testsuite+
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/314f940ca86e
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 722636
Blocks: 722640
Comment on attachment 583711 [details] [diff] [review]
Patch v3.1

>+  canUndo : function()
Why is this not get canUndo()?
Compare get canSpellCheck(), get enabled(), get overMisspelling().
(Also, space before colon is not normal JS object style.)
(In reply to neil@parkwaycc.co.uk from comment #53)
> Comment on attachment 583711 [details] [diff] [review]
> Patch v3.1
> 
> >+  canUndo : function()
> Why is this not get canUndo()?
> Compare get canSpellCheck(), get enabled(), get overMisspelling().
> (Also, space before colon is not normal JS object style.)

Please file a follow-up bug.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: