Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix streak notifications scheduling for iOS 10.0* #430

Merged
merged 2 commits into from
Dec 12, 2018

Conversation

ivan-magda
Copy link
Member

@ivan-magda ivan-magda commented Dec 11, 2018

Задача: #APPS-2166

Описание:
Исправили планирование streak нотификаций. Оригинальный код до внедрения LocalNotificationsService и использования UserNotifications.
При создании компонентов даты для UNCalendarNotificationTrigger многие поля были избыточны и поэтому нотификация возникала в лучшем случае только 1 раз. На iOS 9.0 такое поведение не наблюдается из-за использования специального поля для указания частоты repeatInterval: NSCalendar.Unit?.

Как проверить:

  • включаем стрики
  • выбираем час
  • переходим в настройки телефона -> дата/время
  • изменяем время/дату

Verified

This commit was signed with the committer’s verified signature.
renovate-bot Mend Renovate
@ivan-magda ivan-magda self-assigned this Dec 11, 2018
@ivan-magda ivan-magda requested review from kvld and Ostrenkiy December 11, 2018 16:48
private let calendar: Calendar

private var dateComponents: DateComponents {
let timeZoneDiff = NSTimeZone.system.secondsFromGMT() / 3600
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут кстати вроде как есть bridge в TimeZone

let timeZoneDiff = NSTimeZone.system.secondsFromGMT() / 3600
var localStartHour = self.UTCStartHour + timeZoneDiff

if localStartHour < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не по теме ПРа, но каждый раз смотрю на этот кусок кода и не понимаю, почему вообще в пикере происходит какое-то шаманство с датой. Ведь можно же оттуда отдавать DateComponents с выбранным часом сразу

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Я не разбирался, возможно @Ostrenkiy помнит, почему пришлось шаманить.
Кстати, в момент выбора даты уже выполняется перевод в UTC https://github.com/StepicOrg/stepik-ios/blob/dev/Stepic/NotificationTimePickerViewController.swift#L37
а потом еще раз точно такой же код выполняется тут - в StreakLocalNotificationContentProvider

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Кажется, там шаманство из-за того, что в пикере дата должна быть в локальном часовом поясе, а сохраняем ее мы в UTC формате.
А почему это шаманство именно такое я без понятия, если можно сделать такие вещи по-другому и хорошо - это круто и надо бы сделать

@ivan-magda ivan-magda merged commit ac39974 into dev Dec 12, 2018
@ivan-magda ivan-magda deleted the fix/streak-notifications-scheduling branch December 12, 2018 15:44
@kvld kvld added this to the 1.74 milestone Dec 13, 2018
@kvld kvld mentioned this pull request Dec 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants