Closed Bug 750292 Opened 12 years ago Closed 12 years ago

Fix/renovate feed opml import/export

Categories

(MailNews Core :: Feed Reader, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 15.0

People

(Reporter: alta88, Assigned: alta88)

References

Details

Attachments

(2 files, 3 obsolete files)

      No description provided.
Depends on: 721517
Blocks: 282435
Attached file patch (obsolete) —
this patch attempts:

1. numerous bug fixes; the current code neither imported nor exported all items in all situations.
2. export/import feeds to/from opml files respecting folder/<outline> hierarchy.  a folder is a parent non type=rss <outline> tag currntly.
3. export/import updated titles/summary mode pref.
4. make the subscribe tree smoother on changes.
5. implement feeds imports in Tools->Import, select existing account or create new account.
Assignee: nobody → alta88
Attachment #627323 - Flags: review?(dbienvenu)
Attached patch patch. (obsolete) — Splinter Review
don't forget the suite strings..
Attachment #627323 - Attachment is obsolete: true
Attachment #627323 - Flags: review?(dbienvenu)
Attachment #628100 - Flags: review?(dbienvenu)
Summary: Fix/renovate feed ompl import/export → Fix/renovate feed opml import/export
Comment on attachment 628100 [details] [diff] [review]
patch.

can you rebase this with the trunk? The license changes horked it. I'll try running with it after that...it looks reasonable, though.

some nits I noticed:

this looks like a place you could use the ? operator, and get rid of the braces:

+      if (aValue == "?")
+        el.setAttribute("mode", "undetermined")
+      else
+        el.setAttribute("mode", "determined")

better not to add in commented out code:

-      if (aFolder.server.type != "rss" || FeedUtils.isInTrash(aFolder))
+      if (aFolder.server.type != "rss" ||
+//          this.feedWindow.mActionMode == this.feedWindow.kImportingOPML ||
+          FeedUtils.isInTrash(aFolder))

if you need a document something, better to add a real comment.

similarly, here:

+      aWin.selectFolder(aLastFolder.parent);
+//      let indexInView = aWin.mView.getItemInViewIndex(aLastFolder.parent);
+//      aWin.mView.toggle(indexInView);
+//      aWin.mView.toggleOpenState(indexInView);

should we log the exception here?

+    try {
+      MailServices.accounts.saveAccountInfo();
+    }
+    catch (ex) {}

spaces around +'s here:

+    FeedUtils.log.debug("updateSubscriptionsDS: folder changed, name:unsubscribe - "+
+                        aFolder.filePath.path+":"+aUnsubscribe);
+

I'd really prefer let instead of var where appropriate, at least in new code like +function ImportFeeds()
Comment on attachment 628100 [details] [diff] [review]
patch.

clearing review request for de-bitrotting.
Attachment #628100 - Flags: review?(dbienvenu)
Attached patch patch (obsolete) — Splinter Review
address comments and rebase.
Attachment #628100 - Attachment is obsolete: true
Attachment #628753 - Flags: review?(dbienvenu)
Comment on attachment 628753 [details] [diff] [review]
patch

I tried exporting from an existing profile, and importing in a new profile, and it worked nicely through tools, import, created a feeds account automatically, etc, so kudos on that! (though it didn't fetch the imported feeds automatically; I had to hit get new mail).
Attachment #628753 - Flags: ui-review?(bwinton)
Attachment #628753 - Flags: review?(dbienvenu)
Attachment #628753 - Flags: review+
right, it doesn't do an auto fetch on import, i figured that's making too much of an assumption of the user's work flow, meaning they may be in organize mode and not want a flood right then.
(In reply to alta88 from comment #7)
> right, it doesn't do an auto fetch on import, i figured that's making too
> much of an assumption of the user's work flow, meaning they may be in
> organize mode and not want a flood right then.

yep, I can see that.
Comment on attachment 628753 [details] [diff] [review]
patch

I mostly like it, but there were a couple of things I noticed.

When I exported a feed, the most interesting bit of information, the filename, was cut off by the end of the dialog: http://is.gd/mawMVi

And, since the exporter didn't add the .opml extension, it made re-importing a pain.

But aside from those two things, ui-r=me!

Thanks,
Blake.
Attachment #628753 - Flags: ui-review?(bwinton) → ui-review+
when I exported my feeds, on Windows, I definitely got a .opml extension, and re-importing was a breeze. I wonder if this is a mac issue?
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #9)
> Comment on attachment 628753 [details] [diff] [review]
> patch
> 
> I mostly like it, but there were a couple of things I noticed.
> 
> When I exported a feed, the most interesting bit of information, the
> filename, was cut off by the end of the dialog: http://is.gd/mawMVi
> 

hmm, not sure what to do about it, other than add a tooltip.  the dialog is, of course, sizeable/persisted per user pref.

> And, since the exporter didn't add the .opml extension, it made re-importing
> a pain.

hmm2.  the filepicker, on win, will add the .opml in the suggested name.  i didn't like the fact that if the suggested name is changed to omit .opml, the fp doesn't add it automatically on save, but that gets into native code stuff and the api didn't seem to have anything to force the native fp to add a file ext.

now, are you saying that (are you on mac?) the suggested name didn't have .opml?  it really should; i did make a change to insert the ext into the fp api, and remove it from the string.  it works on win, but it may have been in the string for mac reasons...
I would be happy to change the text at the front from "Feeds in this account have been exported to" to something smaller, or maybe add some vertical space, and wrap the description…

I am on a Mac, and yeah, it didn't have ".opml" at the end.
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #12)
> I would be happy to change the text at the front from "Feeds in this account
> have been exported to" to something smaller, or maybe add some vertical
> space, and wrap the description…

of course.  it can wrap.  those who don't like such movement can widen the dialog.  new patch will wrap it.

> 
> I am on a Mac, and yeah, it didn't have ".opml" at the end.

added .opml back in the string, pls verify.

also there is a bit of css added to ensure the detail pane does not jump when moving between a feed folder and feed subscription item in the subscribe tree.  please verify it's the right css for mac.
Attached patch patchSplinter Review
address comments.
Attachment #628753 - Attachment is obsolete: true
Attachment #629263 - Flags: ui-review+
Attachment #629263 - Flags: review+
Yes, much better on Mac on both counts!

Thanks,
Blake.
Keywords: checkin-needed
comm-central: https://hg.mozilla.org/comm-central/rev/e3698c85b250
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
(In reply to Philip Chee from comment #17)
> Someone needs to fix Suite themes as well:
> http://mxr.mozilla.org/comm-central/source/suite/themes/classic/messenger/
> newsblog/feed-subscriptions.css
> http://mxr.mozilla.org/comm-central/source/suite/themes/modern/messenger/
> newsblog/feed-subscriptions.css

The changes to the strings in suite should have had an r= too
In theory should ask for a backout of the patch due to it not having all the relevant reviews.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Philip Chee from comment #17)
> Someone needs to fix Suite themes as well:
> http://mxr.mozilla.org/comm-central/source/suite/themes/classic/messenger/
> newsblog/feed-subscriptions.css
> http://mxr.mozilla.org/comm-central/source/suite/themes/modern/messenger/
> newsblog/feed-subscriptions.css

so file a bug, per usual.  the 1px css nit fix, despite being slipped into this patch for Tb, was introduced months ago and has nothing to to with this bug.

@neil, r? for suite strings.
Attachment #629263 - Flags: review+ → review?(neil)
Depends on: 762636
Attachment #629263 - Flags: review?(neil) → review+
Hi folks,
not sure if this has been mentioned already – I want to point out that you have changed two properties in newsblog.properties without changing their name:

-## LOCALIZATION NOTE(subscribe-OPMLExportFileDialogTitle): %S is the brandShortName
-subscribe-OPMLExportFileDialogTitle=%S OPML Export
-## LOCALIZATION NOTE(subscribe-OPMLExportDefaultFileName): %S is the brandShortName
-subscribe-OPMLExportDefaultFileName=My%SFeeds.opml
+## LOCALIZATION NOTE(subscribe-OPMLExportFileDialogTitle):
+## %1$S is the brandShortName, %2$S is the name of the feed account folder name.
+subscribe-OPMLExportFileDialogTitle=%1$S OPML Export - %2$S
+## LOCALIZATION NOTE(subscribe-OPMLExportDefaultFileName):
+## %1$S is the brandShortName (Thunderbird for example), %2$S is the account name.
+## The default extension (.opml) is added here as it is not automatically appended in the file picker on MacOS.
+subscribe-OPMLExportDefaultFileName=My%1$SFeeds-%2$S.opml

You should alter the entity name too, otherwise many localizers might not notice the change.
Ludovic asked me if I could come up with a patch for the problem I mentioned in comment #20. Here it goes.
Attachment #637915 - Flags: review?(dbienvenu)
Comment on attachment 637915 [details] [diff] [review]
Rename subscribe-OPMLExportFileDialogTitle and subscribe-OPMLExportDefaultFileName

I'll defer to alta88
Attachment #637915 - Flags: review?(dbienvenu) → review?(alta88)
i'm not familiar with the Tb localization process and thus why an entity value change would not be noticed, perhaps this can be explained.  i know that in babelzilla, entity value changes are obvious.

however, i think this practice of renaming an entity name is Very Bad.  is it merely Bad for well known best practices in software that avoid api/variable/etc name changes for all but the most stringent reasons.  it rises to Very Bad since uncaught changed entity names result in hard UI failure (unfortunately still) in xul.

this bug isn't the place to sort out the larger process question, and neither should the patch be attached to this bug (please open a new one).
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Nobody thinks it's ideal, but thats the how it is.
(In reply to alta88 from comment #23)
> i'm not familiar with the Tb localization process and thus why an entity
> value change would not be noticed, perhaps this can be explained.  i know
> that in babelzilla, entity value changes are obvious.

This is standard process in mozilla to change the entity name. Unfortunately not all l10n tools can detect string changes without entity name change.

> however, i think this practice of renaming an entity name is Very Bad.  is
> it merely Bad for well known best practices in software that avoid
> api/variable/etc name changes for all but the most stringent reasons.  it
> rises to Very Bad since uncaught changed entity names result in hard UI
> failure (unfortunately still) in xul.

Think about it slightly differently - you've changed the context of what the variable holds, it no longer holds the same as before, therefore you would change the variable name. That is the general rule here - if you change the meaning of the string, you change the entity name.

> this bug isn't the place to sort out the larger process question

Correct, it is one that has been discussed at length already. I don't know if l20n would fix it.

> neither should the patch be attached to this bug (please open a new one).

As it has already branched, I suspect that may be the thing to do, unless this needs backing out of the branch, which I'm trying to work out at the moment.
Attachment #637915 - Flags: review?(alta88)
I'm sorry for not getting back to this earlier. As it turns out, looking through the locales for the current beta shows that the majority of locales have actually picked up this change. I strongly suspect this is due to the fact that warnings are now generated if the number of parameters changes, so changing %S to %1$S and %2$S has the side-effect of notifying localisers who then can investigate. Obviously that's a bit more work for localisers, and we don't want to do it generally, but I don't think we need to rename these strings now - I'll keep an eye on warnings for the next sign-offs and file bugs for locales if appropriate.

Still not fixed. Folder structure when importing from OPML is not working. All "outline" entries ends on same level/folder. You can check it importing the next OPML file:

http://dip-badajoz.es/canales/lista_de_fuentes.opml

All feeds channels will be mixed at same level.

You need to log in before you can comment on or make changes to this bug.