Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: properly handle installer language changes #1746

Merged
merged 18 commits into from
Nov 14, 2024
Merged

fix: properly handle installer language changes #1746

merged 18 commits into from
Nov 14, 2024

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Nov 11, 2024

Problem

Problems:

Solution

Fix the D-Bus path of the LocaleProxy to the Manager's org.opensuse.Agama1.Locale interface.

Testing

  • Tested manually

@dgdavid
Copy link
Contributor

dgdavid commented Nov 12, 2024

@imobachgs,

According to the PR's description, looks to me you're working on https://trello.com/c/isE3194l/620-agama-translations-not-working-consitently too. Please, have a look.

Thanks for addressing these translation issues.

@@ -20,7 +20,8 @@
//! [D-Bus standard interfaces]: https://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces,
use zbus::proxy;
#[proxy(
default_service = "org.opensuse.Agama1",
default_service = "org.opensuse.Agama.Manager1",
default_path = "/org/opensuse/Agama/Manager1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch... except it does not work: 🤣 😢

When I run this code and change the UI langue in the browser, journalctl has:

Nov 12 13:10:23 c75fbede8655 agama-web-server[28278]: Sending message: Msg { type: MethodCall, serial: 157, sender: UniqueName(":1.7"), path: ObjectPath("/org/freedesktop/Locale"), iface: InterfaceName("org.freedesktop.DBus.Properties"), member: MemberName("Set"), body: Structure(Dynamic { fields: [Str, Str, Variant] }), fds: [] }
...
Nov 12 13:10:23 c75fbede8655 agama-web-server[28278]: 5:185m5:185mCould not synchronize settings in the localization D-Bus service: D-Bus service error: org.freedesktop.DBus.Error.UnknownObject: Unknown object '/org/freedesktop/Locale'

Looking up "Could not synchronize settings" in the source, I arrive at a update_dbus fn, which uses a set_locales fn, defined in...

fn set_locales(&self, value: &[&str]) -> zbus::Result<()>;

Which is a similar, yet different file from this diff
🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it works for me (TM) 😉 Actually, I would not expect the set_locales method to be involved because that's responsible for changing the locales to install, not the installer's language. Are you changing the language through the "Installer Options" menu? Did you restart the agama-dbus-server process too (restarting the agama.service should be enough)?

Having two interfaces with the same name (org.opensuse.Agama1.Locale) is unfortunate, I know, but they are different things:

  • localization/proxies refers to the Locale API (implemented in agama-dbus-server). We kept a D-Bus interface because it was needed for internal communication. But the idea is to get rid of it and rely only on HTTP.
  • proxies/locale refers to an interface that is implemented by all Ruby-based services which allows to change the language.

I wonder if we should add default_path = "/org/opensuse/Agama1/Locale", to the 1st proxy, but in my testing system it seems to work (unless I am missing something).

In any case, I will build an image that includes all the changes so we can perform more tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fixed both proxies and now I see no error in the logs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having two interfaces with the same name (org.opensuse.Agama1.Locale) is unfortunate, I know, but they are different things:

It's not unfortunate, it is wrong 😄 but it happens to work now. I hope I can add a simple fix, otherwise postpone it for another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is wrong 😅 The point is that we plan to drop one of them, so it won't be duplicated anymore.

@imobachgs imobachgs changed the title fix(rust): fix the D-Bus manager's LocaleProxy path fix: properly handle installer language changes Nov 13, 2024
@imobachgs imobachgs marked this pull request as ready for review November 13, 2024 11:47
@coveralls
Copy link

coveralls commented Nov 13, 2024

Pull Request Test Coverage Report for Build 11825620593

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 71.326%

Files with Coverage Reduction New Missed Lines %
service/lib/agama/storage/manager.rb 1 97.08%
Totals Coverage Status
Change from base Build 11818318839: 0.0%
Covered Lines: 16895
Relevant Lines: 23687

💛 - Coveralls

it has only a SetLocale method, used as a mixin, to localize the output of
various components,
but it collided with org.opensuse.Agama1.Locale which is the main locale
config for the system
Copy link
Contributor

@mvidner mvidner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My manual language switching test works now
(after installing missing packages to my container, will fix that in another PR)

I can't really review the JS/TS code... it looks well formed to me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants