Meetings without a meeting links can now be displayed in fullscreen notifications #812
Conversation
…een notification. That PR change that. * Preferences changes: * Added a section "Notification" * Added a new parameter related to full screen notification that you can disable to receives notification for meetings without links (Only for meetings with a link) * In the fullscreen notification, don't show the Meeting Link button if there is no meeting link.
WalkthroughThe pull request introduces a new feature for MeetingBar. Meetings without links can now be optionally shown in fullscreen notifications, controlled by a new user setting. The changes update event handling logic, adjust UI components for notifications, enhance preferences with a new notifications section, and modify test event generation. In addition, localization strings have been added and an entitlement for time-sensitive notifications removed. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant ES as Event Scheduler
participant FN as Fullscreen Notification
participant Pref as Preferences
U->>Pref: Set "fullscreenNotificationMeetingLinkOnly" toggle
U->>ES: Trigger event check
ES->>ES: Evaluate event properties and setting flag
alt Meeting link exists or toggle disabled
ES->>FN: Open notification with join button
else No meeting link and toggle enabled
ES->>FN: Open notification without join button
end
FN->>U: Display notification
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -65,9 +64,9 @@ | |||
| * ------------------------ | |||
| */ | |||
| let actionTimeForFullscreenNotification = Double(Defaults[.fullscreenNotificationTime].rawValue) | |||
| let nonAlldayCandidateForFullscreenNotification = (timeInterval > -15 && timeInterval < actionTimeForFullscreenNotification) | |||
| let nonAllDayCandidateForFullscreenNotification = (timeInterval > -15 && timeInterval < actionTimeForFullscreenNotification) | |||
Check warning
Code scanning / Tailor (reported by Codacy)
Initializer expression should not be enclosed within parentheses Warning
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
MeetingBar/Helpers.swift (1)
257-267: Function signature change looks good, but there's a minor grammar issue in the title when meeting link is missing.The function has been updated to conditionally create a fake event with or without a meeting link, which aligns well with the PR's objective of supporting fullscreen notifications for meetings without links.
There's a small grammar issue in the title when no meeting link is present. Consider pluralizing "link" consistently:
- title: includeMeetingLink ? "Test Event (with Meeting link)" : "Test Event (without Meeting link)", + title: includeMeetingLink ? "Test Event (with Meeting link)" : "Test Event (without Meeting links)",Or alternatively, make both singular:
- title: includeMeetingLink ? "Test Event (with Meeting link)" : "Test Event (without Meeting link)", + title: includeMeetingLink ? "Test Event (with Meeting link)" : "Test Event (without a Meeting link)",MeetingBar/Resources /Localization /fr.lproj/Localizable.strings (1)
72-75: French localization has a typo in "évènement".The localization strings for the new notification settings have been added correctly, but there's a typo in the French translation.
There's a typo in the French translation - "évènement" should be plural since it refers to multiple events with meeting links:
-"shared_fullscreen_notification_meeting_link_only_toggle" = "Uniquement pour les évènement avec des liens de réunion"; +"shared_fullscreen_notification_meeting_link_only_toggle" = "Uniquement pour les évènements avec des liens de réunion";MeetingBar/Views/Changelog/Changelog.swift (1)
181-185: Changelog entry has a minor grammar issue.The changelog entry correctly documents the new feature but has a small grammar issue.
Fix the plural/singular inconsistency in the changelog text:
- Text("🖥️ Meetings without a meeting links can now be displayed in fullscreen notifications.") + Text("🖥️ Meetings without meeting links can now be displayed in fullscreen notifications.")Or if you want to keep the article:
- Text("🖥️ Meetings without a meeting links can now be displayed in fullscreen notifications.") + Text("🖥️ Meetings without a meeting link can now be displayed in fullscreen notifications.")MeetingBar/ActionsOnEventStart.swift (1)
67-67: Variable name correction for clarityImproved variable naming from
nonAlldayCandidateForFullscreenNotificationtononAllDayCandidateForFullscreenNotificationfor better readability.Note: Static analysis suggests removing the parentheses around the initializer expression.
- let nonAllDayCandidateForFullscreenNotification = (timeInterval > -15 && timeInterval < actionTimeForFullscreenNotification) + let nonAllDayCandidateForFullscreenNotification = timeInterval > -15 && timeInterval < actionTimeForFullscreenNotification🧰 Tools
🪛 GitHub Check: Tailor (reported by Codacy)
[warning] 67-67:
Initializer expression should not be enclosed within parenthesesCHANGELOG.md (3)
15-15: Clarify Meeting Description and Fix Grammar
The bullet point currently reads:
* Meetings without a meeting links can now be displayed in fullscreen notifications. A new setting has been added to control that behaviour.
Consider revising it to improve grammatical clarity. For example:
* Meetings without a meeting link can now be displayed in fullscreen notifications. A new setting has been added to control this behaviour.
This change corrects the article and pluralization, making the description clearer.
16-18: Fix Nested List Indentation and Refine Parameter Description
The nested list under "Preferences changes:" is using hard tabs and inconsistent indentation. According to markdown guidelines, it’s preferable to use spaces (typically 2 spaces for nesting). In addition, in the second nested bullet, the text reads:
* Added a new parameter related to full screen notification that you can disable to receives notification for meetings without links (Only for meetings with a link)
Consider revising this line to correct the grammatical error ("receives" should be "receive") and to improve clarity. For example:
* Added a new parameter for full-screen notifications that allows users to disable notifications for meetings without links (only meetings with a link will trigger a notification).
This revision not only fixes the verb but also clarifies the intent of the parameter.🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...on" * Added a new parameter related to full screen notification that you can disable to re...(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~18-~18: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive.
Context: ...en notification that you can disable to receives notification for meetings without links...(TO_NON_BASE)
🪛 markdownlint-cli2 (0.17.2)
17-17: Unordered list indentation
Expected: 2; Actual: 1(MD007, ul-indent)
17-17: Hard tabs
Column: 1(MD010, no-hard-tabs)
18-18: Unordered list indentation
Expected: 2; Actual: 1(MD007, ul-indent)
18-18: Hard tabs
Column: 1(MD010, no-hard-tabs)
19-19: Review Meeting Link Button Description Consistency
The bullet point:
* In the fullscreen notification, don't show the Meeting Link button if there is no meeting link.
is clear in its intent. However, please verify consistency in terminology and styling (e.g., "fullscreen" vs. "full-screen") with the rest of the changelog and project documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.md(1 hunks)MeetingBar/ActionsOnEventStart.swift(3 hunks)MeetingBar/Extensions/DefaultsKeys.swift(1 hunks)MeetingBar/Helpers.swift(1 hunks)MeetingBar/MeetingBar.entitlements(0 hunks)MeetingBar/Resources /Localization /en.lproj/Localizable.strings(1 hunks)MeetingBar/Resources /Localization /fr.lproj/Localizable.strings(1 hunks)MeetingBar/Views/Changelog/Changelog.swift(1 hunks)MeetingBar/Views/FullscreenNotification.swift(2 hunks)MeetingBar/Views/Preferences/GeneralTab.swift(2 hunks)MeetingBar/Views/Shared.swift(2 hunks)
💤 Files with no reviewable changes (1)
- MeetingBar/MeetingBar.entitlements
🧰 Additional context used
🧬 Code Definitions (3)
MeetingBar/Views/Changelog/Changelog.swift (1)
MeetingBar/Helpers.swift (1)
compareVersions(83-85)
MeetingBar/ActionsOnEventStart.swift (2)
MeetingBar/EventStores/Event.swift (1)
getNextEvent(235-313)MeetingBar/AppDelegate.swift (1)
openFullscreenNotificationWindow(310-335)
MeetingBar/Views/FullscreenNotification.swift (1)
MeetingBar/Helpers.swift (1)
generateFakeEvent(257-276)
🪛 GitHub Actions: Test
MeetingBar/Extensions/DefaultsKeys.swift
[error] 1-1: SwiftCompile normal arm64 Compiling DefaultsKeys.swift (in target 'MeetingBar' from project 'MeetingBar')
MeetingBar/Views/Changelog/Changelog.swift
[error] 1-1: SwiftCompile normal arm64 Compiling Changelog.swift (in target 'MeetingBar' from project 'MeetingBar')
MeetingBar/Views/Shared.swift
[error] 1-1: SwiftCompile normal arm64 Compiling Shared.swift (in target 'MeetingBar' from project 'MeetingBar')
MeetingBar/Views/FullscreenNotification.swift
[error] 1-1: SwiftCompile normal arm64 Compiling FullscreenNotification.swift (in target 'MeetingBar' from project 'MeetingBar')
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~18-~18: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...on" * Added a new parameter related to full screen notification that you can disable to re...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~18-~18: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive.
Context: ...en notification that you can disable to receives notification for meetings without links...
(TO_NON_BASE)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
17-17: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
17-17: Hard tabs
Column: 1
(MD010, no-hard-tabs)
18-18: Unordered list indentation
Expected: 2; Actual: 1
(MD007, ul-indent)
18-18: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🪛 GitHub Check: Tailor (reported by Codacy)
MeetingBar/ActionsOnEventStart.swift
[warning] 67-67:
Initializer expression should not be enclosed within parentheses
🪛 GitHub Check: Tests
MeetingBar/Views/FullscreenNotification.swift
[warning] 89-89:
result of 'FullscreenNotification' initializer is unused
[warning] 88-88:
result of 'FullscreenNotification' initializer is unused
[warning] 89-89:
result of 'FullscreenNotification' initializer is unused
[warning] 88-88:
result of 'FullscreenNotification' initializer is unused
🔇 Additional comments (13)
MeetingBar/Resources /Localization /en.lproj/Localizable.strings (2)
284-284: Clear and well-placed section titleThe new "Notifications" section title follows the established naming convention for preference sections and is appropriately placed in the localization file. This aligns with the PR's goal of improving notification management.
287-287: Toggle description clearly communicates its purposeThe new toggle string "Only for meetings with a link" is clear and concise, effectively communicating to users that they can filter notifications to only show for meetings with links. This properly supports the main feature of this PR - allowing meetings without links to be optionally displayed in fullscreen notifications.
MeetingBar/Extensions/DefaultsKeys.swift (1)
31-33: New preference key for meeting-link-only notifications looks good.The addition of the
fullscreenNotificationMeetingLinkOnlypreference key with a default value offalseis appropriate. This ensures that by default, fullscreen notifications will be shown for all meetings, maintaining backward compatibility while providing the new option to filter notifications.MeetingBar/Views/FullscreenNotification.swift (3)
21-24: Good implementation of conditional rendering for meeting iconsThe conditional check for
event.meetingLink != nilensures the meeting service icon only appears when a link is available, which aligns with the PR objective of better handling meetings without links.
43-47: Proper conditional handling of the join buttonThe join button is now correctly only displayed when a meeting link exists, preventing users from interacting with a button that would have no effect for meetings without links.
88-89:✅ Verification successful
Preview cases now cover both scenarios
Good addition of test cases for the preview, showing both meetings with and without links. This ensures proper testing of the UI in both scenarios.
However, note that the static analysis is flagging unused initializer results. This is a false positive for SwiftUI previews.
🏁 Script executed:
#!/bin/bash # Check if this is a standard SwiftUI preview pattern rg -A 3 -B 3 "#Preview" --glob "*.swift"Length of output: 621
Static Analysis False Positive Confirmed as Standard SwiftUI Preview
Great work expanding the preview cases to cover both scenarios (with and without meeting links). The static analysis warning about unused initializer results is a known false positive in SwiftUI previews, as confirmed by our inspection of the "#Preview" usage in the file. No changes are required here.
🧰 Tools
🪛 GitHub Check: Tests
[warning] 89-89:
result of 'FullscreenNotification' initializer is unused
[warning] 88-88:
result of 'FullscreenNotification' initializer is unused
[warning] 89-89:
result of 'FullscreenNotification' initializer is unused
[warning] 88-88:
result of 'FullscreenNotification' initializer is unusedMeetingBar/Views/Preferences/GeneralTab.swift (2)
32-40: Well-structured new NotificationsSection componentThe new NotificationsSection organizes notification preferences into a dedicated section with appropriate styling and layout, improving the UI organization.
46-63: Improved layout for ShortcutsSectionThe refactored ShortcutsSection now uses a more consistent layout pattern with the new NotificationsSection, with proper text styling and padding.
MeetingBar/Views/Shared.swift (3)
38-41: Good addition of new preference toggleThe new
fullscreenNotificationMeetingLinkOnlyproperty supports the feature to control whether meetings without links should be displayed in notifications.
44-57: Improved layout organization for notification preferencesThe refactored layout from HStack to VStack provides better visual organization and hierarchy, with the meeting link toggle appropriately nested and conditionally disabled.
83-91: Better organization of notification tipsWrapping the notification tips in a Section with appropriate padding improves readability and visual hierarchy.
MeetingBar/ActionsOnEventStart.swift (2)
53-55: Core implementation of feature logicThe code correctly retrieves the user preference and passes it to the
getNextEventfunction, enabling the filtering of meetings without links based on user settings.
78-78: Simplified fullscreen notification logicThe code now correctly opens the fullscreen notification window without requiring a meeting link, as per the feature requirement.
dosubot
bot
added
size:L
…ithout_meeting_links
Status
READY
Description
By default, meetings without a link were not displayed in the fullscreen notification. That PR change that.
Checklist
Steps to Test or Reproduce
Outline the steps to test or reproduce the PR here.
Summary by CodeRabbit
New Features
Bug Fixes