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

Respect adapter power state in manager #2183

Merged
merged 4 commits into from
Nov 12, 2023

Conversation

cschramm
Copy link
Member

@cschramm cschramm commented Nov 4, 2023

Toolbar: Disable search and send buttons if the adapter is not powered (unpaired devices should not be present anyway).

Device menu: Do not add the following if not powered:

  • Connect items
  • Send file
  • Send note

Disable search and send buttons if the adapter is not powered (unpaired devices should not be present anyway).
@cschramm cschramm requested a review from infirit November 4, 2023 17:01
@cschramm cschramm enabled auto-merge (rebase) November 4, 2023 17:01
Do not add the following if not powered:

* Connect items
* Send file
* Send note
@cschramm cschramm changed the title Respect adapter power state in manager toolbar Respect adapter power state in manager Nov 4, 2023
@cschramm cschramm disabled auto-merge November 4, 2023 17:32
@cschramm cschramm enabled auto-merge (rebase) November 4, 2023 17:32
@infirit
Copy link
Contributor

infirit commented Nov 8, 2023

I'll test this over the weekend. I just realized we should probably include blocked devices, the same rules apply.

The error we display when unpowered is actually not that bad in the manager. I'm not at my machine but it's something including it being unpowered.

edit: This is the full error without this PR (Verbinding mislukt: br-connection-adapter-not-powered)

Copy link
Contributor

@infirit infirit left a comment

Choose a reason for hiding this comment

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

The ManagerDeviceMenu in the menubar is not regenerated like the popup. I think it will be enough to call self.device_menu.generate() in here https://github.com/blueman-project/blueman/blob/40449c0fc350cd62806da3407d1792c47b45adef/blueman/gui/manager/ManagerMenu.py#L174C24-L174C24

Otherwise lgtm.

edit: This works for me.

diff --git a/blueman/gui/manager/ManagerMenu.py b/blueman/gui/manager/ManagerMenu.py
index 8364cd7f2..f0fef04b4 100644
--- a/blueman/gui/manager/ManagerMenu.py
+++ b/blueman/gui/manager/ManagerMenu.py
@@ -59,6 +59,8 @@ class ManagerMenu:
         self._sort_alias_item = blueman.builder.get_widget("sort_name_item", Gtk.CheckMenuItem)
         self._sort_timestamp_item = blueman.builder.get_widget("sort_added_item", Gtk.CheckMenuItem)
 
+        self.device_menu: Optional[ManagerDeviceMenu] = None
+
         sort_config = self.Config['sort-by']
         if sort_config == "alias":
             self._sort_alias_item.props.active = True
@@ -105,8 +107,6 @@ class ManagerMenu:
         for adapter in self._manager.get_adapters():
             self.on_adapter_added(None, adapter.get_object_path())
 
-        self.device_menu: Optional[ManagerDeviceMenu] = None
-
         self.Config.connect("changed", self._on_settings_changed)
         self._sort_alias_item.connect("activate", self._on_sorting_changed, "alias")
         self._sort_timestamp_item.connect("activate", self._on_sorting_changed, "timestamp")
@@ -229,6 +229,9 @@ class ManagerMenu:
         self._update_power()
 
     def _update_power(self) -> None:
+        if self.device_menu is not None:
+            self.device_menu.generate()
+
         if any(adapter["Powered"] for (_, adapter) in self.adapter_items.values()):
             self.Search.props.visible = True
             self._adapter_settings.props.visible = True

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@cschramm
Copy link
Member Author

Good catch. I also just remembered that I wanted to care about the double click to connect.

I just realized we should probably include blocked devices, the same rules apply.

🤔 You mean you run into similar errors when trying to connect to a blocked device? Hm, true, makes sense.

The error we display when unpowered is actually not that bad in the manager. I'm not at my machine but it's something including it being unpowered.

edit: This is the full error without this PR (Verbinding mislukt: br-connection-adapter-not-powered)

If we expect br-connection-adapter-not-powered / le-connection-adapter-not-powered (and we actually should have before this) it should get added to ManagerDeviceMenu._BLUEZ_ERROR_MAP, but I think it's better to just avoid it completely. On the other hand, br-connection-aborted-by-local / le-connection-abort-by-local is expected when unpowering the adapter during a connection attempt. 😒

@cschramm cschramm merged commit 359ecea into blueman-project:main Nov 12, 2023
21 of 22 checks passed
@cschramm cschramm deleted the toolbar-powered branch November 13, 2023 07:50
@infirit
Copy link
Contributor

infirit commented Nov 13, 2023

I just realized we should probably include blocked devices, the same rules apply.

🤔 You mean you run into similar errors when trying to connect to a blocked device? Hm, true, makes sense.

The error is that the device does not respond after waiting for a long time. I'll do some testing now this landed.

edit: This is the full error without this PR (Verbinding mislukt: br-connection-adapter-not-powered)

If we expect br-connection-adapter-not-powered / le-connection-adapter-not-powered (and we actually should have before this) it should get added to ManagerDeviceMenu._BLUEZ_ERROR_MAP, but I think it's better to just avoid it completely. On the other hand, br-connection-aborted-by-local / le-connection-abort-by-local is expected when unpowering the adapter during a connection attempt. 😒

I'll play around a bit and see if I can get those errors and add them to the map.

@cschramm
Copy link
Member Author

cschramm commented Nov 13, 2023

I just wanted to write that it's super easy: Just start connecting to a device that is not online and then turn off Bluetooth. Funny enough, I did not get abort(ed)-by-local (ECONNABORTED) this time, but by-remote (ECONNRESET)... The problem with these is that I think they can actually mean a range of things. I even explicitly excluded them from #1363 (not knowing that you can easily reach them by playing with the power state).

@cschramm
Copy link
Member Author

cschramm commented Nov 13, 2023

Oh, I did not know of https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/errors.txt yet. At least it's pretty clear on ECONNRESET. Not so much on ECONNABORTED.

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