Add dictionary switching in the compose window context menu

RESOLVED FIXED in Thunderbird 3.3a3

Status

enhancement
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: clarkbw, Assigned: protz)

Tracking

Trunk
Thunderbird 3.3a3
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

This is coming from a suggestion in the dev.usability google group
https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.usability/OxUZQQReYj0

Right now in Firefox you can easily switch spell check languages via the context menu but this isn't available in Thunderbird.
Bryan shall this make the UXprio list ?

Comment 2

9 years ago
Also a solution for this issue could be to include a multiple-language spell checker, like in Mac OS X 10.6 (but than a solution for all OSes is needed). How this will look like can be seen in the second screenshot in Bug 606156.
Assignee

Updated

8 years ago
Depends on: 626076
Assignee

Updated

8 years ago
Duplicate of this bug: 544448
Assignee

Comment 4

8 years ago
Taking this, since I just dug into the spellchecker code, I'll do that while the thing's still clear in my mind.
Assignee: nobody → jonathan.protzenko
Assignee

Comment 5

8 years ago
Attachment #517696 - Flags: ui-review?(clarkbw)
Assignee

Comment 6

8 years ago
Posted patch Tentative patch (obsolete) — Splinter Review
The patch that produces the screenshot above.
Assignee

Updated

8 years ago
Status: NEW → ASSIGNED
Assignee

Comment 7

8 years ago
Attachment #517697 - Attachment is obsolete: true
Assignee

Comment 8

8 years ago
Posted patch WIP (2) (obsolete) — Splinter Review
This patches fixes a few bugs, namely:
- make sure the add dictionaries entry in the main context menu is only shown when we can't spell check because we have no dictionaries,
- implement the addDictionaries() function.
Attachment #517698 - Attachment is obsolete: true

Comment 9

8 years ago
I know this is not included in this bug, apologies, but people reading this may know if anyone has considered improving the 'Add to dictionary' facility?

A big limitation of using multiple dictionaries is that there is only one personal dictionary. If you add a word to persdict, that word might apply in all languages (a place or personal name, say) or only in the language that you're spell-checking in. It needs an option: Add to current language or add to all languages. (I suspect that this problem may be less apparent to those of you reading this in the US than here in Europe ;-) 

But has this ever been raised? I can't find a bug for it. And it applies to Firefox too - though TB is more important to me because I have far more words in my TB persdict than any of my 4 Firefox ones. (It'd be nice to have a common personal dictionary too!)

And words in persdict  don't appear as suggestions either.

How and where to raise an enhancement bug for all these limitations in the personal dictionary - TB, Firefox, both?
Dave Royal, see bug 481884.
Assignee

Comment 11

8 years ago
Comment on attachment 517699 [details] [diff] [review]
WIP (2)

Sid, I think that's good enough for review. I've been running with it and testing it, and so far it looks to me.
Attachment #517699 - Flags: review?(sid.bugzilla)
Comment on attachment 517696 [details]
Screenshot with my patch running

awesome work! looks good
Attachment #517696 - Flags: ui-review?(clarkbw) → ui-review+
Assignee

Comment 13

8 years ago
Mark,
Status: ASSIGNED → NEW
Summary: add dictionary switching to the spell check context menu → Add dictionary switching in the compose window context menu
Assignee

Comment 14

8 years ago
Sorry, looks like changing the subject also reset the status field.
Status: NEW → ASSIGNED
Assignee

Updated

8 years ago
Attachment #517699 - Flags: review?(sid.bugzilla) → review?(bugzilla)
Comment on attachment 517699 [details] [diff] [review]
WIP (2)

If I alter the dictionary via the new context window, it doesn't update the spell check button menu or the context menu on the subject field. Also doesn't synchronise when changing it the other way around.

Full review context at: http://reviews.visophyte.org/r/517699/

on file: editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd line 93
> <!ENTITY pasteNoFormatting.accesskey "n">

