GNOME Bugzilla – Bug 575991
Allow editing device name for display
Last modified: 2019-03-20 10:36:41 UTC
Some of my devices have trailing/leading spaces and multiple spaces in the middle of their bt device names. Others use underscores at various places where spaces should be used. I think it would make sense to clean up the device names for presentation purposes. i.e. remove non-ascii characters, drop double whitespace, replace _ by space.
Hello, Just a dumb thought but real people like to have fun with their BT devices name and like to have something like "_\0/_". I don't think this would nice to strip some caracters, people would tend to think there is a bug in gnome-bluetooth. Perhaps I'm totally wrong.
That might be true for Mobile Phones and computers and stuff. But for fixed devices like headsets, printers, mice, keyboards this does not really apply. Also, dropping whitespace should not really limit the options for folks who want to set their own bt names.
Dropping leading, trailing and double whitespaces, as well as tabs and newlines should be fine. But removing underscores, or perfectly valid non-ASCII characters is out of the question. The only place we'd have to fix this in would be in bluetooth-client.c, and modify the "Alias" before updating the device's name.
Created attachment 132155 [details] [review] patch to clean the bluetooth device's alias name
clean_alias_string() needs to be a private function, we have no use using it in the wizard, as the data is exported from the BluetoothChooser, which in turn uses the BluetoothClient which we would already have cleaned up. Please add a space before the bracket in if's, or before curly braces. I'd also like to see some testing with UTF-8 strings if at all possible.
Oh, and applet/agent.c shouldn't use it either, it should use BluetoothClient instead.
Created attachment 132305 [details] [review] clean_alias_string with utf8 support This patch is specifically for bluetooth_client.c. You mentioned that applet/agent.c should use BlueoothClient instead however I will have to look into how to convert agent.c to use this. I should have time tomorrow to look into it. I did some utf8/unicode testing like suggested and that was a very good idea since it failed miserably on utf8 strings. It should not work properly. The test code can be seen here: http://codepad.org/QPM6FboV
Comment on attachment 132305 [details] [review] clean_alias_string with utf8 support >Index: common/bluetooth-client.c >=================================================================== >--- common/bluetooth-client.c (revision 575) >+++ common/bluetooth-client.c (working copy) >@@ -91,6 +91,55 @@ > > G_DEFINE_TYPE(BluetoothClient, bluetooth_client, G_TYPE_OBJECT) > >+gchar * clean_alias_string(const gchar *src); You don't need a prototype if it's the first function in the file. >+/** >+ * Remove all leading and trailing spaces from the alias string, as well >+ * as any double spacing characters. >+ */ >+gchar * clean_alias_string(const gchar *src) { >+ gunichar *unichar_string; >+ gunichar last_char; >+ glong num_chars; >+ gchar *cleaned_string; >+ gchar *dst; >+ int i; >+ >+ if (src == NULL) { >+ return NULL; >+ } >+ >+ unichar_string = g_utf8_to_ucs4_fast(src, -1, &num_chars); Why are you converting to UCS-4? >+ cleaned_string = g_malloc( (num_chars * sizeof(gint)) + 1); >+ dst = cleaned_string; >+ >+ last_char = unichar_string[0]; >+ for (i = 0; i < num_chars; i++) { Nope. Use a loop with g_utf8_next_char() instead, after having asserted that the string is valid UTF-8 (Using the aptly named g_utf8_validate()). Rest looks fine apart from the style problems (mind the spaces please).
Created attachment 132524 [details] [review] clean_alias_string with utf8 support Made suggested changes to clean_alias_string()
Comment on attachment 132524 [details] [review] clean_alias_string with utf8 support >Index: common/bluetooth-client.c >=================================================================== >--- common/bluetooth-client.c (revision 581) >+++ common/bluetooth-client.c (working copy) >@@ -91,6 +91,53 @@ > > G_DEFINE_TYPE(BluetoothClient, bluetooth_client, G_TYPE_OBJECT) > >+ No need for the extra linefeed here. >+gchar * clean_alias_string(const gchar *src) { >+ gunichar last_char; >+ gunichar curr_char; >+ gchar *cleaned_string; >+ gchar *dst; >+ gint len; >+ >+ if (src == NULL) { >+ return NULL; >+ } Don't have braces when the conditional branch fits on one-line. >+ Some extraneous tabs. >+ if (!g_utf8_validate(src, -1, NULL)) { >+ return NULL; >+ } >+ Same as above x 2. And I'd rather you sent back a copy of the original string when alias is not proper UTF-8. >+ len = g_utf8_strlen(src, -1); >+ cleaned_string = g_malloc( (len * sizeof(gint)) + 1); g_utf8_strlen() returns the length of the string in characters, not in bytes. So it's clearly wrong. (More extra curly braces and tabs later) <snip> >+ if (alias != NULL) >+ g_free(alias); g_free() can handle NULL, so no need for the conditional.
asweeney86@gmail.com could you update your patch and attach it here ? Cheers.
Just a note that the code would need to be exported for use in applet/agent.c as well.
Lennart mentioned that the Alias for a device could be changed through bluetoothd's API. It might be easier to add support to the properties for renaming the devices, as it would automatically trickle through the whole UI. The device properties dialogue would need to show the real device name if known as well.
*** Bug 597023 has been marked as a duplicate of this bug. ***
But even if you expose the alias stuff it might still make sense to drop redundant whitespace by default.
Given the security concerns (phishing) of doing so, I'm not too keen on it.
(In reply to comment #16) > Given the security concerns (phishing) of doing so, I'm not too keen on it. Phishin? How's that? Could you elaborate?
(In reply to comment #17) > (In reply to comment #16) > > Given the security concerns (phishing) of doing so, I'm not too keen on it. > > Phishin? How's that? Could you elaborate? Is it "Bastien's keyboard" or "Bastien's keyboard " (re-)connecting to my machine?
Created attachment 190941 [details] [review] properties: Allow editing a device's Alias XXX: Note that this patch doesn't implement setting the Alias in bluez itself.
Created attachment 190942 [details] [review] lib: Export the editable entry for the Bluetooth panel Necessary for the Bluetooth panel
One patch for gnome-bluetooth, and one for gnome-control-center. The problem, UI-wise, is that we can't get the labels and the editable entry's label to line up properly. Until there's a good way to do that, we can't really implement the feature.
Created attachment 200900 [details] [review] properties: Allow editing a device's Alias XXX: Note that this patch doesn't implement setting the Alias in bluez itself.
Created attachment 200906 [details] [review] bluetooth: Allow editing a device's Alias XXX: Note that this patch doesn't implement setting the Alias in bluez itself.
Created attachment 200907 [details] alignment problem
(In reply to comment #21) > The problem, UI-wise, is that we can't get the labels and the editable entry's > label to line up properly. Until there's a good way to do that, we can't really > implement the feature. I tried this, and I understand the align problem you described now. I think the problem is intrinsically trying to horizontally align a label with a button; a button content has padding coming from GtkButton, additional padding for the focus line rendering and maybe even more fancy spacings coming from style properties such as inner-border (which is disabled in Adwaita, but still...); if you look carefully you'll see the horizontal spacing is different as well, for the same reasons. In short, it's not going to work, since GtkLabel doesn't have any of these properties. I think a good compromise (the User Accounts panel seems to do the same FWIW) is to use a set of all CcEditableEntry widgets instead of GtkLabels, and mark only the ones that are actually editable as such. Another approach is to have the editable label as a first class citizen in GTK: https://bugzilla.gnome.org/show_bug.cgi?id=310693
(In reply to comment #25) <snip> > I think a good compromise (the User Accounts panel seems to do the same FWIW) > is to use a set of all CcEditableEntry widgets instead of GtkLabels, and mark > only the ones that are actually editable as such. Not that good a compromise, it looks like crap, as all the labels are utterly large padding. > Another approach is to have the editable label as a first class citizen in GTK: > https://bugzilla.gnome.org/show_bug.cgi?id=310693 I'll try and test it.
(In reply to comment #26) > > Another approach is to have the editable label as a first class citizen in GTK: > > https://bugzilla.gnome.org/show_bug.cgi?id=310693 > > I'll try and test it. Note that the patch there might be terribly out of date and not work properly with GTK master; if you want to go that way I'd recommend copy-pasting EelEditableLabel from Nautilus into gnome-control-center for now.
(In reply to comment #27) > (In reply to comment #26) > > > > Another approach is to have the editable label as a first class citizen in GTK: > > > https://bugzilla.gnome.org/show_bug.cgi?id=310693 > > > > I'll try and test it. > > Note that the patch there might be terribly out of date and not work properly > with GTK master; if you want to go that way I'd recommend copy-pasting > EelEditableLabel from Nautilus into gnome-control-center for now. I tested this, and it looks pretty horrible...
Created attachment 206011 [details] [review] test EelEditableLabel
I'm using the same model of bluetooth speakers at home and in my office. After pairing the two devices I have two identical entries "Creative T30 Wireless" in GNOME control center / bluetooth. I'd like to be able to add '(home)' and '(office)' to the device names, so I can pick the correct entry when I want to connect my speakers. For now I have to check the MAC address everytime (because the order/arrangement changes). Please allow editing device names in future releases. I'm using gnome 3.14.1 on debian jessie.
None of these patches apply on master anymore, and Eel was deprecated by Nautilus, so I'm not sure if that's what we want? I'm probably lacking some context here though.
(In reply to Georges Basile Stavracas Neto from comment #31) > None of these patches apply on master anymore, and Eel was deprecated by > Nautilus, so I'm not sure if that's what we want? I'm not sure we want it either. It should already be possible to work-around the display name on the command line, using the "set-alias" command in bluetoothctl after having connected to the device in question.
This code lives in gnome-bluetooth now.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-bluetooth/issues/13.