Fix/renovate feed opml import/export

RESOLVED FIXED in Thunderbird 15.0

Status

MailNews Core
Feed Reader
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

unspecified
Thunderbird 15.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
(Assignee)

Updated

5 years ago
Depends on: 721517
(Assignee)

Updated

5 years ago
Blocks: 282435
(Assignee)

Comment 1

5 years ago
Created attachment 627323 [details]
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.
Assignee: nobody → alta88
Attachment #627323 - Flags: review?(dbienvenu)
(Assignee)

Comment 2

5 years ago
Created attachment 628100 [details] [diff] [review]
patch.

don't forget the suite strings..
Attachment #627323 - Attachment is obsolete: true
Attachment #627323 - Flags: review?(dbienvenu)
Attachment #628100 - Flags: review?(dbienvenu)
(Assignee)

Updated

5 years ago
Summary: Fix/renovate feed ompl import/export → Fix/renovate feed opml import/export

Comment 3

5 years ago
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

5 years ago
Comment on attachment 628100 [details] [diff] [review]
patch.

clearing review request for de-bitrotting.
Attachment #628100 - Flags: review?(dbienvenu)
(Assignee)

Comment 5

5 years ago
Created attachment 628753 [details] [diff] [review]
patch


address comments and rebase.
Attachment #628100 - Attachment is obsolete: true
Attachment #628753 - Flags: review?(dbienvenu)

Comment 6

5 years ago
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+
(Assignee)

Comment 7

5 years ago
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

5 years ago
(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+

Comment 10

5 years ago
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?
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
Created attachment 629263 [details] [diff] [review]
patch

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

Updated

5 years ago
Keywords: checkin-needed
comm-central: https://hg.mozilla.org/comm-central/rev/e3698c85b250
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0

Comment 17

5 years ago
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

5 years ago
(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 → ---
(Assignee)

Comment 19

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #629263 - Flags: review+ → review?(neil)

Updated

5 years ago
Depends on: 762636

Updated

5 years ago
Attachment #629263 - Flags: review?(neil) → review+

Comment 20

5 years ago
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

5 years ago
Created attachment 637915 [details] [diff] [review]
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.
Attachment #637915 - Flags: review?(dbienvenu)

Comment 22

5 years ago
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)
(Assignee)

Comment 23

5 years ago
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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Comment 24

5 years ago
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.
(Assignee)

Updated

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.