feat: reposition popups upon dismissal#240
Draft
d3adb5 wants to merge 1 commit into
Draft
Conversation
Reposition other popups when popups are dismissed / closed. Fixes phuhl#237.
phuhl
requested changes
Aug 5, 2023
| (screenWidth, screenHeight, _) <- getScreenPos (_dMainWindow newpopup) monitorId | ||
| let x = screenWidth - (notificationWidth + distanceRight) | ||
| y <- calculateY preceding distanceBetween distanceTop | ||
| windowMove (_dMainWindow newpopup) x y |
Owner
There was a problem hiding this comment.
I think, this has to be wrapped in an addSource call to make it thread safe
| -- | Adjusts the position of all displayed notifications so they follow standardized placement rules. | ||
| readjustNotificationPositions :: Config -> TVar NotifyState -> IO () | ||
| readjustNotificationPositions config tState = do | ||
| sortedDisplayedPopups <- sortOn _dNotiTop . filter (not . _dHasCustomPosition) . notiDisplayingList <$> readTVarIO tState |
Owner
There was a problem hiding this comment.
The readTVarIO state reads a shared memory location. I think for performance reasons (and readability) it makes sense to get the state once, before
Suggested change
| sortedDisplayedPopups <- sortOn _dNotiTop . filter (not . _dHasCustomPosition) . notiDisplayingList <$> readTVarIO tState | |
| state <- readTVarIO tState | |
| sortedDisplayedPopups <- sortOn _dNotiTop . filter (not . _dHasCustomPosition) . notiDisplayingList state |
| readjustNotificationPositions config tState = do | ||
| sortedDisplayedPopups <- sortOn _dNotiTop . filter (not . _dHasCustomPosition) . notiDisplayingList <$> readTVarIO tState | ||
| newDisplayedPopups <- pushNotificationsUp config sortedDisplayedPopups | ||
| atomically $ modifyTVar' tState $ \state -> |
Owner
There was a problem hiding this comment.
I could be wrong, but this could loose notifications. Consider this scenario:
- You read display notis out in line 451
- then another process might kick in and adds a notification
- You overwrite displayNoti List w/o new noti (as it was not present before)
It probably has all to be atomic
| atomically $ modifyTVar' tState $ \state -> | ||
| state { notiDisplayingList = newDisplayedPopups ++ filter _dHasCustomPosition (notiDisplayingList state) } | ||
|
|
||
| pushNotificationsUp :: Config -> [DisplayingNotificationPopup] -> IO [DisplayingNotificationPopup] |
Owner
There was a problem hiding this comment.
Suggested change
| pushNotificationsUp :: Config -> [DisplayingNotificationPopup] -> IO [DisplayingNotificationPopup] | |
| pushFirstNotificationUp :: Config -> [DisplayingNotificationPopup] -> IO [DisplayingNotificationPopup] |
| newFirst <- autoplaceNotification config Nothing f | ||
| pushNotificationsUp' config (newFirst:r) | ||
|
|
||
| pushNotificationsUp' :: Config -> [DisplayingNotificationPopup] -> IO [DisplayingNotificationPopup] |
Owner
There was a problem hiding this comment.
Suggested change
| pushNotificationsUp' :: Config -> [DisplayingNotificationPopup] -> IO [DisplayingNotificationPopup] | |
| pushNextNotificationUp :: Config -> [DisplayingNotificationPopup] -> IO [DisplayingNotificationPopup] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reposition other popups when popups are dismissed / closed.
Fixes #237.