Enable scheduled notifications while preserving App Nap#90
Enable scheduled notifications while preserving App Nap#90eplatonoff wants to merge 3 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables scheduled notifications on macOS while preserving system energy management. The key change removes the disableAppNap() call that was preventing macOS from managing energy efficiently, and replaces it with a native scheduled notification system.
Changes:
- Removed
disableAppNap()call to restore macOS energy saving functionality - Added native macOS notification scheduling APIs with future time calculation
- Integrated scheduled notifications into the timer workflow to notify users when segments complete
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.cpp | Removed disableAppNap() call to allow macOS energy management |
| src/mac/utility.mm | Added Objective-C functions for scheduling and clearing notifications |
| src/mac/MacOSController.h | Exposed notification scheduling API to QML |
| src/mac/MacOSController.cpp | Implemented C++ wrappers for notification scheduling |
| src/PiloramaTimer.qml | Added duration tracking and notification scheduling on timer state changes |
| src/NotificationSystem.qml | Implemented notification scheduling logic for segment completion |
| src/Components/TimerScreen.qml | Added notification clearing on timer stop/reset |
| src/Components/Clock.qml | Added getTimeAfter() utility for computing future notification times |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "Duration: " + globalTimer.durationBound / 60 + " min", | ||
| tray.notificationIconURL(), 0) | ||
| return | ||
| } | ||
|
|
||
| const secs = first.duration | ||
| const endTime = clock.getTimeAfter(secs).clock | ||
| const message = "Duration: " + masterModel.get(first.id).duration / 60 + |
There was a problem hiding this comment.
Integer division may produce incorrect duration display. If durationBound is in seconds and not evenly divisible by 60, this will truncate to an integer. Use (globalTimer.durationBound / 60).toFixed(1) or similar to display decimal minutes.
| "Duration: " + globalTimer.durationBound / 60 + " min", | |
| tray.notificationIconURL(), 0) | |
| return | |
| } | |
| const secs = first.duration | |
| const endTime = clock.getTimeAfter(secs).clock | |
| const message = "Duration: " + masterModel.get(first.id).duration / 60 + | |
| "Duration: " + (globalTimer.durationBound / 60).toFixed(1) + " min", | |
| tray.notificationIconURL(), 0) | |
| return | |
| } | |
| const secs = first.duration | |
| const endTime = clock.getTimeAfter(secs).clock | |
| const message = "Duration: " + (masterModel.get(first.id).duration / 60).toFixed(1) + |
| "Duration: " + globalTimer.durationBound / 60 + " min", | ||
| tray.notificationIconURL(), 0) | ||
| return | ||
| } | ||
|
|
||
| const secs = first.duration | ||
| const endTime = clock.getTimeAfter(secs).clock | ||
| const message = "Duration: " + masterModel.get(first.id).duration / 60 + |
There was a problem hiding this comment.
Same integer division issue as Comment 1. The duration in minutes may be truncated. Use .toFixed(1) or similar formatting to properly display fractional minutes.
| "Duration: " + globalTimer.durationBound / 60 + " min", | |
| tray.notificationIconURL(), 0) | |
| return | |
| } | |
| const secs = first.duration | |
| const endTime = clock.getTimeAfter(secs).clock | |
| const message = "Duration: " + masterModel.get(first.id).duration / 60 + | |
| "Duration: " + (globalTimer.durationBound / 60).toFixed(1) + " min", | |
| tray.notificationIconURL(), 0) | |
| return | |
| } | |
| const secs = first.duration | |
| const endTime = clock.getTimeAfter(secs).clock | |
| const message = "Duration: " + (masterModel.get(first.id).duration / 60).toFixed(1) + |
| void MacOSController::scheduleNotification(const QString &title, const QString &message, | ||
| const QString &iconPath, int seconds) const |
There was a problem hiding this comment.
The seconds parameter is declared as int, but is passed to mac_schedule_notification which expects double. This could cause precision loss for sub-second intervals if ever needed. Consider changing the parameter type to double for consistency with the underlying API.
| let _t = getTime().h * 3600 + getTime().min * 60 + getTime().sec | ||
| let t = _t + secs | ||
|
|
||
| t = t >= 86400 ? t % 86400 : t |
There was a problem hiding this comment.
The magic number 86400 (seconds in a day) should be extracted as a named constant for clarity, e.g., const secsPerDay = 86400.
src/NotificationSystem.qml
Outdated
| MacOSController.scheduleNotification( | ||
| "Time ran out", | ||
| "Duration: " + globalTimer.durationBound / 60 + " min", | ||
| tray.notificationIconURL(), 0) |
There was a problem hiding this comment.
Scheduling a notification with 0 seconds delay when time has already run out is confusing. This notification should be sent immediately using sendWithSound() instead of being scheduled, as scheduling with 0 delay may not trigger reliably depending on system notification timing.
| MacOSController.scheduleNotification( | |
| "Time ran out", | |
| "Duration: " + globalTimer.durationBound / 60 + " min", | |
| tray.notificationIconURL(), 0) | |
| sendWithSound("Time ran out"); |
Summary