Basic notifications - Githubissues

ginazhouhuiwu commented 2 weeks ago

Can add and clear notifications all using the mantine notifications system! The first notification in the example below disappears because it is set to auto-close after some time.

https://github.com/nerfstudio-project/viser/assets/42229107/9a080da8-1751-4392-9bd5-fcf794d54fd4

(Currently I'm a bit unsure on the design in accordance with the rest of the gui api.)

TODO:

[ ] documentation
[ ] match setter/getter-style properties of other handles
[ ] fix notifs reopening bug caused by persistent messages

ginazhouhuiwu commented 2 weeks ago

I think add_notification being in the gui api seems reasonable, but the clear_notification part kind of breaks the harmony of the rest of the api. There is probably a cleaner way to organize it, especially if there are future notification features.

ginazhouhuiwu commented 2 weeks ago

Should be good to go now!

chungmin99 commented 2 weeks ago

For clear_notifications, can we remove them on a per-message basis, Instead of having a global clear? (https://v4.mantine.dev/others/notifications/#functions)

E.g., we can track the message ID and do the following:

case "AddNotificationMessage": {
  notifications.show({
    id: message.id,
    ...
    });
  return;
}

// Clear all notifications.
case "ClearNotificationMessage": {
  notifications.hide(message.id);
  return;
}

... but this would require us to send messages from the GuiNotificationHandle. To this point, I was wondering if we could do:

@dataclasses.dataclass
class GuiNotificationHandle:
    """Handle for a notification in our visualizer."""

    id: str
    ...
    _send_msg_fn: Callable[[Message], None]

    def open(self) -> None:
        self._send_msg_fn(...

and set _send_msg_fn to self._websock_interface.queue_message (mostly to avoid setting gui_api as one of the handle properties).

(We could also use the message.id to update the message later on, etc)

chungmin99 commented 2 weeks ago

Also curious about people's thoughts, but I feel like the mantine notifications control (autoClose seconds, loading, ) are quite low-level.

Mainly, I can only think of 3 notif types:

disappear both automatically (after 5 seconds) and manually (via click)
can be closed only manually
can be closed only programmatically, via some handle.close()

(and showing loading only makes sense for (3), assuming some "task" is blocking the notification from closing)

ginazhouhuiwu commented 2 weeks ago

Also curious about people's thoughts, but I feel like the mantine notifications control (autoClose seconds, loading, ) are quite low-level.

I was also thinking about this but was kind of unsure about how to separate them, so thank you for your breakdown of the notif types! I think the abstraction makes a lot of sense and is easy to change.

(changing this pr to draft for now and also realizing that I didn't add docs yet)

ginazhouhuiwu commented 2 weeks ago

Added some different types of notifications as per suggested! Can be indicated via type when declaring a notif.

"persistent" (default): user close
"timed": auto close
"controlled": code close

loading is just an additional optional argument.

I also added an individual notification clear option along with the global clear (can be renamed 'close' for clarity if needed). It has a bit of funky behavior if you open the same notification too many times and just clears all of it, but I actually don't think it's a huge issue irl use cases for now.

Still didn't add docs yet but can easily do so in the end after all design is finalized!

https://github.com/nerfstudio-project/viser/assets/42229107/5196024c-eb8a-467a-8669-85e9d46fb649

ginazhouhuiwu commented 1 week ago

I noticed a problem with multiple clients: unless all the notifications are explicitly cleared in the backend (gui.clear_all_notification or notif.clear()), when another client connects or when you reload the window, all the notifications will be reopened. This happens to both timed notifications that are auto-closed as well as the manually closed notifications. The notifications that are code-closed as mentioned earlier, will not reopen. So it looks like we need to explicitly close all the notifications in the backend regardless.

I tried attaching a callback for notifications.show(...onClose: () => notifications.hide(message.id)), but it didn't do anything. Another idea I had is clear the notifications every time a client disconnects, which also didn't solve the problem and is a bit of a poor patchy solution imo... Would appreciate any other ideas!

brentyi commented 1 week ago

when another client connects or when you reload the window, all the notifications will be reopened. This happens to both timed notifications that are auto-closed as well as the manually closed notifications.

This seems like the result of some viser state management logic, particularly the concept of "persistent" messages, see:

https://github.com/nerfstudio-project/viser/blob/d56be1ad1602739594cb30307cd61599a696d588/src/viser/infra/_infra.py#L244-L246

as well as references to the redundancy_key() method: https://github.com/search?q=repo%3Anerfstudio-project%2Fviser%20redundancy_key&type=code

This is usually nice, since when we make scene or GUI updates we want them to take effect even for new clients, but it does complicate the notification logic. It shouldn't be too hard to fix, but we'd need to figure out what the expected behavior is:

if we broadcast a notification, then a new client connects, should the new client receive the notification at all?
if we broadcast a notification that autocloses in 5 seconds, then a new client connects 1 second later, is the notification shown for 4 seconds or 5 seconds?
if a notification is closed from one client, does it get closed in the others as well?
etc

jkulhanek commented 1 week ago

Actually, would it be possible to disable the automatic synchronization between clients? Make it an opt-in feature (not default)? What do you think?

brentyi commented 1 week ago

Actually, would it be possible to disable the automatic synchronization between clients?

Do you mean for everything, including both GUI elements and scene elements?

Disabling synchronization seems hard without some API changes. Notably, attributes like .value currently make sense to read when we know that they should be consistent across all clients:

text_handle = server.gui.add_text("Some text input")

while True:
    print(text_handle.value)  # Makes sense because all clients have the same value for this input.
    time.sleep(1.0)

But things become more complicated if different clients can have different values for the text input. To support this use case we do support creating separate handles for each client though, eg via client.gui.add_text().

ginazhouhuiwu commented 1 week ago

if we broadcast a notification, then a new client connects, should the new client receive the notification at all?

I imagine in some cases, notifications would be client-specific (e.g. rendering a video, downloading things, etc), so a new client should not receive the same set of notifications. But I can also think of some cases where everybody should receive all the notifs (e.g. training complete). Mostly speaking from a nerfstudio pov.

Does this mean that all connected clients would always receive all the broadcasted notifications though? I guess this question applies to the other things to consider. I feel like there are these two types of notifications in terms of broadcasting (global or client-specific). But I'm unsure if making this distinction between the two types of notifications will make it inconvenient for users and if client-specific messages are even possible.

brentyi commented 1 week ago

I guess this question applies to the other things to consider. I feel like there are these two types of notifications in terms of broadcasting (global or client-specific). But I'm unsure if making this distinction between the two types of notifications will make it inconvenient for users and if client-specific messages are even possible.

I think this should be already supported actually, if you call client.gui.add_notification then the notification will only go to a single client. If you want to test you can get the client handle from the GUI callback like in this example: https://viser.studio/latest/examples/19_get_renders/

ginazhouhuiwu commented 6 days ago

Ok that makes sense, for client notifications! Then I think the behavior for global broadcasting of notifications should be:

if we broadcast a notification, then a new client connects, should the new client receive the notification at all?

Yes, since the broadcasted notification isn't client specific then everybody should probably receive it.

if we broadcast a notification that autocloses in 5 seconds, then a new client connects 1 second later, is the notification shown for 4 seconds or 5 seconds?

I don't feel super strongly about this. I do think having the full temporary display of notification makes slightly more sense for users, so somebody doesn't get stuck with a 0.5 second notification that immediately disappears. But whatever makes the implementation cleaner I'm ok with.

if a notification is closed from one client, does it get closed in the others as well?

I don't think so, I think clients should control their own notification queue.

Let me know if you have any thoughts.

nerfstudio-project / viser

Basic notifications #225