bluez5: Clean up static default properties, re-drop PairDevice class_ parameter #192
Conversation
martinpitt
force-pushed
the
bluez-cleanup
branch
from
fc5afe8
to
4b32ade
Compare
… parameter Over the years, this template has accumulated some hacks and bad API which made PairDevice()'s handling of the Modalias/Class/Icon properties buggy and hard to understand: * These are *static* device properties, they are not supposed to change during pairing. * Commit ee29a44 added these as some kind of "dynamic fallback default" when they were not initialized by the caller after AddDevice(). * Commit 59d6af0 silently broke that fallback default by changing AddDevice() to set these device properties to empty strings. * Commit fae4be7 added another really bad API for setting Class in PairDevice()(). That API didn't fit into D-Bus (see commit 8968284 which had to make it a non-default parameter) and also broke the API, and moreover it is totally unintuitive -- the device class has nothing to do with pairing. Clean up all of these: Set the static property defaults in AddDevice() right away, so that the caller can adjust them afterwards. Re-drop the `class_` argument in PairDevice(). Adjust the documentation of AddDevice() to point out that properties should be changed after calling that. Consequently, PairDevice() will stop claiming that the static properties changed. This also gets rid of some redundant code.
martinpitt
force-pushed
the
bluez-cleanup
branch
from
4b32ade
to
381d2e9
Compare
IIRC, I was using BlueZ in python-dbusmock to test part of libfolks, which is something I haven’t worked on for a long time now. I’m afraid I’m not going to resurrect my involvement in it to test this PR, sorry! If (in the unlikely case that) this PR breaks things in libfolks, I’m sure libfolks can be reworked to use the new approach here. |
|
Thanks @pwithnall. @ricotz and @nielsdg you seem to have worked on libfolks recently -- does this change work for/make sense to you? |
|
I think it's fine to land this since it seems to fix a couple of issues and we can fix this in a reactive manner once it breaks CI (and honestly, i had to fix our CI for something similar a while ago already, see https://gitlab.gnome.org/GNOME/folks/-/commit/b6b7d41aad9e36e0). The bluez backend is barely used, and this would only affect testing at this point in any case. |
|
Thanks @nielsdg for checking! Let's go with this then, even if it causes some noise I think it'll make the whole thing a lot easier to understand and adjust at least. |
|
@nielsdg And right, https://gitlab.gnome.org/GNOME/folks/-/commit/b6b7d41aad9e36e0 would have to be reverted again. |
|
Sorry for the delay, I tested with lomiri-system-settings tests and they do work also with this PR :) Thanks for fixing this! But also similarly we need to revert this commit there https://gitlab.com/ubports/development/core/lomiri-system-settings/-/commit/b9aacd88e3789dbb7578f32b31ad5b239db227a2 |
This reverts commit b6b7d41. See martinpitt/python-dbusmock#192
dbusmock 0.30.1 changed the BlueZ template to set the default "Class" property to `MOCK_PHONE_CLASS` right away instead of in PairDevice() [1]. test_bluetooth_le_device() relied on the previous implicit default of a "0" Class value. Set this explicitly to expect a "generic" device. This makes the test work with old and current dbusmock versions. https://bugs.debian.org/1059467 [1] martinpitt/python-dbusmock#192
dbusmock 0.30.1 changed the BlueZ template to set the default "Class" property to `MOCK_PHONE_CLASS` right away instead of in PairDevice() [1]. test_bluetooth_le_device() relied on the previous implicit default of a "0" Class value. Set this explicitly to expect a "generic" device. This makes the test work with old and current dbusmock versions. https://bugs.debian.org/1059467 [1] martinpitt/python-dbusmock#192
dbusmock 0.30.1 changed the BlueZ template to set the default "Class" property to `MOCK_PHONE_CLASS` right away instead of in PairDevice() [1]. test_bluetooth_le_device() relied on the previous implicit default of a "0" Class value. Set this explicitly to expect a "generic" device. This makes the test work with old and current dbusmock versions. https://bugs.debian.org/1059467 [1] martinpitt/python-dbusmock#192
Test-only fix so no revbump. Bug: https://bugs.gentoo.org/923794 Bug: https://bugs.gentoo.org/927405 See: https://gitlab.freedesktop.org/upower/upower/-/merge_requests/207 See: https://bugs.debian.org/1059467 See: martinpitt/python-dbusmock#192 Signed-off-by: Matoro Mahri <matoro_gentoo@matoro.tk>
Over the years, this template has accumulated some hacks and bad API which made PairDevice()'s handling of the Modalias/Class/Icon properties buggy and hard to understand:
Clean up all of these: Set the static property defaults in AddDevice() right away, so that the caller can adjust them afterwards. Re-drop the
class_argument in PairDevice(). Adjust the documentation of AddDevice() to point out that properties should be changed after calling that.Consequently, PairDevice() will stop claiming that the static properties changed. This also gets rid of some redundant code.
@z3ntu, this is meant to fix and Closes #190 .
@pwithnall @hadess @bjoernh I admittedly don't understand the use cases/current callers of this code. I think some API change is inevitable here to clean up, but it should at least make sense. I'd appreciate if you could check my reasoning and do some sanity testing on this with your existing tests. Cheers!