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 Data.List
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 Data.GI.Base.GError (catchGErrorJust)

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 @@ -415,7 +416,10 @@ insertNewNoti newNoti tState = do
(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 @@ removeNotiFromDistList tState id = do
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 -> IO () -> IO () -> IO (TVar NotifyState)
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 @@ instance HasDisplayingNotificationContent DisplayingNotificationPopup where


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

let distanceTopFromConfig = configDistanceTop config
Expand Down Expand Up @@ -99,7 +99,7 @@ showNotificationWindow config noti dispNotis onClose = do
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 @@ showNotificationWindow config noti dispNotis onClose = do
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