Repository: hikari-py/hikari
Pull request #1369 (2022-11-20 17:28:53)

Improve voice listener
- Dont register listener unless needed
- Fix shutting down voice errors

### Checklist
<!-- Make sure to tick all the following boxes by putting an `x` in between (like this `[x]`) -->
- [x] I have run `nox` and all the pipelines have passed.
- [x] I have made unittests according to the code I have added/modified/deleted.




Patch:
diff --git a/changes/1367.bugfix.md b/changes/1367.bugfix.md
new file mode 100644
index 00000000..1528e6db
--- /dev/null
+++ b/changes/1367.bugfix.md
@@ -0,0 +1,1 @@
+Allow re-uploading attachments when creating messages
diff --git a/changes/1369.bugfix.md b/changes/1369.bugfix.md
new file mode 100644
index 00000000..df835a05
--- /dev/null
+++ b/changes/1369.bugfix.md
@@ -0,0 +1,1 @@
+Fix error caused when disconnecting the bot and having active voice connections.
diff --git a/changes/1369.feature.md b/changes/1369.feature.md
new file mode 100644
index 00000000..17cbbf60
--- /dev/null
+++ b/changes/1369.feature.md
@@ -0,0 +1,1 @@
+Only subscribe to voice events when needed in the voice manager.
diff --git a/dev-requirements/flake8.txt b/dev-requirements/flake8.txt
index 15fef9f8..8a93b9a0 100644
--- a/dev-requirements/flake8.txt
+++ b/dev-requirements/flake8.txt
@@ -4,7 +4,7 @@ flake8==5.0.4
 # Ref: https://github.com/DmytroLitvinov/awesome-flake8-extensions
 
 flake8-bandit~=4.1.1                    # runs bandit
-flake8-black==0.3.4                     # runs black
+flake8-black==0.3.5                     # runs black
 flake8-broken-line==0.6.0               # forbey "\" linebreaks
 flake8-builtins==2.0.1                  # builtin shadowing checks
 flake8-coding==1.3.2                    # coding magic-comment detection
@@ -14,7 +14,7 @@ flake8-executable==2.1.2                # shebangs
 flake8-fixme==1.1.1                     # "fix me" counter
 flake8-functions==0.0.7                 # function linting
 flake8-html==0.4.2                      # html output
-flake8-isort==5.0.0                     # runs isort
+flake8-isort==5.0.3                     # runs isort
 flake8-mutable==1.2.0                   # mutable default argument detection
 flake8-pep3101==2.0.0                   # new-style format strings only
 flake8-print==5.0.0                     # complain about print statements in code
diff --git a/dev-requirements/nox.txt b/dev-requirements/nox.txt
index 19ac118c..749e4f01 100644
--- a/dev-requirements/nox.txt
+++ b/dev-requirements/nox.txt
@@ -1,1 +1,1 @@
-nox==2022.8.7
+nox==2022.11.21
diff --git a/dev-requirements/pyright.txt b/dev-requirements/pyright.txt
index c3f7fb20..9d3c5ee6 100644
--- a/dev-requirements/pyright.txt
+++ b/dev-requirements/pyright.txt
@@ -1,1 +1,1 @@
-pyright==1.1.280
+pyright==1.1.281
diff --git a/dev-requirements/slotscheck.txt b/dev-requirements/slotscheck.txt
index aca35f68..89c1338f 100644
--- a/dev-requirements/slotscheck.txt
+++ b/dev-requirements/slotscheck.txt
@@ -1,1 +1,1 @@
-slotscheck==0.16.0
+slotscheck==0.16.1
diff --git a/hikari/impl/bot.py b/hikari/impl/bot.py
index 073f591c..0aae257b 100644
--- a/hikari/impl/bot.py
+++ b/hikari/impl/bot.py
@@ -433,12 +433,13 @@ async def handle(name: str, awaitable: typing.Awaitable[typing.Any]) -> None:
                     }
                 )
 
+        await handle("voice handler", self._voice.close())
+
         shards = tuple((handle(f"shard {s.id}", s.close()) for s in self._shards.values() if s.is_alive))
 
         for coro in asyncio.as_completed(shards):
             await coro
 
-        await handle("voice handler", self._voice.close())
         await handle("rest", self._rest.close())
 
         # Clear out cache and shard map
