Code review comment for lp:~laney/ubuntu-system-settings/as-ringtone

Revision history for this message
Sebastien Bacher (seb128) wrote :

Thanks for the work, I just installed that, some issues/comments

* Sorry for not commenting about that earlier, but looking at the changes (dropping use of gsettings to monitor accountsservice) made me wonder if that's the right approch. I've commented on the bug with some details, but basically I think we should consider gsettings the canonical storage and only deal with that one, and have a service in the desktop that does the "gsettings<->accountsservice syncs".

It's likely that other elements are going to let the user change configs (e.g the phone app itself, third party code) and we don't want each of those to duplicate the sync code

* the migration script hits an error (I've the new glib installed)

ERROR:dbus.proxies:Introspect error on :1.16:/org/freedesktop/Accounts/User1000: dbus.exceptions.IntrospectionParserException: Error parsing introspect data: <class 'xml.parsers.expat.ExpatError'>: not well-formed (invalid token): line 63, column 84

Traceback (most recent call last):
  File "/usr/share/session-migration/scripts/ubuntu-system-settings-sound-gsettings-to-accountsservice.py", line 54, in <module>
    set_as_setting('com.ubuntu.touch.AccountsService.Sound', a, f(g))
  File "/usr/share/session-migration/scripts/ubuntu-system-settings-sound-gsettings-to-accountsservice.py", line 46, in set_as_setting
    dbus_interface=dbus.PROPERTIES_IFACE)
  File "/usr/lib/python3/dist-packages/dbus/proxies.py", line 70, in __call__
    return self._proxy_method(*args, **keywords)
  File "/usr/lib/python3/dist-packages/dbus/proxies.py", line 145, in __call__
    **keywords)
  File "/usr/lib/python3/dist-packages/dbus/connection.py", line 651, in call_blocking
    message, timeout)
dbus.exceptions.DBusException: org.freedesktop.DBus.Error.UnknownMethod: No such interface 'org.freedesktop.DBus.Properties' on object at path /org/freedesktop/Accounts/User1000

* deleting the destructor is ok, it's not related to those changes, would be nice to have cleanups made in different commits (same for the lines wrapping/spacing changes ;-)

* I'm probably overlooking something trivial, but in

" - soundSelector.selectedIndex = Utilities.indexSelectedFile(soundFileNames, soundSettings.incomingCallSound)
 + soundSelector.selectedIndex =
 + Utilities.indexSelectedFile(soundFileNames,
 + incomingCallSound)"

you drop the "soundSettings." prefix ... where is "incomingCallSound" defined?

The code otherwise looks fine, thanks for the work there!

review: Needs Fixing

« Back to merge proposal