Refactor and simplify the feed Subscribe dialog options updates

RESOLVED FIXED in Thunderbird 59.0

Status

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: alta88, Assigned: alta88)

Tracking

unspecified
Thunderbird 59.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird60 fixed, thunderbird61 fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Assignee

Description

2 years ago
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.
Assignee

Comment 1

2 years ago
Posted patch subscribeTweaks.patch (obsolete) — Splinter Review
Assignee: nobody → alta88
Attachment #8931741 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 2

2 years ago
Posted 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+
Assignee

Comment 4

2 years ago
Posted 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+
Assignee

Comment 5

2 years ago
unbitrot.
Attachment #8931743 - Attachment is obsolete: true
Attachment #8932936 - Flags: review+
Assignee

Updated

2 years ago
Keywords: checkin-needed
Assignee

Comment 6

2 years ago
Posted patch subscribeTweaks.patch (obsolete) — Splinter Review
entity bump.
Attachment #8932935 - Attachment is obsolete: true
Attachment #8932945 - Flags: review+
Assignee

Comment 7

2 years ago
and in the xul.
Attachment #8932945 - Attachment is obsolete: true
Attachment #8932949 - Flags: review+

Comment 8

2 years ago
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: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

2 years ago
Target Milestone: --- → Thunderbird 59.0
Assignee

Comment 9

2 years ago
Posted patch hide.patchSplinter Review
follow up, buttons hidden initially to prevent them showing briefly on load.
Attachment #8933457 - Flags: review+
Assignee

Updated

2 years ago
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---

Comment 10

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f44c101732e9
Follow-up: hide buttons initially. r=me
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Depends on: 1439393
Assignee

Comment 11

Last year
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)

Comment 13

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

Comment 16

Last year
(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 → ---

Updated

Last year
Attachment #8964137 - Flags: approval-comm-beta+

Comment 18

Last year
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: 2 years agoLast year
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.