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

Feature room bot can be removed after disconnection #4

Merged
merged 20 commits into from
Nov 29, 2023

Conversation

VincePaulin
Copy link

Added the ability to delete a room with a Bot Bridge after trying to disconnect from it.

Copy link

@ignyx ignyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many good things ! Some parts should be broken down into smaller components/functions/variables for better readability. Some errors appear not be handled. Looks great otherwise !

assets/l10n/intl_en.arb Outdated Show resolved Hide resolved
assets/l10n/intl_en.arb Outdated Show resolved Hide resolved
assets/l10n/intl_en.arb Outdated Show resolved Hide resolved
lib/pages/add_bridge/add_bridge_body.dart Outdated Show resolved Hide resolved
lib/pages/add_bridge/add_bridge_body.dart Outdated Show resolved Hide resolved
lib/pages/add_bridge/service/bot_bridge_connection.dart Outdated Show resolved Hide resolved
lib/pages/add_bridge/show_bottom_sheet.dart Outdated Show resolved Hide resolved
pubspec.yaml Outdated Show resolved Hide resolved
assets/l10n/intl_en.arb Outdated Show resolved Hide resolved
@ignyx
Copy link

ignyx commented Nov 21, 2023

Multiple things arose while initially testing :

  • the pinging loop does not stop when the user leaves the network page (ping messages are still being sent)
  • failed to identify an error message (below), but should consider any unknown message as an error
  • pinging loop should stop after a certain number of failed tries (maybe 10) instead of a while (true) condition

Unrecognized error message :

You are not whitelisted to use this bridge.

If you are the owner of this bridge, see the bridge.permissions section in your config file.

Testing was done on alpha, which explains the whitelisting issue. However this should be treated as an error and not ping indefinitely. Note that after sending the whitelist error message, the bridge bot leaves the room.

Copy link

@ignyx ignyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good ! A few minor details

lib/pages/add_bridge/add_bridge_body.dart Outdated Show resolved Hide resolved
lib/pages/add_bridge/show_bottom_sheet.dart Outdated Show resolved Hide resolved
@ignyx
Copy link

ignyx commented Nov 24, 2023

Awesome ! One detail : the login username password message was sent one too many times, so it is not automatically deleted by the bot (in the screenshot I deleted it manually).

image

Copy link

@ignyx ignyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ignyx ignyx merged commit b848297 into dev Nov 29, 2023
3 of 5 checks passed
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

Successfully merging this pull request may close these issues.

2 participants