750292 - Fix/renovate feed opml import/export

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Comment 1

[alta88]

Attachment: patch

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.

Comment 2

[alta88]

Attachment: patch.

don't forget the suite strings..

Comment 3

[David :Bienvenu]

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 4

[David :Bienvenu]

Comment on attachment 628100 [details] [diff] [review]
patch.

clearing review request for de-bitrotting.

Comment 5

[alta88]

Attachment: patch

address comments and rebase.

Comment 6

[David :Bienvenu]

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).

Comment 7

[alta88]

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.

Comment 8

[David :Bienvenu]

(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 9

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 10

[David :Bienvenu]

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?

Comment 11

[alta88]

(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...

Comment 12

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 13

[alta88]

(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.

Comment 14

[alta88]

Attachment: patch

address comments.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Yes, much better on Mac on both counts!

Thanks,
Blake.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

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

Comment 17

[Philip Chee]

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

Comment 18

[Ian Neal]

(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.

Comment 19

[alta88]

(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.

Comment 20

[Rimas Kudelis [use needinfo to ping]]

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.

Comment 21

[Rimas Kudelis [use needinfo to ping]]

Attachment: Rename subscribe-OPMLExportFileDialogTitle and subscribe-OPMLExportDefaultFileName

Ludovic asked me if I could come up with a patch for the problem I mentioned in comment #20. Here it goes.

Comment 22

[David :Bienvenu]

Comment on attachment 637915 [details] [diff] [review]
Rename subscribe-OPMLExportFileDialogTitle and subscribe-OPMLExportDefaultFileName

I'll defer to alta88

Comment 23

[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).

Comment 24

[Magnus Melin [:mkmelin]]

Nobody thinks it's ideal, but thats the how it is.

Comment 25

[Mark Banner (:standard8)]

(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.

Comment 26

[Mark Banner (:standard8)]

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.

Comment 27

[rafael.linux.user]

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.