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

warning notifications #1688

Closed
totaam opened this issue Nov 14, 2017 · 6 comments
Closed

warning notifications #1688

totaam opened this issue Nov 14, 2017 · 6 comments

Comments

@totaam
Copy link
Collaborator

totaam commented Nov 14, 2017

If something goes wrong, the message ends up in the console output and server log (via remote logging), but most users won't look there and will therefore be unaware of the problem.

We should add an option to allow notifications to be used for major events, ie: another user joined the session, file uploads (#1375), the connection is having problems (#401)

The initial warnings during the client startup could be bunched up together to avoid spamming the notification system with multiple messages on startup.
In any case, this should probably be rate limited and controllable from the system tray menu.

Links:

See also: #1492, #1305

@totaam
Copy link
Collaborator Author

totaam commented Jan 22, 2018

Large change in r18110: refactoring + a user joining the session will trigger a notification message sent to:

  • all other connected users for seamless sessions
  • the session itself for shadow sessions

Still TODO:

@totaam
Copy link
Collaborator Author

totaam commented Jan 23, 2018

  • r18111 + r18113: the notifications are now tied to our system tray on win32
  • r18116: refactoring preparation for bandwidth congestion warnings

Still TODO:

  • better bandwidth congestion detection: the heuristics may be improved (detecting late acks) and this should fire the "congestion" handling code
  • maybe add a way for us to intercept the notification "action" response, so we can fire code server side?

@totaam
Copy link
Collaborator Author

totaam commented Jan 26, 2018

@totaam
Copy link
Collaborator Author

totaam commented Jan 30, 2018

From now on, we can easily add more warning notifications as we go along.


To test:

  • XPRA_ACK_TOLERANCE=0 may be enough to go over the threshold, or lower XPRA_CONGESTION_WARNING_EVENT_COUNT, and/or use a bandwidth limiter (see re-implement bandwidth constraint option #417), then generate lots bandwidth (ie: glxspheres): notification allows the client to lower the bandwidth limit (unless it is already so low it cannot be lowered)
  • enable sharing and connect with more than one client: notification shows details about the new user connecting
  • start the server with: xpra start --idle-timeout=20 and connect a client (you should be able to cancel the timeout)

All of this should work equally well on all platforms, including the html5 client. But full backwards compatibility with older clients is not possible and those will just not be aware of the notification actions.

@totaam
Copy link
Collaborator Author

totaam commented Feb 1, 2018

2018-02-01 23:52:22: maxmylyn commented


Okay I've tested this on every platform I have available to me immediately:

  • Fedora 26 (r18247)

  • Win8.1 (r18200 beta)

  • MacOSX (r18200 beta)

  • I made the mistake of clicking the update button thinking it would only take a few minutes. Boy was I wrong. I won't be making that mistake again.

Everything appears to be working as expected. Is there anything left to do or should I close this?

@totaam totaam closed this as completed Feb 2, 2018
@totaam
Copy link
Collaborator Author

totaam commented Feb 2, 2018

Also use notifications for reporting failures in:

Minor related fixes:

  • r18264: notification errors should not cause connection failures
  • r18265: use a valid notification id to prevent errors with some backends that require it to be a uint32 (ie: win32)
  • r18266: failures to show a notification should not cause further breakage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant