Closed Bug 1420473 Opened 4 years ago Closed 3 years ago

Refactor and simplify the feed Subscribe dialog options updates

Categories

(MailNews Core :: Feed Reader, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird60 fixed, thunderbird61 fixed)

RESOLVED FIXED
Thunderbird 59.0
Tracking Status
thunderbird60 --- fixed
thunderbird61 --- fixed

People

(Reporter: alta88, Assigned: alta88)

References

Details

Attachments

(4 files, 4 obsolete files)

1. Only show buttons that apply to the selected account, folder, or feed tree item.
2. Some options are instant apply and some need Update; this is confusing and double processing. All options will be instant apply and the Update button replaced by Verify (the url), except for..
3. An edit to an existing feed url must still require an Update. If the value is changed, the button will change from Verify to Update.
4. Buttons are only enabled if the url is focused (folders) or has a value (feeds).

The Update button currently verifies an existing stored feed url for network/cert/valid xml errors, so Update isn't accurate.
Attached patch subscribeTweaks.patch (obsolete) — Splinter Review
Assignee: nobody → alta88
Attachment #8931741 - Flags: review?(mkmelin+mozilla)
Attached patch feedsImport.patch (obsolete) — Splinter Review
Some tweaks for import. Remove cached feeds/feeditems db references, as they are already cached and cause stale problems for the account removal/recreation edge case as they aren't cleaned up on removal.
Attachment #8931743 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8931741 [details] [diff] [review]
subscribeTweaks.patch

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

::: mail/locales/en-US/chrome/messenger-newsblog/feed-subscriptions.dtd
@@ +10,5 @@
>  <!ENTITY feedTitle.accesskey         "T">
>  
>  <!ENTITY feedLocation.label          "Feed URL:">
>  <!ENTITY feedLocation.accesskey      "F">
> +<!ENTITY feedLocation.placeholder    "Enter a valid feed url">

you need to bump the entity key, so that localizers will notice

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +1153,5 @@
>    },
>  
>    setPrefs: function(aNode)
>    {
> +FeedUtils.log.debug("setPrefs: aNode.id:disabled - " + aNode.id+":"+aNode.disabled);

remove

@@ +1299,5 @@
>    },
>  
>    updateButtons: function (aSelectedItem)
>    {
> +FeedUtils.log.debug("updateButtons: ");

remove
also debugging left in a few other places

@@ +1364,4 @@
>      if (isServer)
>      {
> +      let disable = addFeedButton != focusedElement &&
> +                    !locationValue.hasAttribute("focused") && !locationValue.value;

I'd not use this intermediate variable. It's just used once

@@ +1376,5 @@
> +      updateEnabled.disabled = autotagEnable.disabled = disable;
> +
> +      disable = addFeedButton != focusedElement &&
> +                !locationValue.hasAttribute("focused") && !locationValue.value;
> +      addFeedButton.disabled = disable;

here too just assign, without messing around with disable

@@ +1395,5 @@
> +      // Set button state.
> +      let disable = (focusedElement != updateFeedButton ||
> +                     updateFeedButton.disabled) &&
> +                    !locationValue.hasAttribute("focused");
> +      updateFeedButton.disabled = disable;

here too
Attachment #8931743 - Flags: review?(mkmelin+mozilla) → review+
Attached patch subscribeTweaks.patch (obsolete) — Splinter Review
update for comments.
Attachment #8931741 - Attachment is obsolete: true
Attachment #8931741 - Flags: review?(mkmelin+mozilla)
Attachment #8932935 - Flags: review+
unbitrot.
Attachment #8931743 - Attachment is obsolete: true
Attachment #8932936 - Flags: review+
Keywords: checkin-needed
Attached patch subscribeTweaks.patch (obsolete) — Splinter Review
entity bump.
Attachment #8932935 - Attachment is obsolete: true
Attachment #8932945 - Flags: review+
and in the xul.
Attachment #8932945 - Attachment is obsolete: true
Attachment #8932949 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1c1e48b383bd
Refactor and simplify the feed Subscribe dialog options updates. r=mkmelin
https://hg.mozilla.org/comm-central/rev/1654a1adc838
Refactor and simplify the feed Subscribe dialog options updates, import/export part. r=mkmelin CLOSED TREE
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
Attached patch hide.patchSplinter Review
follow up, buttons hidden initially to prevent them showing briefly on load.
Attachment #8933457 - Flags: review+
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f44c101732e9
Follow-up: hide buttons initially. r=me
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 1439393
the only possible change in this patch that may remotely cause Bug 1439393 would be the FeedUtils.jsm hunk in
https://hg.mozilla.org/comm-central/rev/1654a1adc838

so that hunk could be reverted.  it was implemented long ago as an extra cache in case the even more ancient comment
"GetDataSourceBlocking has a cache, so it's cheap to do this again once we've already done it once." was
not true.

the cache was removed since the cached entry wasn't being deleted on account deletion, and absent a restart a new account would get the old account's cache, and assumed the above comment re rdf was true. it is possible something in core rdf was changed to make the old comment untrue, in fact.

it's entirely possible that processing huge feed files has nothing to do with rdf or this change, and something to do with creating a dom that large for parsing and is a side effect of some other core change.  

a solution might be to reject feeds with a file over a certain size, as defined in a pref.  there is also a pref to throttle concurrent feeds being processed (25) and people with big feed files need to lower this (as noted in  the guide).
(In reply to alta88 from comment #11)
> the only possible change in this patch that may remotely cause Bug 1439393
> would be the FeedUtils.jsm hunk in
> https://hg.mozilla.org/comm-central/rev/1654a1adc838
You mean all hunks in FeedUtils.jsm?

Rctgamer3, if I do a try build with the partial backout, would you be able to test this and see whether the crash goes away? If so, which platform do you need? Linux32/64, Mac, Windows 32/64?
Flags: needinfo?(rctgamer3)
(In reply to Jorg K (GMT+1) from comment #12)
> would you be able to test this and see whether the crash goes away? If so, which platform do
> you need? Linux32/64, Mac, Windows 32/64?
Sure. Windows 32 (even though i'm using W10 x64)
Flags: needinfo?(rctgamer3)
Backout patch for FeedUtils.jsm. Try coming for Win 32bit.
(In reply to Jorg K (GMT+1) from comment #15)

Doesn't crash, yay! - everything's working just fine with this build. It's been running for over an hour with no CPU spikes or memory leaks - stable at ~200MB memory.
OK, let's go with this then.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Attachment #8964137 - Flags: approval-comm-beta+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9f58c4a6ec4a
Partially backed out changeset 1654a1adc838 (FeedUtils.jsm) to fix crash (bug 1439393). a=jorgk
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.