886124 - Add UI for extraction

Status: RESOLVED FIXED

(Calendar :: Lightning Only, enhancement)

Description

[Merike Sell (:merike)]

+++ This bug was initially created as a clone of Bug #403222 +++

Comment 1

[Merike Sell (:merike)]

Attachment: UI strings

These should be enough for adding message header buttons. I think "add" works better on buttons and "extract" in tooltips.

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Ok, will take care in the morning before the merge.

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I've taken the liberty to change the tooltips to provide a little more information.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 766444 [details] [diff] [review]
UI strings

https://hg.mozilla.org/comm-central/rev/f9e996b4a302

Comment 5

[Merike Sell (:merike)]

Attachment: extract-ui

This enables extraction functionality from UI:
* adds buttons for main mail toolbar and message header
* makes context-menu email-to-event/task conversion use extraction when datetime information is found in email

Basic conversion is just one click on the button and uses locale detection based on dictionaries installed and falls back to application locale. Drop-down lists all available languages for extraction without detection with recently used ones at the top. In cases where no datetime information is found in email or when there's an active selection containing no dates and times context-menu conversion should work exactly as before.

Considering how far Thunderbird 24 cycle is this is probably too high of a risk. In that case I propose to divide it into two for checkin:
* check in parts affecting extraction algorithm everywhere (calExtract.jsm is enough here)
* put UI parts only in central (and maybe aurora in case there will be intermediate releases?) and update the AMO extension to include the same UI and use the module in Lightning directly.

This would maximize potential users and ease of use while minimizing the risk of pushing this to everybody. Right now the extraction logic in extension is a much earlier version and doing this would mean there's no need to include it there any more.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 801735 [details] [diff] [review]
extract-ui

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

calExtract changes look fine. If you are comfortable pushing this, r=philipp with these changes. I haven't tested the patch yet.

::: calendar/base/content/calendar-extract.js
@@ +5,5 @@
> +Components.utils.import("resource://calendar/modules/calExtract.jsm");
> +Components.utils.import("resource://calendar/modules/calUtils.jsm");
> +
> +let calendarExtract = {
> +    onShowLocaleMenu: function onShowLocaleMenu(target) {

General note: Did you see nsIToolkitChromeRegistry.idl ? It seems to have a method to give you all locales for a package.

@@ +19,5 @@
> +        let entries = file.directoryEntries;
> +        let service = Components.classes["@mozilla.org/intl/stringbundle;1"]
> +                                .getService(Components.interfaces.nsIStringBundleService);
> +        // window bundle only available in compose but not in main window
> +        let bundle = service.createBundle("chrome://global/locale/languageNames.properties");

You can also get the strings using: cal.calGetString("languageNames", stringName, paramsOrNull, "global");

or if you want:

function getLangName(lang) cal.calGetString("languageNames", lang, null, "global");

@@ +20,5 @@
> +        let service = Components.classes["@mozilla.org/intl/stringbundle;1"]
> +                                .getService(Components.interfaces.nsIStringBundleService);
> +        // window bundle only available in compose but not in main window
> +        let bundle = service.createBundle("chrome://global/locale/languageNames.properties");
> +        let aConsoleService = Components.classes["@mozilla.org/consoleservice;1"]

The "a" prefix is only for arguments.

@@ +24,5 @@
> +        let aConsoleService = Components.classes["@mozilla.org/consoleservice;1"]
> +                                        .getService(Components.interfaces.nsIConsoleService);
> +
> +        let langs = new Array();
> +        while (entries.hasMoreElements()) {

for (let entry in fixIterator(entries, Components.interfaces.nsIFile)) {
...
}

(or possibly for each(), need to check iteratorUtils, be sure to import it too)

@@ +27,5 @@
> +        let langs = new Array();
> +        while (entries.hasMoreElements()) {
> +            let entry = entries.getNext();
> +            entry.QueryInterface(Components.interfaces.nsIFile);
> +            let localeJar = /^calendar-((..)-*(.*))\.jar$/;

Possibly define the regex outside of the while loop, this way it only needs to be constructed once

@@ +36,5 @@
> +                try {
> +                    langName = bundle.GetStringFromName(langName);
> +                } catch (ex) {}
> +
> +                let label = calGetString("calendar", "extractUsing", [langName]);

cal.calGetString

@@ +38,5 @@
> +                } catch (ex) {}
> +
> +                let label = calGetString("calendar", "extractUsing", [langName]);
> +                if (locale[3] != "") {
> +                    label += " (" + locale[3] + ")";

This should probably be another paramter of the string extractUsing? Since its optional, maybe just use two strings, one with and one without the extra param.

@@ +49,5 @@
> +        // sort
> +        let pref = "calendar.patterns.last.used.languages";
> +        let lastUsedLangs = cal.getPrefSafe(pref, "");
> +
> +        function compare(lastUsedLangs) {

possibly rename this function "createLanguageComptor" or something, the double compare is confusing.

@@ +70,5 @@
> +        langs.sort(compare(lastUsedLangs));
> +
> +        while (localeList.hasChildNodes()) {
> +            localeList.removeChild(localeList.firstChild);
> +        }

I believe we had a function in calendar-ui-utils for this, not sure if thats available.

@@ +72,5 @@
> +        while (localeList.hasChildNodes()) {
> +            localeList.removeChild(localeList.firstChild);
> +        }
> +
> +        for (let i = 0; i < langs.length; i++) {

If you don't need the index, you could use for each() ?

@@ +112,5 @@
> +        let date = new Date(message.date/1000);
> +        let time = (new Date()).getTime();
> +
> +        let locale = cal.getPrefSafe("general.useragent.locale", "en-US");
> +        let baseUrl = "jar:resource://calendar/chrome/calendar-LOCALE.jar!/locale/LOCALE/calendar/calendar-extract.properties";

This uri could possibly change, if the locales are not packaged as a .jar file. I don't know of a different way to get the url though.

@@ +142,5 @@
> +            let endGuess = extractor.guessEnd(guessed, !isEvent);
> +            let allDay = (guessed.hour == null || guessed.minute == null) && isEvent;
> +
> +            if (isEvent) {
> +                if (guessed.year != null)

Extra brackets

Comment 7

[Merike Sell (:merike)]

Attachment: module changes

calExtract.jsm changes only

Comment 8

[Merike Sell (:merike)]

Attachment: extract-ui_v2

(In reply to Philipp Kewisch [:Fallen] from comment #6)
> General note: Did you see nsIToolkitChromeRegistry.idl ? It seems to have a
> method to give you all locales for a package.
No. And it works great so using it now.

> The "a" prefix is only for arguments.
Turns out it was not used anywhere so I just removed it.

> This should probably be another paramter of the string extractUsing? Since
> its optional, maybe just use two strings, one with and one without the extra
> param.
Added another string.

> I believe we had a function in calendar-ui-utils for this, not sure if thats
> available.
Using functions there both for removing entries and constructing new ones now.

> If you don't need the index, you could use for each() ?
Using for...of instead as recommended by MDN.

> This uri could possibly change, if the locales are not packaged as a .jar
> file. I don't know of a different way to get the url though.
Is that done by someone generating releases too?

I'd appreciate if someone with a build could run calendar unit tests with the patch applied. On my setup I'm unable to run any unit tests successfully.

Comment 9

[Merike Sell (:merike)]

Attachment: extract-ui_v3

Adding previously forgotten lightning-toolbar.css.

Comment 10

[Merike Sell (:merike)]

Comment on attachment 819177 [details] [diff] [review]
extract-ui_v3

https://hg.mozilla.org/comm-central/rev/c81082fc62b3

Comment 11

[Martin Schröder [:mschroeder]]

Merike, can we close this issue and file new ones for followups?