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

feat: reposition popups upon dismissal #240

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 48 additions & 5 deletions src/NotificationCenter/Notifications.hs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE OverloadedStrings, TupleSections #-}

module NotificationCenter.Notifications
( startNotificationDaemon
Expand Down Expand Up @@ -46,7 +46,7 @@
import qualified Data.Map as Map
import Data.Time
import Data.Time.LocalTime
import Data.Maybe (fromMaybe)
import Data.Maybe (fromMaybe, fromJust, isNothing)

import qualified Data.Yaml as Yaml
import qualified Data.Aeson as Aeson
Expand All @@ -62,6 +62,7 @@

import GI.Gio.Interfaces.AppInfo (appInfoGetIcon, appInfoGetAll, appInfoGetName)
import GI.Gio.Interfaces.Icon (iconToString, Icon(..))
import GI.Gtk (windowMove, windowGetPosition)

data NotifyState = NotifyState
{ notiStList :: [ Notification ]
Expand Down Expand Up @@ -245,7 +246,7 @@
state <- readTVarIO tState
time <- getTime
icon <- parseIcon config hints icon appName
let newNotiWithoutId = Notification

Check warning on line 249 in src/NotificationCenter/Notifications.hs

View workflow job for this annotation

GitHub Actions / Build

• Fields of ‘Notification’ not initialised:
{ notiAppName = appName
, notiRepId = replaceId

Expand Down Expand Up @@ -415,7 +416,10 @@
(notiConfig state)
newNoti
(notiDisplayingList state)
(removeNotiFromDistList tState $ notiId newNoti)
(\cfg -> do
removeNotiFromDistList tState $ notiId newNoti
readjustNotificationPositions cfg tState
)
atomically $ modifyTVar' tState $ \state ->
state { notiDisplayingList = dnoti : notiDisplayingList state }
return ()
Expand All @@ -441,18 +445,57 @@
return False
return ()

-- | 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
Copy link
Owner

Choose a reason for hiding this comment

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

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

newDisplayedPopups <- pushNotificationsUp config sortedDisplayedPopups
atomically $ modifyTVar' tState $ \state ->
Copy link
Owner

Choose a reason for hiding this comment

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

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

state { notiDisplayingList = newDisplayedPopups ++ filter _dHasCustomPosition (notiDisplayingList state) }

pushNotificationsUp :: Config -> [DisplayingNotificationPopup] -> IO [DisplayingNotificationPopup]
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
pushNotificationsUp :: Config -> [DisplayingNotificationPopup] -> IO [DisplayingNotificationPopup]
pushFirstNotificationUp :: Config -> [DisplayingNotificationPopup] -> IO [DisplayingNotificationPopup]

pushNotificationsUp _ [ ] = return [ ]
pushNotificationsUp config (f:r) = do
newFirst <- autoplaceNotification config Nothing f
pushNotificationsUp' config (newFirst:r)

pushNotificationsUp' :: Config -> [DisplayingNotificationPopup] -> IO [DisplayingNotificationPopup]
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
pushNotificationsUp' :: Config -> [DisplayingNotificationPopup] -> IO [DisplayingNotificationPopup]
pushNextNotificationUp :: Config -> [DisplayingNotificationPopup] -> IO [DisplayingNotificationPopup]

pushNotificationsUp' _ [ ] = return [ ]
pushNotificationsUp' _ [l] = return [l]
pushNotificationsUp' config (f:s:r) = do
newSecond <- autoplaceNotification config (Just f) s
newRest <- pushNotificationsUp' config (newSecond:r)
return (f:newRest)

-- | Places a notification popup on the screen automatically given a preceding notification.
autoplaceNotification :: Config -> Maybe DisplayingNotificationPopup -> DisplayingNotificationPopup -> IO DisplayingNotificationPopup
autoplaceNotification config preceding newpopup = do
let monitorId = fromIntegral $ configNotiMonitor config
notificationWidth = fromIntegral $ configWidthNoti config
distanceRight = fromIntegral $ configDistanceRight config
distanceTop = fromIntegral $ configDistanceTop config
distanceBetween = fromIntegral $ configDistanceBetween config
(screenWidth, screenHeight, _) <- getScreenPos (_dMainWindow newpopup) monitorId
let x = screenWidth - (notificationWidth + distanceRight)
y <- calculateY preceding distanceBetween distanceTop
windowMove (_dMainWindow newpopup) x y
Copy link
Owner

Choose a reason for hiding this comment

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

I think, this has to be wrapped in an addSource call to make it thread safe

return newpopup { _dNotiTop = y }
where calculateY Nothing _ d = return d
calculateY (Just dn) db _ = (db + _dNotiTop dn +) <$> _dNotiGetHeight dn

hideAllNotis tState = do
state <- readTVarIO tState
mapM (removeNotiFromDistList tState . _dNotiId)
$ notiDisplayingList state
return ()

closeNotification tState id = do
closeNotification config tState id = do
state <- readTVarIO tState
let notis = filter (\n -> (notiId n) /= fromIntegral id) (notiStList state)
sequence $ (\noti -> addSource $ do notiOnClosed noti $ CloseByCall
return False) <$> notis
removeNotiFromDistList' tState id
readjustNotificationPositions config tState


notificationDaemon :: (AutoMethod f1, AutoMethod f2) =>
Expand All @@ -476,5 +519,5 @@
startNotificationDaemon config onUpdate onUpdateForMe = do
istate <- newTVarIO $ NotifyState [] [] 1 onUpdate onUpdateForMe config []
forkIO (notificationDaemon config (notify config istate)
(closeNotification istate))
(closeNotification config istate))
return istate
14 changes: 7 additions & 7 deletions src/NotificationCenter/Notifications/NotificationPopup.hs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@


showNotificationWindow :: Config -> Notification
-> [DisplayingNotificationPopup] -> (IO ()) -> IO DisplayingNotificationPopup
-> [DisplayingNotificationPopup] -> (Config -> IO ()) -> IO DisplayingNotificationPopup
showNotificationWindow config noti dispNotis onClose = do

let distanceTopFromConfig = configDistanceTop config
Expand All @@ -80,13 +80,13 @@
labelBG <- label objs "label_background"

dispNotiWithoutHeight <- createNotification config builder noti
$ DisplayingNotificationPopup

Check warning on line 83 in src/NotificationCenter/Notifications/NotificationPopup.hs

View workflow job for this annotation

GitHub Actions / Build

• Fields of ‘DisplayingNotificationPopup’ not initialised:
{ _dMainWindow = mainWindow
, _dLabelBG = labelBG
, _dNotiId = notiId noti
, _dNotiDestroy = widgetDestroy mainWindow
, _dHasCustomPosition = hasCustomPosition
, _dpopupContent = DisplayingNotificationContent {} }

Check warning on line 89 in src/NotificationCenter/Notifications/NotificationPopup.hs

View workflow job for this annotation

GitHub Actions / Build

• Fields of ‘DisplayingNotificationContent’ not initialised:

let dispNoti = set dNotiGetHeight
(getHeight (view dContainer dispNotiWithoutHeight) config)
Expand All @@ -99,7 +99,7 @@
setUrgencyLevel (notiUrgency noti)
$ (flip view) dispNoti <$> [dLabelTitel, dLabelBody, dLabelAppname]

height <- updateNoti' config onClose noti dispNoti
height <- updateNoti' config (onClose config) noti dispNoti

-- Ellipsization of Body
numLines <- fromIntegral <$> (layoutGetLineCount =<< labelGetLayout lblBody)
Expand Down Expand Up @@ -154,23 +154,23 @@
defaultAction = configPopupDefaultActionButton config == mouseButton
if | valid && dismiss -> do
notiOnClosed noti $ User
onClose
onClose config
| valid && defaultAction -> do
notiOnAction noti "default" Nothing
notiOnClosed noti $ User
onClose
onClose config
| not validDismiss -> do
putStrLn $ "Warning: Unknown mouse button '" ++ (show $ configPopupDismissButton config) ++ "'."
notiOnClosed noti $ User
onClose
onClose config
| not validDefaultAction -> do
putStrLn $ "Warning: Unknown mouse button '" ++ (show $ configPopupDefaultActionButton config) ++ "'."
notiOnClosed noti $ User
onClose
onClose config
| otherwise -> do
putStrLn $ "Warning: Popup received unknown mouse input '" ++ (show mouseButton) ++ "'."
notiOnClosed noti $ User
onClose
onClose config
return False

widgetShow mainWindow
Expand Down
Loading