Implementation details discussion

[d3adb5]

Hope you don't mind me opening a semi-permanent thread to discuss implementation details, @phuhl. This kind of thing might usually be done over IRC, especially given the ephemeral nature of some of the topics that ensue, so please point me to something like it if it already exists.

I'll kickstart it! I'm working on minor style changes and refactoring, and happened upon the following snippet in src/NotificationCenter.hs:

updateNotis :: Config -> TVar State -> IO()
updateNotis config tState = do
  state <- readTVarIO tState
  notiState <- readTVarIO $ stNotiState state

  let newNotis = filter (
        \n -> (find (\nd -> _dNotiId nd == notiId n )
               (stDisplayingNotiList state))
              == Nothing
              && ((configIgnoreTransient config) || (not $ notiTransient n)))
                 $ notiStList notiState
  -- ...

I feel like there are plenty of long functions and actions in the project's modules, so I'm looking into creating some pure functions with names that help better understand the intent behind most of the code. My idea was turning the above filter predicate into something like:

isNewNotification :: Config -> State -> NotifyState -> Notification -> Bool
isNewNotification config state nstate notification
  =  (configIgnoreTransient config || not (notiTransient notification))
  && isNothing (find ((== notiId notification) . _dNotiId) (stDisplayingNotiList state))

And then have the filter application go something like:

-- ...
let newNotifications = filter (isNewNotification config state notiState) $ notiStList notiState
-- ...

I am, however, a little confused about configIgnoreTransient config || not (notiTransient notification). If I'm reading it correctly, we treat any notification --- transient or not --- as a potential new notification if we've configured Deadd to ignore transient notifications / windows, and otherwise only transient notifications / windows as potential new notifications...?

Am I even reading the intent correctly there, and this filter aims to compose a list of notifications that have not yet been displayed in the notification center?

I'm sure I'll have similar prompts for clarification in the future as I try to make sense of and modify the existing codebase, which is why I created this discussion thread.

[phuhl]

Hope you don't mind me opening a semi-permanent thread to discuss implementation details, @phuhl. This kind of thing might usually be done over IRC, especially given the ephemeral nature of some of the topics that ensue, so please point me to something like it if it already exists.

Hey, not at all. I am not experiencend with using IRC and I just looked at libera.chat to open a channel but it seems, I'd have to register a project first and read tons of stuff and yadayada.

Instead I just created a matrix room for now: #deadd-notification-center:beeper.com. Feel free to join, I guess, the communication over there would be a bit more immediate.

Ok, back to the topic now.

I feel like there are plenty of long functions and actions in the project's modules, so I'm looking into creating some pure functions with names that help better understand the intent behind most of the code.

Good idea. Maybe, for functions that are only used in one place it makes sense to declare them locally, using where but I'll leave that to you.

I am, however, a little confused about configIgnoreTransient config || not (notiTransient notification). If I'm reading it correctly, we treat any notification --- transient or not --- as a potential new notification if we've configured Deadd to ignore transient notifications / windows, and otherwise only transient notifications / windows as potential new notifications...?

So, the filter searches for notifications that are not in the DisplayingNOtificationInCenter list so they can be added. The notification protocol has a field that defines if a notification should be transient or not

When set the server will treat the notification as transient and by-pass the server's persistence capability, if it should exist.
--- https://specifications.freedesktop.org/notification-spec/notification-spec-latest.html

But there is a setting to ignore this. Hence, yes, your analysis is correct. Either, the user chooses to ignore the transient field (in that case the notification will be displayend in center) or the transient flag is inspected. If it is set, the notification will not be shown in center.

Am I even reading the intent correctly there, and this filter aims to compose a list of notifications that have not yet been displayed in the notification center?

Yes. Notis, that have not been shown in center but should be, taking transient into account.

Feel free to shoot more questions. If you have matrix, join the channel and we can have a chat.

Quick note on PRs:
For my reviewing pleasure, please keep PRs small and focused on one issue/feature. You did that, just for the future so you know :)