diff --git a/hikari/impl/rest.py b/hikari/impl/rest.py
index feecdea8..2c14f712 100644
--- a/hikari/impl/rest.py
+++ b/hikari/impl/rest.py
@@ -1327,7 +1327,7 @@ def _build_message_payload(  # noqa: C901- Function too complex
             attachment_id = 0
 
             for f in final_attachments:
-                if isinstance(f, messages_.Attachment):
+                if edit and isinstance(f, messages_.Attachment):
                     attachments_payload.append({"id": f.id, "filename": f.filename})
                     continue
 
diff --git a/hikari/impl/voice.py b/hikari/impl/voice.py
index f3c26f2c..0dfc530d 100644
--- a/hikari/impl/voice.py
+++ b/hikari/impl/voice.py
@@ -54,7 +54,14 @@ class VoiceComponentImpl(voice.VoiceComponent):
     voice channels with.
     """
 
-    __slots__: typing.Sequence[str] = ("_app", "_connections", "connections", "_is_alive", "_is_closing")
+    __slots__: typing.Sequence[str] = (
+        "_app",
+        "_connections",
+        "connections",
+        "_is_alive",
+        "_is_closing",
+        "_voice_listener",
+    )
 
     _connections: typing.Dict[snowflakes.Snowflake, voice.VoiceConnection]
     connections: typing.Mapping[snowflakes.Snowflake, voice.VoiceConnection]
@@ -65,6 +72,7 @@ def __init__(self, app: traits.GatewayBotAware) -> None:
         self.connections = types.MappingProxyType(self._connections)
         self._is_alive = False
         self._is_closing = False
+        self._voice_listener = False
 
     @property
     def is_alive(self) -> bool:
@@ -101,7 +109,9 @@ async def disconnect_all(self) -> None:
     async def close(self) -> None:
         self._check_if_alive()
         self._is_closing = True
-        self._app.event_manager.unsubscribe(voice_events.VoiceEvent, self._on_voice_event)
+
+        if self._voice_listener:
+            self._app.event_manager.unsubscribe(voice_events.VoiceEvent, self._on_voice_event)
 
         if self._connections:
             _LOGGER.info("shutting down %s active voice connection(s)", len(self._connections))
@@ -109,6 +119,7 @@ async def close(self) -> None:
 
         self._is_alive = False
         self._is_closing = False
+        self._voice_listener = False
 
     def start(self) -> None:
         """Start this voice component."""
@@ -116,7 +127,7 @@ def start(self) -> None:
             raise errors.ComponentStateConflictError("Cannot start a voice component which is already running")
 
         self._is_alive = True
-        self._app.event_manager.subscribe(voice_events.VoiceEvent, self._on_voice_event)
+        self._voice_listener = False
 
     async def connect_to(
         self,
@@ -216,6 +227,10 @@ async def connect_to(
 
             raise
 
+        if not self._voice_listener:
+            self._app.event_manager.subscribe(voice_events.VoiceEvent, self._on_voice_event)
+            self._voice_listener = True
+
         self._connections[guild_id] = voice_connection
         return voice_connection
 
@@ -242,6 +257,10 @@ async def _on_connection_close(self, connection: voice.VoiceConnection) -> None:
         try:
             del self._connections[connection.guild_id]
 
+            if not self._connections:
+                self._app.event_manager.unsubscribe(voice_events.VoiceEvent, self._on_voice_event)
+                self._voice_listener = False
+
             # Leave the voice channel explicitly, otherwise we will just appear to
             # not leave properly.
             await self._app.shards[connection.shard_id].update_voice_state(
diff --git a/hikari/messages.py b/hikari/messages.py
index 2dbc1569..22355725 100644
--- a/hikari/messages.py
+++ b/hikari/messages.py
@@ -201,6 +201,8 @@ class Attachment(snowflakes.Unique, files.WebResource):
 
     You can use this object in the same way as a `hikari.files.WebResource`,
     by passing it as an attached file when creating a message, etc.
+
+    It can also be used when editing a message to keep a previous attachment.
     """
 
     id: snowflakes.Snowflake = attr.field(hash=True, repr=True)


Test patch:
diff --git a/tests/hikari/impl/test_bot.py b/tests/hikari/impl/test_bot.py
index a1b0a208..7c37ca11 100644
--- a/tests/hikari/impl/test_bot.py
+++ b/tests/hikari/impl/test_bot.py
@@ -424,10 +424,10 @@ def assert_awaited_once(self):
         # Closing components
         ensure_future.assert_has_calls(
             [
+                mock.call(voice.close()),
                 mock.call(shard0.close()),
                 mock.call(shard1.close()),
                 mock.call(shard2.close()),
-                mock.call(voice.close()),
                 mock.call(rest.close()),
             ],
         )
diff --git a/tests/hikari/impl/test_rest.py b/tests/hikari/impl/test_rest.py
index 8184f9eb..b013b522 100644
--- a/tests/hikari/impl/test_rest.py
+++ b/tests/hikari/impl/test_rest.py
@@ -1381,6 +1381,7 @@ def test__build_message_payload_with_plural_args(self, rest_client):
         resource_attachment3 = mock.Mock(filename="attachment3.png")
         resource_attachment4 = mock.Mock(filename="attachment4.png")
         resource_attachment5 = mock.Mock(filename="attachment5.png")
+        resource_attachment6 = mock.Mock(filename="attachment6.png")
         component1 = mock.Mock(build=mock.Mock(return_value={"component": 1}))
         component2 = mock.Mock(build=mock.Mock(return_value={"component": 2}))
         embed1 = object()
@@ -1405,6 +1406,7 @@ def test__build_message_payload_with_plural_args(self, rest_client):
                     resource_attachment3,
                     resource_attachment4,
                     resource_attachment5,
+                    resource_attachment6,
                 ],
             )
         )