We need to get SM to agree to this change as this file affects them as well
(just request review from Neil).


on file: mail/components/compose/content/MsgComposeCommands.js line 3917
> function addDictionaries() {

Although this does what we currently do elsewhere, would you mind changing it
so that it opens a content tab in a new window with that URL and amo as the
site for the click handler? (utilityOverlay.js should make this easy with
probably open in a new window).

(I've filed bug 640627/bug 604628 on updating our current instances).
Attachment #517699 - Flags: review?(bugzilla) → review-
Assignee

Comment 16

8 years ago
Alright. We have to keep in mind that Thunderbird can be opened with the -compose command line argument, which implies that there might not always be a tabbrowser available. I'm currently writing a addDictionaries function in MailUtils.js that will fallback to the current method if we fail to find an open mail:3pane window.

Other bugs should probably use this function. bug 640627 bug 640628 (your 2nd bug number had a typo) and also bug 640714 now.
Status: ASSIGNED → NEW
Assignee

Updated

8 years ago
Blocks: 640627, 640628, 640714
Assignee

Updated

8 years ago
Status: NEW → ASSIGNED
Assignee

Comment 17

8 years ago
FWIW, if I open the dictionaries page from AMO in a content tab, it does not offer to install the dictionary in Thunderbird, but tries to open the page in Firefox instead... To try this, run the piece of code below in the Error Console:

Components.classes['@mozilla.org/appshell/window-mediator;1'].getService(Components.interfaces.nsIWindowMediator).getMostRecentWindow("mail:3pane").document.getElementById("tabmail").openTab("contentTab", {contentPage: "https://addons.mozilla.org/en-US/thunderbird/language-tools/"});
Assignee

Comment 18

8 years ago
Jim, you might have ideas...
Assignee

Comment 19

8 years ago
Nevermind, I'd just forgotten to set a clickHandler.
Assignee

Comment 20

8 years ago
Posted patch WIP (3) (obsolete) — Splinter Review
All review comments have been taken into account.
- Added MailUtils.addDictionaries that does the right sequence of actions to open the AMO dictionary list, be it in a content tab if we happen to have a mail:3pane, or through the external browser instead.
- Fixed language propagation between the two menu items.
Attachment #517699 - Attachment is obsolete: true
Attachment #518498 - Flags: ui-review+
Attachment #518498 - Flags: review?(bugzilla)
Assignee

Comment 21

8 years ago
Comment on attachment 518498 [details] [diff] [review]
WIP (3)

Neil, I'm changing the access key of "Paste without formatting" from 'o' to 'n', because otherwise it would conflict with "Add to dictionary", which already has 'o'. We cannot change the latter, because it's a toolkit file, which we have no control over. Is that ok on the seamonkey side of things?
Attachment #518498 - Flags: review?(neil)
(In reply to comment #20)
> - Added MailUtils.addDictionaries that does the right sequence of actions to
> open the AMO dictionary list, be it in a content tab if we happen to have a
> mail:3pane, or through the external browser instead.

Can't you just include utilityOverlay.js and call openContentTab(url, "window", regexp)?
(In reply to comment #22)
> (In reply to comment #20)
> > - Added MailUtils.addDictionaries that does the right sequence of actions to
> > open the AMO dictionary list, be it in a content tab if we happen to have a
> > mail:3pane, or through the external browser instead.
> 
> Can't you just include utilityOverlay.js and call openContentTab(url, "window",
> regexp)?

Should we add the ability to open a content tab in the most recent 3pane? e.g. pass "lastwindow" instead of "window".
Whoops, nevermind. "tab" already does that. And I think we should probably do it that way, since the compose window isn't modal or anything like that.
Assignee

Comment 25

8 years ago
(In reply to comment #22)
> (In reply to comment #20)
> > - Added MailUtils.addDictionaries that does the right sequence of actions to
> > open the AMO dictionary list, be it in a content tab if we happen to have a
> > mail:3pane, or through the external browser instead.
> 
> Can't you just include utilityOverlay.js and call openContentTab(url, "window",
> regexp)?

utilityOverlay.js is designed to be included in a XUL file, and MailUtils.jsm is a JS code module, they don't share the same scope, so I don't think I can reuse it... unless I use mozIJSSubscriptLoader, is that what you had in mind?
(In reply to comment #25)
> utilityOverlay.js is designed to be included in a XUL file, and MailUtils.jsm
> is a JS code module, they don't share the same scope, so I don't think I can
> reuse it... unless I use mozIJSSubscriptLoader, is that what you had in mind?

Sorry, not well described. I was more thinking that we should just skip adding to MailUtils.jsm and call the utilityOverlay.js version direct.

The other alternative would be to move openContentTab into MailUtils.jsm.

Basically we shouldn't be duplicating that functionality which the patch would currently do.
Assignee

Comment 27

8 years ago
Posted patch WIP (4)Splinter Review
Attachment #518498 - Attachment is obsolete: true
Attachment #518678 - Flags: review?(bugzilla)
Attachment #518498 - Flags: review?(neil)
Attachment #518498 - Flags: review?(bugzilla)
Assignee

Comment 28

8 years ago
Comment on attachment 518678 [details] [diff] [review]
WIP (4)

So I took the latest remarks into account by adding a new function openDictionaryList() in utilityOverlay.js. That function directly uses openContentTab and does not duplicate the logic.

After thinking about it, I'm not sure this made much sense to store in a JSM, since this is purely UI-based interactions, so there's good chances we call these functions from a XUL file anyway.
Attachment #518678 - Flags: review?(neil)
Comment on attachment 518678 [details] [diff] [review]
WIP (4)

So there's still a minor issue in the drop-down from the spell check button, that when the language is changed elsewhere it doesn't always update properly, however that seems to be pre-existing, so we can deal with it in a follow-up bug.

>+function getDictionaryURL() {
>+  return Services.urlFormatter
>+    .formatURLPref("spellchecker.dictionaries.download.url");
>+}
>+

nit: I really can't think of a use for this function being separate, so just include it in openDictionaryList please.

>@@ -3902,16 +3914,41 @@

>+  gSpellChecker.init(editor);
>+  document.getElementById('menu_inlineSpellCheck')
>+    .setAttribute('disabled', !gSpellChecker.canSpellCheck);
>+  document.getElementById('spellCheckEnable')
>+    .setAttribute('disabled', !gSpellChecker.canSpellCheck);
>+  // If canSpellCheck = false, then hidden = false, i.e. show it so that we can
>+  // still add dictionaries. Else, hide that.
>+  document.getElementById('spellCheckAddDictionariesMain')
>+    .setAttribute('hidden', gSpellChecker.canSpellCheck);

nit: for these I think its better to align the dots as you've done elsewhere.

>+  <menu id="spellCheckDictionaries"
>+        label="&spellDictionaries.label;"
>+        accesskey="&spellDictionaries.accesskey;">
>+      <menupopup id="spellCheckDictionariesMenu">
>+          <menuseparator id="spellCheckLanguageSeparator"/>
>+          <menuitem id="spellCheckAddDictionaries"
>+                    label="&spellAddDictionaries.label;"
>+                    accesskey="&spellAddDictionaries.accesskey;"
>+                    oncommand="openDictionaryList();"/>
>+      </menupopup>
>+  </menu>

nit: we normally use 2-space indents for elements.
Attachment #518678 - Flags: review?(bugzilla) → review+
Assignee

Comment 30

8 years ago
Nits fixed, checked-in http://hg.mozilla.org/comm-central/rev/325c8df6ba18
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a3
Comment on attachment 518678 [details] [diff] [review]
WIP (4)

Clearing review request - Neil confirmed that the accesskey change was OK over irc.
Attachment #518678 - Flags: review?(neil)
Blocks: 665290
You need to log in before you can comment on or make changes to this bug.