Merge ~enr0n/ubuntu-release-upgrader:ubuntu/main into ubuntu-release-upgrader:ubuntu/main

Hi Nick,

I have only had a brief look at it so far, but can't help wondering:

When I struggled with the other MP, I found that setting DBUS_SESSION_BUS_ADDRESS and using subprocess instead of Gio.Settings were needed to make it work. Can you tell what the 'secret' is, or was i simply mistaken as regards those details?

DBUS_SESSION_BUS_ADDRESS is set in do-release-upgrade, so it is set correctly in this spot when the upgrade is run "for real". When you test manually, you are entering at a different place (i.e. do-release-upgrade is not executed directly), so you need to set in manually, e.g.

sudo env DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus ./mantic

Thanks for that explanation. As regards Gio I see that you used "settings.sync()" which I didn't. Maybe that's it. Or not, see below.

I have a couple of observations on the part of the code I understand. :/

The partition() method does not safely split the value into font family and size. It ought to work for e.g. 'Ubuntu 11', since the family is one single word, but it didn't work for me when testing. I had set 'Ubuntu 14' and it switched to 'Sans 11'.

=> This makes we wonder if the fetching of the user value is at all successful.

(And it would safely fail if the user value was e.g. 'Liberation Sans 12'.)

Another thing is that you use "settings.get_string" to grab the user font. That should give you a string irrespective of whether there actually is a specific user value or not. If not it gives you the system default ('Ubuntu 11' in standard Ubuntu).

In the equivalent variant of my code I used "settings.get_user_value" instead, which gives you None if there is no specific user value, which means that you can reset it with "gsettings reset".

With your way, the user dconf database will always get a font-name value even if there was no such value when the upgrade started. Maybe subtle and not so important...

> Thanks for that explanation. As regards Gio I see that you used
> "settings.sync()" which I didn't. Maybe that's it. Or not, see below.
>
> I have a couple of observations on the part of the code I understand. :/
>
> The partition() method does not safely split the value into font family and
> size. It ought to work for e.g. 'Ubuntu 11', since the family is one single
> word, but it didn't work for me when testing. I had set 'Ubuntu 14' and it
> switched to 'Sans 11'.

Ah yes, this should be changed. I will test the font size issue, but obviously that's not the intention.
>
> => This makes we wonder if the fetching of the user value is at all
> successful.
>
> (And it would safely fail if the user value was e.g. 'Liberation Sans 12'.)
>
> Another thing is that you use "settings.get_string" to grab the user font.
> That should give you a string irrespective of whether there actually is a
> specific user value or not. If not it gives you the system default ('Ubuntu
> 11' in standard Ubuntu).
>
> In the equivalent variant of my code I used "settings.get_user_value" instead,
> which gives you None if there is no specific user value, which means that you
> can reset it with "gsettings reset".
>
> With your way, the user dconf database will always get a font-name value even
> if there was no such value when the upgrade started. Maybe subtle and not so
> important...
In the general case, get_string('font-name') should give the same as get_user_value('font-name').get_string(), and if not I think getting the system default is the right thing to do.

On 2023-09-28 15:42, Nick Rosbrook wrote:
> In the general case, get_string('font-name') should give the same as
> get_user_value('font-name').get_string(), and if not I think getting
> the system default is the right thing to do.

Well, I'd say that the general case is that the user did not change the desktop font, so my belief is that they differ more often than not.

But this is a detail I'm not going to argue about more.

Alright, yeah I was missing something with the Gio.Settings approach, so shelling out to gsettings seems the way to go. I did this with `systemd-run --user -M $user@.host ...` to simplify the permissions/user env. That way, there is no need to change our running env or EUID -- just need to get the original UID via SUDO_UID or PKEXEC_UID.

TMTOWTDI. :) Looking forward to next variant.

> TMTOWTDI. :) Looking forward to next variant.

It was already pushed.

This looks good to me now. Didn't know that the rpartition() method existed, but it fits well in this context. And it did The Right Thing for me when testing.

Thanks for your work, and especially the idea to postpone the font resetting until after the reboot!

> This looks good to me now. Didn't know that the rpartition() method existed,
> but it fits well in this context. And it did The Right Thing for me when
> testing.
>
> Thanks for your work, and especially the idea to postpone the font resetting
> until after the reboot!

Glad it works well for you now. Thanks for all of your help on this!

This looks great to me, my only suggestion would be putting 'Sans' in a variable in case we change our minds some day.

> This looks great to me, my only suggestion would be putting 'Sans' in a
> variable in case we change our minds some day.

Ack, will do before I merge. Thanks!

On 2023-09-29 17:18, Brian Murray wrote:
> ... in case we change our minds some day.

FTR I have anticipated that we may want to change our minds in next cycle:

https://git.launchpad.net/~ubuntu-core-dev/ubuntu-seeds/+git/platform/commit/?id=67c4e53c

i.e. for the case the fonts-noto source package gets updated with latest upstream.