Closed Bug 1355965 Opened 4 years ago Closed 4 years ago

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

Categories

(MailNews Core :: Feed Reader, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 59.0

People

(Reporter: aryx, Assigned: alta88)

References

Details

Attachments

(1 file, 1 obsolete file)

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")
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.
Attached patch recUnits.patch (obsolete) — Splinter Review
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)
s/hours/minutes
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 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+
Attached patch recUnits.patchSplinter Review
Attachment #8930287 - Attachment is obsolete: true
Attachment #8932231 - Flags: review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7e90fcd346bb
Fix rec units for days calculation. r=mkmelin
Status: NEW → RESOLVED
Closed: 4 years 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.