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

Three D-Bus services #538

Merged
merged 4 commits into from
Feb 5, 2020
Merged

Three D-Bus services #538

merged 4 commits into from
Feb 5, 2020

Conversation

ederag
Copy link
Collaborator

@ederag ederag commented Feb 3, 2020

This PR is a different proposition for #533, without the shortcomings of #536.

The old WindowServer is kept, except internally it just launches hamster <window name>.
It seems to work well.

The Gtk.Application must then have a different name.
For now, it is org.gnome.Hamster.GtkApp
Not org.gnome.Hamster.GtkApplication to make it clear it is a custom name.
Not a more generic name such as org.gnome.Hamster.Windows as the actions
can only be accessed through a org.gtk.Actions interface anyway ?

Copy link
Contributor

@bochecha bochecha left a comment

Choose a reason for hiding this comment

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

Not a more generic name such as org.gnome.Hamster.Windows

I have to say, I was convinced this "windows service" was something used when installing Hamster on Windows rather than Linux. I didn't expect it to be something related to displaying app windows. 😅

For now, it is org.gnome.Hamster.GtkApp
Not org.gnome.Hamster.GtkApplication to make it clear it is a custom name.

In general, apps would just use a name like org.gnome.Hamster. I've seen the equivalent of org.gnome.Hamster.Application in other apps.

as the actions can only be accessed through a org.gtk.Actions interface anyway ?

I don't think there's any relationship between the name of your service and the interfaces it implements?

@@ -27,7 +27,7 @@
</ul>
</description>

<launchable type="desktop-id">hamster.desktop</launchable>
<launchable type="desktop-id">org.gnome.Hamster.GtkApp.desktop</launchable>
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of flatpaking Hamster, and renaming the desktop file (and other resources as required by Flatpak) is something I was going to do.

So a big +1 for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I knew you were on to something good !

source= "org.gnome.hamster.service.in",
target= "org.gnome.hamster.service",
source= "org.gnome.Hamster.service.in",
target= "org.gnome.Hamster.service",
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't mind, I'd like to be sure I understand the difference between the org.gnome.Hamster and org.gnome.Hamster.GtkApp (currently called org.gnome.Hamster.Windows) services.

