[dev] Make Notifier more general

[drew2a]

Make Notifier more general

There are three points, that we can improve in the current Implementation of Notifier:

1. Notifier doesn't support async/await:

def notify(self, subject, *args):

But it seems wrong because we developing an Application that supports async/await.

2. Notifier prohibits us from introducing new topics without changing NTFY:

    def add_observer(self, subject, callback):
        assert isinstance(subject, NTFY)

What makes the NTFY kinda global event registry.
In the future, this will lead to permanent merge conflicts, because every developer, that uses the Notifier should change NTFY.

It also makes it difficult to find the topics which belong to, say, a particular Community.

3. Notifier.notify method uses *args instead **kwargs which is more vulnerable to making TypeError just by misprint:

    def notify(self, subject, *args):

Example:

notifier = Notifier()
topic = ''

def _f(one, two, three):
    pass

notifier.add_observer('', _f)
notifier.notify(topic, 'one', 'two', 'three')  # ok
notifier.notify(topic, 'one', 'two', 'three')  # ok
notifier.notify(topic, 'one', 'two', '', 'three')  # TypeError: _f() takes 3 positional arguments but 4 were given

def _f1(one, two, three, four):
    pass

notifier.add_observer('', _f1)
notifier.notify(topic, 'one', 'two', 'three')  # TypeError: _f1() missing 1 required positional argument: 'four'

Resuming

It would be nice if Notifier will be closer to the following:

class Notifier(object):
    def __init__(self):
        self.observers = defaultdict(lambda: [])

    def add_observer(self, subject, callback):
        self.observers[subject].append(callback)

    async def notify(self, subject, **kwargs):
        await asyncio.gather(*[callback(**kwargs) for callback in self.observers[subject]])

[devos50]

Our Notifier has been included in Tribler for many, many years already, but is not considered important enough to spend much time on, unfortunately. Also, we barely make any changes to our existing Notifier code. I think @ichorid actually refactored this piece of code some time ago.

You identified a few points for improvement, which is nice 👍 . I have a few comments:

Notifier doesn't support async/await:

A notifier, at least in my eyes, is a one-way channel. Therefore, I do not see the need for asynchronous support. Is there any specific use-case you envision?

Notifier.notify method uses *args instead **kwargs which is more vulnerable to making TypeError just by misprint:

Yes, this is not a very good design but again, it's legacy code. If you can improve it, please go ahead 👍

[ichorid]

Related to #4953

I think @ichorid actually refactored this piece of code some time ago.

I basically deleted the old implementation that was a horrible mess of thread-using code from even the pre-Twisted times and replaced that with a small stub-like thing.

The idea there was to move as much stuff (e.g. configs, definition) into individual components, as possible. In that case, Notifications could become an object class. However, many Notifications are used in both GUI and Core. By our rules, we try to put common definitions into TriblerCommon package. As JSON util does not know how to serialize custom classes, we have to provide our own serialization definitions for this classes (e.g. NTFY) in the form of Enums in the TriblerCommon module.

Also, gathering the responses is a bad idea. Notifications were supposed to be "fire and forget" stuff.

[drew2a]

Yes, agree 🤦 stupid idea.

[ichorid]

Yes, agree facepalm stupid idea.

I wouldn't say so. It depends on what we try to achieve...