provided recommended update interval for feeds ("Publisher recommends:") also used for calculation with different time unit

RESOLVED FIXED in Thunderbird 59.0

Status

MailNews Core
Feed Reader
RESOLVED FIXED
9 months ago
2 months ago

People

(Reporter: aryx, Assigned: alta88)

Tracking

unspecified
Thunderbird 59.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.52 KB, patch
alta88
: review+
Details | Diff | Splinter Review
Thunderbird Daily 55.0a1 20170408 + 54.0a2 20170412 on Windows 8.1

The provided recommended update interval for feeds ("Publisher recommends:") also gets used for the calculation with the other time unit.

Steps to reproduce:
1. Subscribe to https://leipzig.craigslist.de/search/aos?format=rss This has
> <syn:updateFrequency>6</syn:updateFrequency>
> <syn:updatePeriod>hourly</syn:updatePeriod>
2. Select the feed if necessary.
3. Change the time unit from minutes to days.
Actual result:
The publisher recommendation changes from 10 to 0.25. The 6 gets divided by 24 now:
https://hg.mozilla.org/releases/comm-aurora/rev/8571f68411c7#l20.350
Expected result:
6 / (24 * 60) ~= 0.004 shown (or the time with unit and no conversion: "6 minutes")
(Assignee)

Comment 1

9 months ago
heh, i sure hope that's the worst bug in Feeds..  aryx, if you can do a patch fixing the calculation, i will review it.
(Assignee)

Comment 2

2 months ago
Created attachment 8930287 [details] [diff] [review]
recUnits.patch

the correct calculation is actually (60/6)/(24*60), or 1/6/24. the value in days is merely to be correct informationally; only int values are accepted and values less than a day must use hours.
Assignee: nobody → alta88
Attachment #8930287 - Flags: review?(aryx.bugmail)
(Assignee)

Comment 3

2 months ago
s/hours/minutes
(Assignee)

Comment 4

2 months ago
Comment on attachment 8930287 [details] [diff] [review]
recUnits.patch

This is a prereq for Bug 1420473 to apply.
Attachment #8930287 - Flags: review?(aryx.bugmail) → review?(mkmelin+mozilla)

Comment 5

2 months ago
Comment on attachment 8930287 [details] [diff] [review]
recUnits.patch

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

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +1193,2 @@
>      if (aUpdates.updatePeriod == null)
> +      return val;

i'd keep the return "" here for easier readability

@@ +1214,5 @@
>          val = 365 * units / frequency;
>          break;
>      }
>  
> +    return val ? Math.round(val * 1000) / 1000 : val;

here too keep the "" at the end. It's easier to understand
Attachment #8930287 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 6

2 months ago
Created attachment 8932231 [details] [diff] [review]
recUnits.patch
Attachment #8930287 - Attachment is obsolete: true
Attachment #8932231 - Flags: review+
(Assignee)

Updated

2 months ago
Keywords: checkin-needed

Comment 7

2 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7e90fcd346bb
Fix rec units for days calculation. r=mkmelin
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
You need to log in before you can comment on or make changes to this bug.