Created attachment 267567 [details] [review]
mtpvolume: fix a memory leak

Make the buffer really static, as the comment above documents.

Created attachment 267568 [details] [review]
mtpvolume: improve volume name

Replace _ and - with spaces, remove a set of common strings.
The idea (but not the code, because of incompatible license)
is taken from network-manager-applet.

Also, apply that pass on the product string too.

Created attachment 267569 [details] [review]
mtpvolume: implement symbolic icons

The places sidebar in nautilus and the places menu in the shell
use symbolic icons exclusively, so make sure one is provided, to
avoid falling back to a generic "network mount" icon.

Created attachment 267570 [details] [review]
various: reduce the verboseness of debug messages

With a recent enough dbus-daemon (or with kdbus), the standard
error of dbus activated services is sent to the journal, so
we should not pollute it with debug messages by default.

Review of attachment 267567 [details] [review]:

Thanks for your patch. I think returning static buffer isn't really good idea despite of it is safe in this case. Would be better to pass pointer into the function instead, or free allocated memory to fix leaking.

Review of attachment 267569 [details] [review]:

Looks fine.

Review of attachment 267567 [details] [review]:

I'm ok with this patch. The method is internal to the file, so the semantics can be whatever we want - and all the callers assume it's a static buffer that they need to duplicate themselves.

Review of attachment 267568 [details] [review]:

Is this really necessary? What's an example where the existing code doesn't do something reasonable?

Review of attachment 267570 [details] [review]:

Fine.

(In reply to comment #8)
> Review of attachment 267568 [details] [review]:
> 
> Is this really necessary? What's an example where the existing code doesn't do
> something reasonable?

Replacing '-' and '_' with ' ' is necessary, the other is just a seemingly good idea I borrowed from network-manager-applet

Btw, ideally we would use MTP_GetFriendlyName() here, but I don't know if that's possible without claiming the device and opening a session (which would later break gvfsd-mtp on mount).
Alternatively, we could hack something and watch the standard::display-name attribute of the mtp:// file, and update the mount name as it changes...

In my case, without the patch it says "SAMSUNG_Android", which is ugly. With the patch, it says "SAMSUNG Android", which is ok but suboptimal. Using GetFriendlyName it would say "GS4Campax", which is what I configured in the phone settings, and is consistent with how the phone is presented in bluetooth

(In reply to comment #10)
> 
> Btw, ideally we would use MTP_GetFriendlyName() here, but I don't know if
> that's possible without claiming the device and opening a session (which would
> later break gvfsd-mtp on mount).
> Alternatively, we could hack something and watch the standard::display-name
> attribute of the mtp:// file, and update the mount name as it changes...
> 
> In my case, without the patch it says "SAMSUNG_Android", which is ugly. With
> the patch, it says "SAMSUNG Android", which is ok but suboptimal. Using
> GetFriendlyName it would say "GS4Campax", which is what I configured in the
> phone settings, and is consistent with how the phone is presented in bluetooth

Where are you seeing it say this?

For example, I have a Nexus 5, and it shows up in Nautilus as "Nexus 5" with the existing code, and here's the interesting part of the ID section of the udev output for the device:

E: ID_GPHOTO2=1
E: ID_MEDIA_PLAYER=1
E: ID_MODEL=Nexus_5
E: ID_MODEL_ENC=Nexus\x205
E: ID_MODEL_FROM_DATABASE=Nexus 4 (debug)
E: ID_MODEL_ID=4ee2
E: ID_MTP_DEVICE=1
E: ID_PATH=pci-0000:00:14.0-usb-0:4
E: ID_PATH_TAG=pci-0000_00_14_0-usb-0_4
E: ID_REVISION=0232
E: ID_USB_INTERFACES=:ffff00:ff4201:
E: ID_VENDOR=LGE
E: ID_VENDOR_ENC=LGE
E: ID_VENDOR_FROM_DATABASE=Google Inc.
E: ID_VENDOR_ID=18d1

What does yours look like?

I have:

udevadm info /dev/bus/usb/002/048

E: ID_BUS=usb
E: ID_FOR_SEAT=usb-pci-0000_00_1d_7-usb-0_3
E: ID_GPHOTO2=1
E: ID_MEDIA_PLAYER=1
E: ID_MODEL=SAMSUNG_Android
E: ID_MODEL_ENC=SAMSUNG_Android
E: ID_MODEL_FROM_DATABASE=GT-I9100 Phone [Galaxy S II], GT-I9300 Phone [Galaxy S III], GT-P7500 [Galaxy Tab 10.1]
E: ID_MODEL_ID=6860
E: ID_MTP_DEVICE=1
E: ID_PATH=pci-0000:00:1d.7-usb-0:3
E: ID_PATH_TAG=pci-0000_00_1d_7-usb-0_3
E: ID_REVISION=0400
E: ID_SERIAL=SAMSUNG_SAMSUNG_Android_26191497
E: ID_SERIAL_SHORT=26191497
E: ID_USB_INTERFACES=:060101:020201:0a0000:
E: ID_VENDOR=SAMSUNG
E: ID_VENDOR_ENC=SAMSUNG
E: ID_VENDOR_FROM_DATABASE=Samsung Electronics Co., Ltd
E: ID_VENDOR_ID=04e8

gvfs-info 'mtp://[usb:002,048]/'

display name: GS4Campax
name: [usb:002,048]
type: directory
size:  0
uri: mtp://[usb:002,048]/
attributes:
  standard::type: 2
  standard::name: [usb:002,048]
  standard::display-name: GS4Campax
  standard::icon: multimedia-player
  standard::content-type: inode/directory
  standard::size: 0
  id::filesystem: mtp:host=%5Busb%3A002%2C048%5D
  access::can-read: TRUE
  access::can-write: TRUE
  access::can-execute: TRUE
  access::can-delete: FALSE
  access::can-trash: FALSE
  access::can-rename: TRUE
  filesystem::size: 17967579136
  filesystem::free: 9873321984
  filesystem::type: mtpfs

I guess the problem is that in your case, the space is encoded as \x20 in ID_MODEL_ENC, in my case it's left as _
In any case, being able to use display-name would be a lot better.

Friendlyname is not a magic solution - in the case of Nexus devices, there is no default friendly name and, I believe, no way to set it from the device itself. Setting the bluetooth name has no effect on the friendly name.

Interestingly, it can be renamed through libmtp with Set_Friendlyname, but that's not particularly useful.

So given that, and the complexity around connecting and disconnecting, I don't think the monitor should be trying to mess with it.

I'm fine with the -/_ -> ' ' replacement but I think we can skip the company name stuff unless that really shows up for people.

Pushed with suggested changes.

Attachment 267567 [details] pushed as 5076749 - mtpvolume: fix a memory leak
Attachment 267568 [details] pushed as 8a1d6a0 - mtpvolume: improve volume name
Attachment 267569 [details] pushed as ed451a1 - mtpvolume: implement symbolic icons
Attachment 267570 [details] pushed as edd777b - various: reduce the verboseness of debug messages