@@ -1440,21 +1442,22 @@ def test__build_message_payload_with_plural_args(self, rest_client):
             "components": [{"component": 1}, {"component": 2}],
             "attachments": [
                 {"id": 0, "filename": "attachment.png"},
-                {"id": 123, "filename": "attachment123.png"},
                 {"id": 1, "filename": "attachment2.png"},
                 {"id": 2, "filename": "attachment3.png"},
                 {"id": 3, "filename": "attachment4.png"},
                 {"id": 4, "filename": "attachment5.png"},
+                {"id": 5, "filename": "attachment6.png"},
             ],
             "allowed_mentions": {"allowed_mentions": 1},
         }
         assert form is url_encoded_form.return_value
 
         # Attachments
-        assert ensure_resource.call_count == 5
+        assert ensure_resource.call_count == 6
         ensure_resource.assert_has_calls(
             [
                 mock.call(attachment1),
+                mock.call(attachment2),
                 mock.call(embed_attachment1),
                 mock.call(embed_attachment2),
                 mock.call(embed_attachment3),
@@ -1475,6 +1478,103 @@ def test__build_message_payload_with_plural_args(self, rest_client):
             mentions_everyone, mentions_reply, user_mentions, role_mentions
         )
 
+        # Form builder
+        url_encoded_form.assert_called_once_with(executor=rest_client._executor)
+        assert url_encoded_form.return_value.add_resource.call_count == 6
+        url_encoded_form.return_value.add_resource.assert_has_calls(
+            [
+                mock.call("files[0]", resource_attachment1),
+                mock.call("files[1]", resource_attachment2),
+                mock.call("files[2]", resource_attachment3),
+                mock.call("files[3]", resource_attachment4),
+                mock.call("files[4]", resource_attachment5),
+                mock.call("files[5]", resource_attachment6),
+            ]
+        )
+
+    def test__build_message_payload_with_edit_and_attachment_object_passed(self, rest_client):
+        attachment1 = object()
+        attachment2 = mock.Mock(message_models.Attachment, id=123, filename="attachment123.png")
+        resource_attachment1 = mock.Mock(filename="attachment.png")
+        resource_attachment2 = mock.Mock(filename="attachment2.png")
+        resource_attachment3 = mock.Mock(filename="attachment3.png")
+        resource_attachment4 = mock.Mock(filename="attachment4.png")
+        resource_attachment5 = mock.Mock(filename="attachment5.png")
+        component1 = mock.Mock(build=mock.Mock(return_value={"component": 1}))
+        component2 = mock.Mock(build=mock.Mock(return_value={"component": 2}))
+        embed1 = object()
+        embed2 = object()
+        embed_attachment1 = object()
+        embed_attachment2 = object()
+        embed_attachment3 = object()
+        embed_attachment4 = object()
+
+        stack = contextlib.ExitStack()
+        ensure_resource = stack.enter_context(
+            mock.patch.object(
+                files,
+                "ensure_resource",
+                side_effect=[
+                    resource_attachment1,
+                    resource_attachment2,
+                    resource_attachment3,
+                    resource_attachment4,
+                    resource_attachment5,
+                ],
+            )
+        )
+        url_encoded_form = stack.enter_context(mock.patch.object(data_binding, "URLEncodedFormBuilder"))
+        rest_client._entity_factory.serialize_embed.side_effect = [
+            ({"embed": 1}, [embed_attachment1, embed_attachment2]),
+            ({"embed": 2}, [embed_attachment3, embed_attachment4]),
+        ]
+
+        with stack:
+            body, form = rest_client._build_message_payload(
+                content=987654321,
+                attachments=[attachment1, attachment2],
+                components=[component1, component2],
+                embeds=[embed1, embed2],
+                flags=120,
+                tts=True,
+                mentions_everyone=None,
+                mentions_reply=None,
+                user_mentions=None,
+                role_mentions=None,
+                edit=True,
+            )
+
+        # Returned
+        assert body == {
+            "content": "987654321",
+            "tts": True,
+            "flags": 120,
+            "embeds": [{"embed": 1}, {"embed": 2}],
+            "components": [{"component": 1}, {"component": 2}],
+            "attachments": [
+                {"id": 0, "filename": "attachment.png"},
+                {"id": 123, "filename": "attachment123.png"},
+                {"id": 1, "filename": "attachment2.png"},
+                {"id": 2, "filename": "attachment3.png"},
+                {"id": 3, "filename": "attachment4.png"},
+                {"id": 4, "filename": "attachment5.png"},
+            ],
+            "allowed_mentions": {"parse": []},
+        }
+        assert form is url_encoded_form.return_value
+
+        # Attachments
+        assert ensure_resource.call_count == 5
+        ensure_resource.assert_has_calls(
+            [
+                mock.call(attachment1),
+                mock.call(embed_attachment1),
+                mock.call(embed_attachment2),
+                mock.call(embed_attachment3),
+                mock.call(embed_attachment4),
+            ]
+        )
+
         # Form builder
         url_encoded_form.assert_called_once_with(executor=rest_client._executor)
         assert url_encoded_form.return_value.add_resource.call_count == 5
diff --git a/tests/hikari/impl/test_voice.py b/tests/hikari/impl/test_voice.py
index a1d8f321..3edffc02 100644
--- a/tests/hikari/impl/test_voice.py
+++ b/tests/hikari/impl/test_voice.py
@@ -112,33 +112,45 @@ async def test_disconnect_all(self, voice_client):
         voice_client._disconnect_all.assert_awaited_once_with()
 
     @pytest.mark.asyncio()
-    async def test_close(self, voice_client, mock_app):
+    @pytest.mark.parametrize("voice_listener", [True, False])
+    async def test_close(self, voice_client, mock_app, voice_listener):
         voice_client._disconnect_all = mock.AsyncMock()
         voice_client._connections = {123: None}
         voice_client._check_if_alive = mock.Mock()
+        voice_client._voice_listener = voice_listener
 
         await voice_client.close()
 
+        if voice_listener:
+            mock_app.event_manager.unsubscribe.assert_called_once_with(
+                voice_events.VoiceEvent, voice_client._on_voice_event
+            )
+        else:
+            mock_app.event_manager.unsubscribe.assert_not_called()
+
         voice_client._check_if_alive.assert_called_once_with()
-        mock_app.event_manager.unsubscribe.assert_called_once_with(
-            voice_events.VoiceEvent, voice_client._on_voice_event
-        )
         voice_client._disconnect_all.assert_awaited_once_with()
         assert voice_client._is_alive is False
         assert voice_client._is_closing is False
 
     @pytest.mark.asyncio()
-    async def test_close_when_no_connections(self, voice_client, mock_app):
+    @pytest.mark.parametrize("voice_listener", [True, False])
+    async def test_close_when_no_connections(self, voice_client, mock_app, voice_listener):
         voice_client._disconnect_all = mock.AsyncMock()
         voice_client._connections = {}
         voice_client._check_if_alive = mock.Mock()
+        voice_client._voice_listener = voice_listener
 
         await voice_client.close()
 
+        if voice_listener:
+            mock_app.event_manager.unsubscribe.assert_called_once_with(
+                voice_events.VoiceEvent, voice_client._on_voice_event
+            )
+        else:
+            mock_app.event_manager.unsubscribe.assert_not_called()
+
         voice_client._check_if_alive.assert_called_once_with()
-        mock_app.event_manager.unsubscribe.assert_called_once_with(
-            voice_events.VoiceEvent, voice_client._on_voice_event
-        )
         voice_client._disconnect_all.assert_not_called()
         assert voice_client._is_alive is False
         assert voice_client._is_closing is False
@@ -148,7 +160,6 @@ def test_start(self, voice_client, mock_app):
 
         voice_client.start()
 
-        mock_app.event_manager.subscribe.assert_called_once_with(voice_events.VoiceEvent, voice_client._on_voice_event)
         assert voice_client._is_alive is True
 
     @pytest.mark.asyncio()
@@ -159,7 +170,8 @@ async def test_start_when_already_alive(self, voice_client, mock_app):
             await voice_client.start()
 
     @pytest.mark.asyncio()
-    async def test_connect_to(self, voice_client, mock_app):
+    @pytest.mark.parametrize("voice_listener", [True, False])
+    async def test_connect_to(self, voice_client, mock_app, voice_listener):
         voice_client._init_state_update_predicate = mock.Mock()
         voice_client._init_server_update_predicate = mock.Mock()
         mock_other_connection = object()
@@ -170,6 +182,7 @@ async def test_connect_to(self, voice_client, mock_app):
         mock_app.shards = {0: mock_shard}
         mock_connection_type = mock.AsyncMock()
         voice_client._check_if_alive = mock.Mock()
+        voice_client._voice_listener = voice_listener
 
         result = await voice_client.connect_to(123, 4532, mock_connection_type, deaf=False, mute=True, timeout=None)
 
@@ -192,6 +205,14 @@ async def test_connect_to(self, voice_client, mock_app):
         mock_app.cache.get_me.assert_called_once_with()
         voice_client._init_state_update_predicate.assert_called_once_with(123, mock_app.cache.get_me.return_value.id)
         voice_client._init_server_update_predicate.assert_called_once_with(123)
+        if voice_listener:
+            mock_app.event_manager.subscribe.assert_not_called()
+        else:
+            mock_app.event_manager.subscribe.assert_called_once_with(
+                voice_events.VoiceEvent, voice_client._on_voice_event
+            )
+
+        assert voice_client._voice_listener is True
         mock_shard.update_voice_state.assert_awaited_once_with(
             123,
             4532,
@@ -324,16 +345,28 @@ class StubError(Exception):
         asyncio_wait_for.assert_awaited_once_with(update_voice_state_call_2.return_value, timeout=5.0)
 
     @pytest.mark.asyncio()
-    async def test__on_connection_close(self, voice_client, mock_app):
-        mock_other_connection = object()
+    @pytest.mark.parametrize("more_connections", [True, False])
+    async def test__on_connection_close(self, voice_client, mock_app, more_connections):
         mock_shard = mock.AsyncMock()
         mock_app.shards = {69: mock_shard}
-        voice_client._connections = {65234123: mock_other_connection, 123123: object()}
-
-        await voice_client._on_connection_close(mock.Mock(guild_id=123123, shard_id=69))
-
-        mock_shard.update_voice_state.assert_awaited_once_with(guild=123123, channel=None)
-        assert voice_client._connections == {65234123: mock_other_connection}
+        voice_client._connections = {65234123: object()}
+        expected_connections = {}
+        if more_connections:
+            mock_connection = object()
+            voice_client._connections[123] = mock_connection
+            expected_connections[123] = mock_connection
+
+        await voice_client._on_connection_close(mock.Mock(guild_id=65234123, shard_id=69))
+
+        if more_connections:
+            mock_app.event_manager.unsubscribe.assert_not_called()
+        else:
+            mock_app.event_manager.unsubscribe.assert_called_once_with(
+                voice_events.VoiceEvent, voice_client._on_voice_event
+            )
+
+        mock_shard.update_voice_state.assert_awaited_once_with(guild=65234123, channel=None)
+        assert voice_client._connections == expected_connections
 
     def test__init_state_update_predicate_matches(self, voice_client):
         predicate = voice_client._init_state_update_predicate(42069, 696969)