Because what they do might have an impact on the naming to follow if we want Hamster to work properly inside Flatpak. (which I'd very much want)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

org.gnome.Hamster is the database server:

bus_name = dbus.service.BusName("org.gnome.Hamster", bus=self.bus)

org.gnome.Hamster.WindowServer is the legacy application windows server:

bus_name = dbus.service.BusName("org.gnome.Hamster.WindowServer", bus=self.bus)

Copy link
Contributor

Choose a reason for hiding this comment

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

Be aware that this "little" change with "hamster" becoming "Hamster" can cause quite some headaches for users already having those files.
If users start complaining about services not starting any more for strange reasons, they need to perform a "sudo rm /usr/share/dbus-1/services/org.gnome.hamster.*" to remove the old files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the warning,
but beware that postings on already merged issues are hard to remember.

So dbus launch is not case sensitive ?
Anyway there have been several other issues with old install remnants.
Warning set on the release page, in the wiki Troubleshooting section,
and updated README (ab5e9ea).

Copy link
Contributor

Choose a reason for hiding this comment

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

So dbus launch is not case sensitive ?

It is.

It's just that if people installed Hamster manually (i.e not with a package) then the old service files will be kept on the system, in parallel to the old ones, when those users updated their Hamster installation by simply running waf install from a source tree.

Given that you also renamed the desktop files, the old desktop files would also linger on the system in the same way.

That has the potential to cause lots of confusion.

Personally, I'm of the opinion that people who compile/install their software manually from sources, without going through the safer packaging routes, are expected to be capable of dealing with this kind of problems, and the notice you added to the wiki/readme seems perfectly sufficient to me.

Side note:

because to comply with freedesktop specifications some files are camel-cased now.

The spec mandates a few things like the dotted-notation, the absence of hyphens (-) and that each component starts with a letter. It does not mandate capitalizing the first letter of the last component. This is merely the emerging convention, but lots of apps use lower-case and that doesn't cause any problem and never should. I assume you did that on my suggestion, so I apologize for not having made that sufficiently clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

... the notice you added to the wiki/readme seems perfectly sufficient to me.

Thanks for the discussion, it's reassuring.

I assume you did that on my suggestion

Partly, but your suggestions were perfectly inline with both
the freedesktop recommendations and the flatpak conventions.
Let's get to the bottom of it in #547.
If we can, it would be much better to make all file changes in one go.

@ederag
Copy link
Collaborator Author

ederag commented Feb 4, 2020

In general, apps would just use a name like org.gnome.Hamster.

Indeed, that is the gnome recommendation.
Unfortunately, that clashes with the database server.
And the shell-extension relies on that name.

Maybe we should transition from org.gnome.Hamster
to org.gnome.Hamster.DatabaseServer.
We could create an option for the name,
and create two services calling the same script with each name.
(We would get back to hard-coding later, as simplicity is good)

I've seen the equivalent of org.gnome.Hamster.Application in other apps.

That would be good as well. But application is a

program or group of programs designed for end users

So that would encompass the command line interface as well.

I don't think there's any relationship between the name of your service and the interfaces it implements?

And from the D-Bus API Design Guidelines

Type information is never encoded in the parameter name (i.e. not Hungarian notation)

GtkApp just made it clear it was GUI stuff.
Maybe that should be org.gnome.Hamster.GUI ?

@ederag
Copy link
Collaborator Author

ederag commented Feb 4, 2020

On second thoughts, could we have different services accessing the same database ? Seems risky.
So one could be a wrapper for the other. Need to think.

@bochecha
Copy link
Contributor

bochecha commented Feb 4, 2020

On second thoughts, could we have different services accessing the same database ?

Don't: https://sqlite.org/faq.html#q5


So, what I'm understanding is that Hamster has an app (which is also a DBus service) and a database server. (which is a second DBus service)

Flatpak is quite strict in enforcing some things which used to only be conventions:

  • the "app id" is a valid DBus name
  • only icons with a name prefixed by the app id will be exported outside the sandbox and thus be visible to the app launchers (e.g menus on the desktop)
  • only DBus services with names prefixed by the app id are exported and thus callable outside the sandbox (e.g to DBus activate an app, or for a GNOME Shell extension to talk to the DB server)
  • same for desktop files, appdata files, etc…

(Flatpak allows escape hatches for a lot of the above, but it makes life much more annoying)

For the case we're looking at right now, that means all the DBus services must have a common prefix and that prefix should be the Flatpak app id.

With all that in mind, my recommendation would be to have:

  • org.gnome.Hamster as the Flatpak app id;
  • org.gnome.Hamster as the GtkApplication app id;
  • org.gnome.Hamster as the DBus name for the main app/service;
  • org.gnome.Hamster.Database (or something like that) for the database service;
  • org.gnome.Hamster.desktop as the launcher;
  • org.gnome.Hamster.{png,svg} for the main app icon (the one used in the launcher);

The transition for the DB server would be a bit of a pain though. 🙁

Another possibility (and do keep in mind I don't know much about the project or its codebase, which means I have no idea how feasible/desirable this is, I'm just a random person on the Internet 😜) would be to merge everything into a single org.gnome.Hamster GtkApplication, which provides a DBus service with the same name, and the APIs for both what is currently the app and the database server. This could help preserve compatibility with the GNOME Shell extension (and other third-party tools you might know about or not) and simplify quite a few things.

This is similar to how GNOME Calendar works for example:

  • it has a single DBus service, which takes the org.gnome.Calendar name on the user bus;
  • it provides 2 object paths:
    • /org/gnome/Calendar for the app window and its actions;
    • /org/gnome/Calendar/SearchProvider for the GNOME Shell search provider;
  • each of the object paths implements various interfaces like org.gtk.Actions or org.gnome.Shell.SearchProvider2;
  • the app can be running in the background (with --gapplication-service) to provide the GNOME Shell search feature, or show its window when activated, etc…

tl;dr: the most important thing is that all DBus names share a common prefix, and that prefix must be the Flatpak app id; things work much better when the Flatpak app id is also the GtkApplication appid, the desktop file name, the icon names, …

Whatever you do, please please please do not end the app id / DBus name with .desktop. (only the desktop file launcher should have that as its extension)

@ederag
Copy link
Collaborator Author

ederag commented Feb 4, 2020

Thanks for the discussion.

The second layout (same id, different paths) is interesting,
but that would tie the database server into the Gtk application.
There are pros and important cons to that, too late to do that right now.

The plan is to release 3.0.beta this week-end and 3.0.rc1 the next one.
If by then someone packages for ubuntu 20.04LTS (10 years, so it is important),
then it everything settles and 3.0 is out. There was hard work to give it a chance.
Otherwise we can delay and think about something better.

The first layout would make sense for a new application,
but no way right now; stability is too important.

One possibility: consider the database server as the first component
(which it is, historically and fundamentally; the GUI could be secondary):

  • org.gnome.Hamster as the Flatpak app id;
  • org.gnome.Hamster as the database server app id.
  • org.gnome.Hamster as the DBus name for the main service (database server);
  • org.gnome.Hamster.GUI (or something like that) for the GtkApplication app id;
  • org.gnome.Hamster.GUI.desktop as the launcher;

org.gnome.Hamster.{png,svg} for the main app icon (the one used in the launcher);

Sorry, that might break, too close to release.

please do not end the app id / DBus name with .desktop

Sure.

@ederag
Copy link
Collaborator Author

ederag commented Feb 5, 2020

org.gnome.Hamster.{png,svg} for the main app icon (the one used in the launcher);

@bochecha OK, on second thoughts, that's doable,
if this really helps, please file a PR.
Merging this one (that's the last proposition above) for now.

@ederag ederag merged commit b3cf2c5 into projecthamster:master Feb 5, 2020
@ederag ederag deleted the three-services branch February 5, 2020 20:32
@ederag ederag changed the title WIP: Three D-Bus services Three D-Bus services Feb 18, 2020
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.

3 participants