Closed Bug 349586 Opened 18 years ago Closed 18 years ago

Sunbird/Lightning should migrate data from CalExt and Sunbird 0.2

Categories

(Calendar :: Import and Export, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Sunbird 0.5

People

(Reporter: jminta, Assigned: jminta)

References

Details

(Whiteboard: [patch in hand])

Attachments

(1 file, 16 obsolete files)

60.88 KB, patch
cmtalbert
: first-review+
cmtalbert
: second-review+
cmtalbert
: ui-review+
Details | Diff | Splinter Review
One of the features that the Firefox team often attributes a lot of its success to was the ability to automatically migrate your settings from your previous browser.  To increase user-satisfaction/conversion rate, we should offer similar functionality, detecting other calendar apps on the system and migrating data in from them.
Attached patch work in progress (obsolete) β€” β€” Splinter Review
This is a work in progress.  In my opinion, the most important bit for 0.3 is to migrate the old 0.2 data.  This part works great.  iCal migration almost works and we just need to figure out a reasonable solution to the fact that timezone data isn't stored in the file.  Evolution migration is untested, but largely similar to iCal.  In neither case have I made an effort to migrate remote-subscriptions yet.  Someone with a Windows system will need to handle the Outlook bit.

Migrating from a Firefox-calendar.xpi works in Sunbird, but not in Lightning, because Thunderbird puts its profile folder in a weird location.

I'll feel comfortable asking for review once we have iCal timezones solved, and maybe the Thunderbird profile issue.  Outlook import and remote subscription migration can be followup bugs.
Assignee: nobody → jminta
Status: NEW → ASSIGNED
is it possible to migrate data + settings from existing profile to another?
This is very comfortable when Sunbird is still developing hardly and we always should try to reproduce defect in clean profile.

Example. I installed some extensions and used some external calnedars is means that my profile is not useful for testing. I get many errors that does not exists in clean profile - old extensions left some stored settings etc

The solution is to keep sunbird settings in one file and extensions options (external calendar) in another. I would be able to migrate data + sunbird settings to clean profile and make sure that have clean env for testing (like migrate data and recovery all setting manually)
Blocks: 349048
After discussion in #calendar-qa, we'd like a partial fix for this bug to block 0.3.  In particular, we'd like to take a version of the patch that just includes the 0.2 migration stuff, and none of the rest.  We should discuss whether or not we can get away without doing the wizard UI at all for this.
Assignee: jminta → mattwillis
Status: ASSIGNED → NEW
Flags: blocking0.3+
Whiteboard: [l10n impact][need patch]
Attached patch rev1 - refinement of jminta's patch (obsolete) β€” β€” Splinter Review
Changes from jminta's patch:
- Removed iCal.app, Evolution, VistaCal, and OtherMozApps importers (or stubs)
- Refactored profile directory finding dealio. (Man that sucks.)
- Fixed some typos
- Used xreAppInfo to determine if we're Lightning (running in Tbird) or not
Attachment #234866 - Attachment is obsolete: true
Attachment #237217 - Flags: second-review?(dmose)
Attachment #237217 - Flags: first-review?(cmtalbert)
Whiteboard: [l10n impact][need patch] → [l10n impact][needs ctalbert dmose review]
Status: NEW → ASSIGNED
Blocks: 351743
Updating summary to limit scope to CalExt and Sunbird 0.2.

Importers for iCal.app, Evolution, n' friends spun off to bug 351743
Summary: Sunbird/Lightning should migrate data from other calendar apps on the system → Sunbird/Lightning should migrate data from CalExt and Sunbird 0.2
Comment on attachment 237217 [details] [diff] [review]
rev1 - refinement of jminta's patch

drive by:

+const THUNDERBIRD_UID = "{3550f703-e582-4d05-9a08-453d09bdfdc6}";
+
...
+        this._isLightning = !(appInfo.ID == SUNBIRD_UID)
Think you're gonna get some undefined errors here.

+
+        var dirEnum = localFile.directoryEntries;
In various cases (alternate OS, or custom profile), localFile will actually point to a non-existent file, in which case this will throw, which is really bad, since it will stop loading of the app.  Maybe we should just try/catch the whole thing, since we're late in the risk/reward cycle.
Attached patch rev2 - thanks to jminta (obsolete) β€” β€” Splinter Review
Fixes stuff jminta found.
Attachment #237217 - Attachment is obsolete: true
Attachment #237232 - Flags: second-review?(dmose)
Attachment #237232 - Flags: first-review?(cmtalbert)
Attachment #237217 - Flags: second-review?(dmose)
Attachment #237217 - Flags: first-review?(cmtalbert)
this is very important issue, please provide more no-technical information that will be helpful to verify this issue

thank you
Attached image Wizard page 1 (obsolete) β€”
Attached image Wizard page 2 (obsolete) β€”
Attached image Wizard page 3 (obsolete) β€”
per dmose and mvl in IRC, this is not going to make 0.3
Flags: blocking0.3+ → blocking0.3-
Target Milestone: --- → Sunbird 0.4
(In reply to comment #11)
> Created an attachment (id=237358) [edit]
> Wizard page 3
if cancel button is enabled it means that I can rollback all changes - remove imported data
If not only done button should be enabled, cancel must be disabled

this is similar to subscribing remote calendar: cancel is enabled but if you press it created calendar will not be removed so whatever you press cancel or done you get the same result
Comment on attachment 237232 [details] [diff] [review]
rev2 - thanks to jminta

Sorry. I have serious reservations. Please see my comment. (this space is too narrow).
Attachment #237232 - Flags: first-review?(cmtalbert) → first-review-
The r- is because this patch has some serious issues that need to be addressed. I got a bit of traction when jminta told me that I could force the migration to occur by simply starting Sunbird with an empty profile. I was able to debug a little bit until I became stuck again. This is what I've found:

--ISSUES--

1. Needs Comments. This is complex code that will have to be maintained as we release subsequent versions. It needs better commenting on how it works. For a simple example, if I had known about jminta's suggestion, I would have had this review done three days ago.

2. The else block in migration.js at line 307 (else { switch platform.toLowerCase) is missing a closing brace

3. if statement that this else belongs to (if statement beginning on line 282) makes use of the variable "diverge" but this variable is neither initialized nor declared.

4. In the same if statement (line 282), in the case statements within that if/else, you routinely grab the index for the profile path's "application" directory, but you do nothing with that value. For example: idx = ffSpec.indexOf("Thunderbird");

5. The case statement at line 311 should be case "winnt": not case "win32":

6. In the switch statement at line 308:
case "darwin":
case "win32":
This means that OS X and Windows will be treated the same. Is this what you intend?  I ask because in the switch above, these were separated.

6. When causing a migration by removing the sunbird profile directory, the checkCalExt function is called. I don't know if this is by design or not. It seems that this should be avoided by appropriate action at line 144 could prevent  checkCalExt from being called when our application is Sunbird. However, if you want to call checkCalExt for Sunbird, then you need to reconsider your profile string. For example, I found that when we went into the if statement at line 282, the ffSpec variable is: /C:/Documents%20and%20Settings/ctalbert/Application%20Data/Mozilla/Sunbird/Profiles/yei3ylch.default/Calendar
on my windows box. This does not seem to be the profile directory that your code is expecting, and that is why I wonder if you intend to be in this code at all when starting Sunbird on an empty profile.

7. I think that the problem at line 370:
+ gItems = icsImporter.importFromStream(inputStream, {});
+ putItemsIntoCal(cal);
is best solved by refactoring, not by recreating the proper side-effect behavior for the code to work by cheating the scope. As I noted earlier, as we add providers, this code will probably need to be maintained and extended. And when it is extended, I think the functionality in putItemsIntoCal will be crucial. I think it is worth refactoring the code in import-export.js so that we do not have to "cheat scope". In order to do this refactoring now, it is pretty easy: putItemsIntoCal is used in these places:
base\content\import-export.js(116): putItemsIntoCal(aCalendar);
base\content\import-export.js(126): putItemsIntoCal(calendars[0]);
base\content\import-export.js(130): args.onOk = putItemsIntoCal;
base\content\migration.js(370): putItemsIntoCal(cal);
It is defined here:
base\content\import-export.js(138): function putItemsIntoCal(destCal) {
Perhaps we could simply use an optional parameter on putItemsIntoCal that is an inputStream, and that inputStream if specified will override the gItems variable that lives in import-export.js. Even that simple refactoring would make a huge difference.

8. Parsing the RDF as XML...I heartily agree with the comments here. I heartily agree that the RDF parser is too complex and too difficult to use, and I would argue that this code is more maintainable than actually using the RDF parser. 

However, we decided to use our RDF technology for Calendar.rdf (either rightly or wrongly), then we should continue to use that technology when we need to get to that file. Since this code will be maintained and extended by several pairs of hands, I think the code in this file does have to set the standard for those that will follow. I think that we should use the RDF interfaces to get the data out, not parse the RDF as though it were XML.

I'll be happy to review an updated patch. Best of luck.
Now that this isn't an 0.3 blocker, I'm handing it back to jminta.
Assignee: lilmatt → jminta
Status: ASSIGNED → NEW
Whiteboard: [l10n impact][needs ctalbert dmose review]
Attachment #237232 - Flags: second-review?(dmose)
Attached patch for testing (obsolete) β€” β€” Splinter Review
Putting this here for better test coverage.

Changes since last revision:
-added some more explanatory comments
-refactored the getProfile stuff, to make it reusable, and hopefully easier to understand
-fixed syntax errors pointed to by clint
-rolled in bug 349048, since it got reject in favor of this patch
-restored ical and evolution import.
-added import for multiple profiles
-added import for Sunbird if you're lightning, and vice versa
-renamed checkCalExt to checkOldCal, since it's really a generic 0.2 importer (this is what threw clint off about running it in Sunbird)
-refactored import-export code to avoid scope tricks.  Now we just call a legally global function.

Testing situation:
Since the iCal.app framework was the only broken importer last I touched this, that's the one I tested.  I don't yet have a good build environ for lightning yet, so I only tested in Sunbird.  I'm assuming the 0.2-importer and evolution-importer still work, since they did before

I will file follow-up bugs for the following:
-Exposing a Tools menu-item to manually trigger import (a la Firefox)
-Role in a 'Manual' option (again a la Firefox).  This ought to replace the Import option under the File menu.
-Create importer for modern moz-cal apps (If you previously installed Sunbird and now are in Lightning, we should read+import storage.sdb)

I'll ask for review once Lightning and the cal-ext importer have gotten a bit more test-coverage.
Attachment #237232 - Attachment is obsolete: true
(In reply to attachment 242827 [details] [diff] [review])
Drive-by comments from IRC:

> ? calendar/locales/en-US/chrome/calendar/migrationWizard.dtd.txt
Awww:

> +     * Sunbird:
> +     *     Unix:     ~jdoe/.mozilla/sunbird/
> +     *     Windows:  %APPDATA%\Mozilla\sunbird\Profiles
> +     *     Mac OS X: ~jdoe/Library/Application Support/Sunbird/Profiles
I apparently missed the capital S in Sunbird for Windows

> +disableExtTitle=Disabling old extension
> +disableExtMessage=You currently have the old calendar extension installed,
>  which is not compatible with Lightning.  The old extension will now be
>  disabled and Thunderbird will restart.
You prob want $MOZ_APP_NAME or something to handle Tb/Fx
Comment on attachment 242827 [details] [diff] [review]
for testing

+  content/lightning/lightningMigration.xul		(content/lightningMigration.xul)

These need to be ltnMigration.xul

+            evoFile.initWithPath(icalSpec);
+        } else {
+            var diverge = icalSpec.indexOf(".thunderbird");
+            if (diverge == -1) {
These should each be evoSpec, not icalSpec

with those changes, Lightning works great, and 0.2 migration also tested successfully.  I'll try to get a good, clean patch up soon, but right now my tree is a mess.
Attached patch migration v2 (obsolete) β€” β€” Splinter Review
This patch brings in the minor changes I found when testing Lightning.  Should be all ready for review now.
Attachment #242827 - Attachment is obsolete: true
Attachment #243717 - Flags: ui-review?(mvl)
Attachment #243717 - Flags: second-review?(mvl)
Attachment #243717 - Flags: first-review?(lilmatt)
Comment on attachment 243717 [details] [diff] [review]
migration v2

I had a bunch of comments, but I couldn't sleep so I gave you a gift and addressed all my review comments but one in another patch.
Attachment #243717 - Flags: ui-review?(mvl)
Attachment #243717 - Flags: second-review?(mvl)
Attachment #243717 - Flags: first-review?(lilmatt)
Attachment #243717 - Flags: first-review-
Attached patch migration v3 (obsolete) β€” β€” Splinter Review
The remaining comment I had was that on Mac, the migration wizard appears in the upper left corner, not parented to the main calendar window, but instead to the hidden window.

This means that you end up having to click on the Sunbird dock icon to see any errors that may have occurred in the import, and is altogether goofy.

I tried to fix it in a number of ways, such as only including the migration.js script in calendar.xul (so hiddenWindow.xul didn't have it) and using the window mediator/watcher to find the "calendarMainWindow" window and use that to parent the openDialog() call.  They didn't work, so I pulled them out.

The only migration I tried was iCal.app, as I don't have CalExt or Evo on this machine.  That worked, except it had errors due to events with duplicate UIDs in multiple calendars.  We have a bug on handling duplicate UIDs more gracefully, so I won't fault you there. :)

Minusing solely on the goofy window parenting issue.
Attachment #243717 - Attachment is obsolete: true
Attachment #243880 - Flags: ui-review?(mvl)
Attachment #243880 - Flags: first-review-
Comment on attachment 243880 [details] [diff] [review]
migration v3

Index: calendar/base/content/migration.js
>+var gDataMigrator = {
>+    _isLightning: false,
>+    _isInFirefox: false,
>+    _platform: null,

the underscore prefix isn't used in calendar code. Let's not introduce yet another code style...
No longer blocks: 351743
Before I forget, we should add this to the <wizard> element in migration.xul

windowtype="Calendar:MigrationWizard"
Attached patch migration v4 (obsolete) β€” β€” Splinter Review
The Mac window parenting issue was due to the "modal" attribute.
We're borrowing this approach from Fx and Tb.

r=lilmatt
Attachment #243880 - Attachment is obsolete: true
Attachment #244165 - Flags: first-review+
Attachment #243880 - Flags: ui-review?(mvl)
Whiteboard: [patch in hand]
Comment on attachment 244165 [details] [diff] [review]
migration v4

back to clint for more eyes on this
Attachment #244165 - Flags: second-review?(cmtalbert)
Comment on attachment 244165 [details] [diff] [review]
migration v4

This patch looks great.
I have made a list of the follow up bugs that need to be filed from here and this is r+ with those bugs filed. The bugs are: 
1. //XXX may want to wire this into the 'cancel' function once that's written -- need a bug for this
2. //XXX also define a category and an interface here for pluggability -- need a bug for this
3. In the iCal.app migrator we strip out the timezones because our code cannot deal with foreign timezones at the moment. Please file a follow-up bug that will not strip the timezones once we have completed our foreign timezone story.
Great job!
Attachment #244165 - Flags: second-review?(cmtalbert) → second-review+
Comment on attachment 244165 [details] [diff] [review]
migration v4

Somehow the UI-review flag got lost in the process.
Attachment #244165 - Flags: ui-review?(mvl)
Comment on attachment 244165 [details] [diff] [review]
migration v4

Discussed the UI on irc with joey. We concluded that the first screen needs some welcome text, and that the third screen can go, because the second screen already says it's finished.
Attachment #244165 - Flags: ui-review?(mvl) → ui-review-
Attached patch migration v5 (obsolete) β€” β€” Splinter Review
Making the changes suggested by mvl.  Had to put in a hack in order to get proper working text for lightning, but once we have branding for lightning in place, that can go away.
Attachment #244165 - Attachment is obsolete: true
Attachment #245180 - Flags: ui-review?
Attachment #245180 - Flags: first-review?(lilmatt)
Attachment #245180 - Flags: ui-review? → ui-review?(mvl)
Attached image Wizard new Page 1 (obsolete) β€”
Attached image Wizard New Page 2 (obsolete) β€”
Attachment #237355 - Attachment is obsolete: true
Attachment #237356 - Attachment is obsolete: true
Attachment #237358 - Attachment is obsolete: true
Comment on attachment 245180 [details] [diff] [review]
migration v5

>Index: calendar/base/content/migration.js
>===================================================================
>+    loadMigrators: function gmw_load() {
>+        var listbox = document.getElementById("datasource-list");
>+
>+        //XXX Once we have branding for lightning, this hack can go away
>+        var sbs = Cc["@mozilla.org/intl/stringbundle;1"]
>+                  .getService(Ci.nsIStringBundleService);
>+        var props = sbs.createBundle("chrome://calendar/locale/migration.properties");
>+        // XXX Having issues using gDataMigrator for this, making call directly
>+        var appInfo = Cc["@mozilla.org/xre/app-info;1"]
>+                      .getService(Ci.nsIXULAppInfo);
>+        if (!(appInfo.ID == SUNBIRD_UID)) {
>+            var desc = document.getElementById("wizard-desc");
>+            // Since we don't translate "Lightning"...
>+            desc.textContent = props.formatStringFromName("migrationDescription",
>+                                                          ["Lightning"],
>+                                                          1);
>+        }
Please replace the "Having issues" comment with a blank line, and add blank lines between sbs and props, props and appinfo, and appinfo and the if statement.

>+    checkAndMigrate: function gdm_migrate() {
>+        var appInfo = Cc["@mozilla.org/xre/app-info;1"]
>+                      .getService(Ci.nsIXULAppInfo);
>+        this._isLightning = !(appInfo.ID == SUNBIRD_UID)
>+        LOG("_isLightning is: " + this._isLightning);
>+        if (appInfo.ID == FIREFOX_UID) {
>+            this._isInFirefox;
This should be this._isInFirefox == true;


>Index: calendar/base/content/migration.xul
>===================================================================
>+    <script type="application/x-javascript" src="chrome://calendar/content/migration.js"/>
>+    <script type="application/x-javascript" src="chrome://calendar/content/import-export.js"/>
>+    <script type="application/x-javascript" src="chrome://calendar/content/calendarUtils.js"/>
Let's wrap and align these 3 lines so we're under 80 chars.


>Index: calendar/locales/en-US/chrome/calendar/migration.dtd
>===================================================================
>+<!ENTITY migration.title "Data Import" >
>+<!ENTITY migration.list.description "Welcome to &brandFullName;.  Which of these existing data sources would you like to automatically import?">
>+<!ENTITY migration.progress.description "Importing selected data">
>+<!ENTITY migration.finished.description "Data import complete">
Personally, I would prefer putting "Welcome to fooApp" in the title location and only put the instructions in the description area.  It's dmose/mvl's call.


>Index: calendar/locales/en-US/chrome/calendar/migration.properties
>===================================================================
>+migratingApp = Migrating %1$SÒ€¦
The UTF-8 ellipsis got munged here.


Also, Joey wanted to cache the directory service and lose the io service.

Let's around go once more while we wait for ui-r.
Attachment #245180 - Flags: first-review?(lilmatt)
Comment on attachment 245180 [details] [diff] [review]
migration v5

>+var gDataMigrator = {
>+    _isLightning: false,
>+    _isInFirefox: false,
>+    _platform: null,

What happened to my comment about not intructing a new style with those underscores? Please remove them. I think they want to be an m instead: mIsLightning.
(In reply to comment #33)
> >+    <script type="application/x-javascript" src="chrome://calendar/content/migration.js"/>
> >+    <script type="application/x-javascript" src="chrome://calendar/content/import-export.js"/>
> >+    <script type="application/x-javascript" src="chrome://calendar/content/calendarUtils.js"/>
> Let's wrap and align these 3 lines so we're under 80 chars.

Don't do that. This way is much, much more readable. 80 chars is a guide, not a hard rule. If over 80 chars is more readable, just go for it.
Attached patch Migration v6 (obsolete) β€” β€” Splinter Review
Matt and Joey, I've asked for both your reviews on this. Here is what changed:
1. Added a gDataMigrator.isLighting accessor for the migrationWizard to use.
2. Provided caching for the dirService and ioService XPCOM Services
3. ***Corrected a bug in the migrateChecked::getNextMigrator function***

Number 3 is what I most want you to take a look at. As I worked on this I saw that the migrator is given a callback to getNextMigrator. Upon a migrator's completion, this would call back into getNextMigrator _without_ incrementing i. Then the original code would attempt to call migrator i again. If that second call into migrator i did not throw, we would be stuck in an infinite loop.

I changed this code to prevent duplicate calls into the same migrator by having the code remember the next migrator's index. It can then prevent calls into the same migrator by detecting a mismatch between i and the next migrator's index.
Attachment #245180 - Attachment is obsolete: true
Attachment #245364 - Flags: second-review?(jminta)
Attachment #245364 - Flags: first-review?(lilmatt)
Attachment #245180 - Flags: ui-review?(mvl)
Attachment #245364 - Flags: second-review?(jminta)
Attachment #245364 - Flags: first-review?(lilmatt)
Attached patch Migration v7 (obsolete) β€” β€” Splinter Review
Sorry, I didn't see mvl's comments before checking in the last patch.  Same comments apply about the stuff in getNextMigrator.
Attachment #245364 - Attachment is obsolete: true
Attachment #245366 - Flags: ui-review?(mvl)
Attachment #245366 - Flags: second-review?(jminta)
Attachment #245366 - Flags: first-review?(lilmatt)
Comment on attachment 245366 [details] [diff] [review]
Migration v7

>Index: calendar/base/content/import-export.js
>-            args.onOk = putItemsIntoCal;
>+            args.onOk = function(aCal) { putItemsIntoCal(aCal, items); };
Shouldn't this not be an anon function?

>Index: calendar/base/content/migration.js
>+ * Contributor(s):
>+ *   Matthew Willis <mattwillis@gmail.com>
Stick your name in here big boy.

>+    /**
>+    * Gets the value for mIsLightning, and sets it if this.mIsLightning is
>+    * not initialized. This is used by objects outside gDataMigrator to
>+    * access the mIsLightning member.
>+    */
Your comment alignment's goofy here.


These are all tiny. r+ with those fixed.
Attachment #245366 - Flags: first-review?(lilmatt) → first-review+
Attached patch Migration v8 (obsolete) β€” β€” Splinter Review
Addressed lilmatt's comments on version 7 and carried his r+ forward.
Attachment #245366 - Attachment is obsolete: true
Attachment #245464 - Flags: ui-review?(mvl)
Attachment #245464 - Flags: second-review?(jminta)
Attachment #245464 - Flags: first-review+
Attachment #245366 - Flags: ui-review?(mvl)
Attachment #245366 - Flags: second-review?(jminta)
Comment on attachment 245464 [details] [diff] [review]
Migration v8

+            if (migrators[i]) {
+                if (i == nextMig) {
+                    LOG("starting migrator: " + migrators[i].title);
+                    label.value = props.formatStringFromName("migratingApp",
+                                                             [migrators[i].title],
+                                                             1);
+                    meter.value = i/migrators.length*100;
+                    migrators[i].args.push(getNextMigrator);
+                    nextMig++;
+                    try {
+                        migrators[i].migrate.apply(migrators[i],
+                                                   migrators[i].args);
+                    } catch (e) {
+                        LOG("Failed to migrate: " + migrators[i].title);
+                        LOG(e);
+                        i++; 
+                        getNextMigrator();
+                    }
+                } else {
+                    // We have called back onto the same migrator go to the
+                    // next migrator
+                    i++;
+                    getNextMigrator();
+                }

I think it would be cleaner to just increment i earlier.  Something like:
  if (migrators[i]) {
    var mig = migrators[i];
    i++;
    LOG("starting migrator: " mig.title);
    ...

Then the catch() block can just LOG and do getNextMigrator.

+    mDirService: Cc["@mozilla.org/file/directory_service;1"]
+                 .getService(Ci.nsIProperties),
+    mIoService: Cc["@mozilla.org/network/io-service;1"]
+                .getService(Ci.nsIIOService),
This isn't the ideal way to cache a service, because it will require that we make these getService calls when the variable is declared, rather than when we need the service.  This will impact startup time.  The simple case is:
js> function foo() { print('in foo'); return 1; }
js> var bar = { baz: foo() };
in foo
js> bar.baz;
1
(caching works, but declared at startup)

A better way would be like http://lxr.mozilla.org/mozilla/source/calendar/base/content/preferences/advanced.js#77
Which caches AND only makes the first call if we're sure we're going to need the service (if we're actually trying to migrate).

         if (count.value == 1) {
             // There's only one calendar, so it's silly to ask what calendar
             // the user wants to import into.
             putItemsIntoCal(calendars[0]);
         } else {
We want |items| as a second argument here too, otherwise we won't import anything.

r2=jminta with those comments, based on interdiff
Attachment #245464 - Flags: second-review?(jminta) → second-review+
Comment on attachment 245464 [details] [diff] [review]
Migration v8

>+<!ENTITY migration.title "Data Import" >

The window title should at least include the app name, to make it clear which app in importing data.

>+<!ENTITY migration.list.description "Welcome to &brandFullName;.  Which of these existing data sources would you like to automatically import?">

What is a 'data source'? That's too geeky.
On IRC, we cam up with: "Welcome to sunbird. Do you want to import data from other applications?", but thinking of that, it has the problem of being a question, which you can't answer 'no' to, because there is no 'no' button.
maybe "Welcome to sunbird. Select the application from which you want to import data". But other suggestions are welcome.


>+migrationDescription=Welcome to %1$S.  Which of these existing data sources would you like to automatically import?

Huh? Is this string used, or the entity above? One copy would be enough :)

>+disableExtText = You have an extension installed which is not compatible with Lightning.  It will be disabled and %1$S will restart.

Can you include the name of the extension? Worded like this, the statement is pretty general and vague. We should make it clear that the extension is the old calendar extension, so it makes sense that lightning would disable it.
Attachment #245464 - Flags: ui-review?(mvl) → ui-review-
(In reply to comment #41)
How about:

+-------------------------------------------------------------------+
| Welcome to $appname                                               |
|                                                                   |
| $appname can import calendar data from many popular applications. |
| Data from the following applications were found on your computer. |
| Please select which of these you'd like to import data from.      |
|                                                                   |
|  +-------------------------------------------------------------+  |
|  | [X] Apple iCal                                              |  |
|  | [X] Evolution                                               |  |
Revision after chatting with mvl:

+-------------------------------------------------------------------+
| ## $appname: Data Import                                 [_][D][X]|
+-------------------------------------------------------------------+
| Welcome to $appname                                               |
|                                                                   |
| $appname can import calendar data from many popular applications. |
| Data from the following applications were found on your computer. |
| Please select which of these you'd like to import data from.      |
|                                                                   |
|  +-------------------------------------------------------------+  |
|  | [X] Apple iCal                                              |  |
|  | [X] Evolution                                               |  |
(In reply to comment #43)

received ui-r+ from mvl via IRC for the layout in comment #43
Attached patch Migration v9 β€” β€” Splinter Review
One last patch. Addressing jminta and mvl's comments. Also including new UI from last comment. Carrying forward jminta and mvl's reviews.
Attachment #245182 - Attachment is obsolete: true
Attachment #245184 - Attachment is obsolete: true
Attachment #245464 - Attachment is obsolete: true
Attachment #245522 - Flags: ui-review+
Attachment #245522 - Flags: second-review+
Attachment #245522 - Flags: first-review?(lilmatt)
Comment on attachment 245522 [details] [diff] [review]
Migration v9

Carrying lilmatt's review forward.
Attachment #245522 - Flags: first-review?(lilmatt) → first-review+
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: