D-Bus service #54

Open
wants to merge 10 commits into
from

Projects

None yet

7 participants

@jonls
Owner
jonls commented Mar 27, 2014

Implements Redshift as a separate D-Bus service. It must be explicitly activated by passing --enable-dbus to configure and will produce a separate executable that can be automatically activated through the service file. The D-Bus interface is draft and requires more discussion to finalize (spec: https://github.com/jonls/redshift/blob/dbus-service/src/redshift-dbus.c#L196).

Current draft interface

The way the draft interface works is as follows: The properties Temperature, Period ("Day", "Night", "Transition"), and CurrentLatitude and CurrentLongitude can be read (but not written). In addition there is a property called Inhibited which tells whether Redshift is temporarily disabled (also read-only). There is a method called GetElevation which returns the current solar elevation. I am considering that this should maybe also be a property instead of a method. The properties TemperatureDay and TemperatureNight can be used to set the respective color temperatures.

A program that wants to interact with Redshift has to first acquire a cookie (integer) using the AcquireCookie method. This is needed so that two different programs won't step on each others toes. Different programs can acquire cookies, but the following methods put some restrictions on how programs with different cookies can interact with Redshift. There is an associated method ReleaseCookie which invalidates a previously acquired cookie, and undoes all interactions that the cookie has been doing. The method of using a cookie is somewhat similar to the method used by GNOME Screensaver (https://people.gnome.org/~mccann/gnome-screensaver/docs/gnome-screensaver.html#dbus-interface).

The Inhibit method can be used with a cookie to temporarily disable Redshift and it will stay disabled as long as at least one cookie is inhibiting. EnforceLocation can be used (also with a cookie) to change the location. This could be used by an external program to set the location by some method that is not supported by Redshift itself. It could also be used to manually set a location. Only one cookie can enforce the location at a time.

The EnforceTemperature method can be used to force a specific color temperature, so that the user can override the temperature manually or use a program that implements a different color temperature cycle than Redshift. For this method there is a priority parameter which basically means that two cookies can force a temperature at the same time but one will have priority over the other. I think this would be useful to have for a user interface that temporarily shows the desired color temperature when the user moves a slider, and this effect should have priority over the temperature enforcement as described previously. Lastly there are three methods, Uninhibit, UnenforceLocation and UnenforceTemperature which will undo the effect of the previous three methods.

I still haven't though about how a program would interact with the gamma method driver. Perhaps there should be a way to select a driver though the D-Bus interface, set options for driver, and see error messages if a driver fails to work. The location is also not obtained through Geoclue yet, so the only way to use it now is to force a location through the EnforceLocation method. I'm thinking about adding writable properties called FallbackLatitude and FallbackLongitude so that Redshift will use these if Geoclue doesn't work, but I am not sure if that is the best solution. Lastly, there should also be properties for gamma and brightness I think, but these should perhaps be integrated with the gamma method interface so they can be set per screen.

Todo

  • Obtain location from geoclue.
  • Support multiple monitors separately configured(?).
  • Store configuration state persistently (using gsettings?).
@maandree
Contributor

Not sure what you mean with ‘store configuration state’ in the last todo,
but if it is configurations found in redshift.conf I susggest simply modifing
redshift.conf.

Is this implementation able to distinguish users, X displays and
display servers (including the TTY)? Additionally I suggest adding
an environment variable like REDSHIFT_ID which could be used for
example if I have separate redshift instances for my monitors.

@jonls
Owner
jonls commented Mar 27, 2014

Yes, it may be possible to simply use redshift.conf. I think a GUI should be able to make persistent changes (e.g. set night temperature to X even after restart) so I am wondering how to best accomplish that. It could go through the D-Bus interface so the interface would have to be extended to accommodate that and Redshift would have to be able to update the conf file. Since by going in this direction we are now depending on D-Bus and glib anyway, so it may be useful to investigate if some of these dependencies offer solutions already so we don't have to reimplement this. I haven't really looked into all the details of dconf but it seemed to me that it could be an option.

Currently this implementation only has vidmode and randr support. It is running on the D-Bus session bus so it should only apply to the session that it is running in. I think it would be reasonable to add your drm support as well even though logically the TTYs are not contained in the session and are shared with multiple users, but I guess it would be convenient for single-user systems. Ideally I think Redshift should be able to run with both drm and randr (or drm and vidmode) at the same time so that only one instance is necessary. I would also like for Redshift to be able to run with no gamma drivers. This was the purpose of the dummy driver but I think now that it should be implemented slightly differently.

I am not sure I understand completely what use the REDSHIFT_ID would have. I think it is reasonable to have multiple instances running presently but also we should try to improve Redshift so only one instance will be necessary. In any case I think there would be some issues with having the automatic activation through D-Bus work with multiple instances and ids passed in environment variables.

Lastly, I should mention that I plan on keeping the old implementation alive so that people who are not enthusiastic about the dependency on D-Bus and glib can still use redshift from the terminal, but I think for any progress to happen on the GUI side we really need to move to a proper interface that is easy to develop for.

@maandree
Contributor

If Redshift is improved to be able to control the monitors individually (more than just the gamma correction) than REDSHIFT_ID whould not be neccessary. Although implementing this is not too
difficult, as it as been done for the gamma correction in the branch maandree:multimonitor, I am not
sure it is really the right thing to do.

I have reevaluated the advantages of D-Bus versus domain sockets and I think that domain sockets are not sufficient for the future of Redshift.

@jonls
Owner
jonls commented Mar 27, 2014

Yes, I saw that you already did some work in this direction in the multimonitor branch. What do you think is the risk of implementing individual control of monitors like that?

@maandree
Contributor

I am afraid that the change would increase complexity without substantial gain. Currently the only problem with running one instance per monitor is that you would need one redshift-gtk instance per
monitor if you want to have a status icon. However this have a nice property: it is very simple to just
disable it for one of the monitors, which I find very preferable, although I do not use the system tray myself. Implementing a GUI that works on mutiple instance of redshift is not too difficult and REDSHIFT_ID could contain multiple ID:s.

@jonls
Owner
jonls commented Mar 27, 2014

Ok, I agree with your point to a certain extent but having multiple IDs in REDSHIFT_ID would seem to just shift the complexity into the GUI. What do you think about keeping the current implementation around mostly unchanged and then work on getting individual monitor control into redshift-dbus instead since it seems to have a higher importance there?

@maandree
Contributor

I agree that it is just shifting the complexity, but I do think that the over all complexity would be decreased. In any case, only implementing it in redshift-dbus would increase the complexity only more. I think it is best, if not using ID:s, to implement it in both commands and let it be possible to configure the temperature and brightness individually for the monitors in the same way as the gamma correction, and, as with the gamma correction, still allow it to be specified in [redshift] to apply to all monitors that have not hade it specified especially; and then try to separate out the implementation of the D-Bus service from redshift-dbus.c as I did in my proof of concept, ideally redshift-dbus.c and redshift.c would be the same file just one compiled with -DENABLE_DBUS and the other without it.

@maandree
Contributor

Since it should be possible to run both DRM and RandR or vidmode at the same time, I think it makes since to also add support for running in multiple X displays. I started to implement this but I found that it would increase the complexity of gamma-vidmode and especially gamma-randr significantly. So I thought that when you have merged/decided not to merge issues #44, #47 and #49, I could rewrite gamma-*.{c,h} and redshift.c to support running with multiple adjustment methods and multiple displays and reduce code duplication in gamma-*.c when doing so.

@simgunz
simgunz commented Mar 27, 2014

Hi,
I'm really looking forward for the dbus functionality of redshift, because this can simplify a lot the implementation of the kde interface. In fact the current way of interacting with redshift, with the USR signal, is really not enough to support the various features I've implemented in the kde interface, and because of this the code of the data engine I wrote is really complex and unclear.

I'll now try to explain how the kde interface interacts with redshift and the structure of it, and I'll explain why dbus can simplify the implementation.

Currently the kde interface is written in c++ and it is made of a backend (a plasma dataengine) and a frontend a plasmoid. There can exist an infinite number of plasmoids (in the system tray, on the desktop, in different activities ecc.) that don't interfere with each other, because all of them interact with a single data engine that actually controls a single redshift process.

The backend starts a KProcess to launch redshift on a shell, and it launches it with all the parameters every time bypassing so the redshift configuration file in the user home directory.

The frontend is a kde plasmoid that shows an icon of redshift. When the user clicks on the icon, he can turn redshift on and off. When redshift is "on", it changes the color temperature automatically during the day as it is supposed to do. If the user scrolls the wheel on the icon instead, redshift is brought in "manual" mode. When in this mode, redshift sets a fixed temperature and won't change it anymore until the user doesn't click again on the redshift icon to bring back redshift to "auto" mode. When the mouse wheel is scrolled over the icon the temperature can be selected. While scrolling an osd appears showing the current color temperature.

Some users asked me to show the current temperature also in the tooltip balloon that appears when the mouse is hoovered over the redshift icon, but I haven't implemented it yet because I can only know the current temperature when redshift is in manual mode (because I set it). When it's in "auto" mode, with the current implementation of redshift, there is no way to know the current temperature. So if the redshift-dbus implementation will export a property with the current temperature, this can be easily shown in the interface.

The following paragraph explain a kde activity dependent behavior of redshift.
A feature of the redshift kde interface allows redshift to behave in different way according to the current KDE activity.
Redshift can be in the following three states: Always On, Always Off, Auto. As an example the user can have a "Main" activity where it wants redshift to work in "auto" mode and a "Graphics editing" activity where it needs to see the real color of the image he is editing, so he wants redshift always off. When the user switch between the two activities redshift is turned off or auto accordingly.
Currently when redshift is in the state "Always Off" the user can't activate it manually even clicking on the icon (the opposite happens for the "always on" state). I'm not sure this is a smart behavior, maybe it should be "default off" and "default on" in order to let the user activate/deactivate redshift manually if he wants. What do you think?
In any case the dbus method "Inhibit" is useful to put redshift in the always off state.

All the parameters of redshift and the activity based behavior can be edited by the user from a settings page that can be accessed from the right-click menu of the applet. When the user change the parameters the redshift process must be terminated and started again since the parameters can't be hotswapped on the running process. If the smooth transition option is enabled the user will see a transition red-neutro (process terminated) followed by a transition neutro-red (process started). If the dbus implementation will allow the parameters hot swapping the transition can be current color -> new color that will be much more nicer. Moreover if the user edits a parameters and click "Apply" and then he edits another parameters and click "Apply" again before the red-neutro transition is over, the screen become instantly red and a new red - neutro transition begin. This is really annoying.

Another problem I have is that if the user select the autostart option in order to run redshift on the startup of the pc, since I run the redshift process from within the kde backend, it happens that the process is started but the display doesn't turn red. This happens because the kde redshift dataengine, which starts the redshift process, is loaded too early and (I don't know why) redshift can't manage the display correctly. To avoid this problem I created a process called redshift autostartenabler that is started on the kde startup in an usual way. When this program is loaded,it enables the redshift dataengine to start the redshift process, so that the start time of the redshift process happens late in the desktop environment boot. This can be solved with the dbus implementation in the following way: the redshift process is started at the desktop startup (as any auto start program) and reads its parameters from the settings file of redshift, then the dbus interface allows the kde interface to hot swap the new parameters if the user changes them and start.

Currently the location must be set by the user, and the proposed location the first time the kde interface is started is the location grabbed from the kde location settings. I do not understand if redshift can currently get the location from geoclue or not.

As I said the kde interface is written in c++, this is due to the fact that it needs a central dataengine to manage the redshift process.
This implies that the redshift kde interface needs to be compiled on the user pc and packaged for the various distribution of linux. When redshift will support dbus, the kde interface can be rewritten in python and packaged as a *.plasmoid package so that it can be distributed through KGetNewStuff (the user can download any *.plasmoid package hosted on kde-apps.org through the desktop) without compiling it. This will allow an easier way to distribute the kde redshift interface to all the linux distributions.
The python plasmoids can interact with the single redshift process through dbus without interfering with each others.

Dbus should provide:
-export all parameters (also the current calculated color temperature) in order to let other applications to read them
-Hot swapping of all the parameters. Redshift should detect changes in the parameters and change its behavior transparently to the user
-possibility of setting a fixed temperature
-and/or possibility to set the current temperature (e.g. if the default day color temperature is 6000K, but today I'm in a dark office, or the sky is cloudy, I may want to lower the current temperature (5000K), but I still want redshift keep adjusting the color temperature during the day.

@simgunz
simgunz commented Mar 27, 2014

A last thing. Some user asked me to implement a personal schedule for redshift, with multiple temperatures during the day so that a user can decide the hours when the color transition must happen based on his habits. As a reference, this feature have recently implemented in Flux.

@maandree
Contributor

Still reading, but before I forget, I think that if you start redshift with -v you will be get the temperature, brightness and location, it is printed every it is changed, including during transition. Perhaps I am missing something but that seems like enough to get the current colour temperature even when not in manual mode.

@simgunz
simgunz commented Mar 27, 2014

Thanks. I never used the -v option, and I didn't noticed it.

@TingPing
Contributor

Since by going in this direction we are now depending on D-Bus and glib anyway, so it may be useful to investigate if some of these dependencies offer solutions already so we don't have to reimplement this. I haven't really looked into all the details of dconf but it seemed to me that it could be an option.

@jonls GSettings should do and be cross-platform (in theory).

@jonls
Owner
jonls commented Apr 2, 2014

Thanks for the comments @simgunz. I think the draft interface will take care of the features that you need reasonably well. Recently redshift-gtk was extended to parse the -v output from redshift to obtain the current temperature so you may want to look at the code if you want to support that before the D-Bus support is ready.

added some commits Dec 9, 2013
@jonls Add DBus service
Add a seperate executable redshift-dbus that functions like redshift but runs
the main loop through Glib. In addition it registers a service on the session
DBus such that it can be queried and controlled.
ab43258
@jonls Update RPM spec to build dbus service 4eed993
@jonls redshift-gtk: Adapt to use DBus service c37add1
@jonls redshit-dbus: Get location from Geoclue
Try to obtain location on program start and register a signal so that
the position will be updated with new information from Geoclue.
242176a
@jonls redshift-dbus: Use proper Glib message output functions da392d7
@jonls Fix: redshift-dbus failure when starting at boot 708aab3
@jonls redshift-dbus: Save and restore known position for next startup
When a position is obtained from Geoclue it is saved to
`$XDG_DATA_HOME/redshift/position`. This file is loaded on program start to
find the initial position. If the file does not exist the default position
of 0,0 is used until the correct position can be found.
4b6e647
@jonls
Owner
jonls commented May 8, 2014

Obtaining position from Geoclue is implemented now.

@jonls
Owner
jonls commented May 9, 2014

As mentioned in #80 the Geoclue code causes a segfault on startup. Since the old Geoclue is no longer maintained it will be necessary to move to Geoclue2 and hope it is less broken.

@ghost
ghost commented May 13, 2014

Some comments on the API interface. Having a cookie with inhibit/uninhibit is fine, as there are only 2 possible states so the logic is straight-forward and the behaviour is predictable for clients.

However, using the same system for (Un)EnforceLocation and (Un)EnforceTemperature seems over-complex, and I don't see the benefit. If multiple clients attempt to (for example) call EnforceLocation, redshift would need to store the previous location value for each EnforceLocation call, and would need to have a non-trivial algorithm for deciding which location to revert to when UnEnforceLocation is called. A scenario:

Imagine we have locations (A, B, C) and clients (X,Y).
Initial location is A.
Client X calls EnforceLocation(B) and gets cookie X'.
Client Y calls EnforceLocation(C) and gets cookie Y'.
Client X calls UnEnforceLocation(X'). What do we do here? We can't set the location to A without violating Client Y's contract. Therefore we have to leave it at C. At this point the user might be confused, because they just requested that something revert, but nothing happened.
Client Y calls UnEnforceLocation(Y'). In this case, we can't revert to the location that was set when Y' was issued, as that location is no longer enforced. Therefore we have to revert the location to A. At this point the user might be confused, because the reversion resulted in a different location than was set when they called EnforceLocation.

It is possible to modify how this algorithm works, but I would argue that all possible implementations are potentially confusing.

Instead of this, I recommend simply having simple Get/SetLocation and Get/SetTemperature methods. If you wish to implement something like a 'try it and see' temperature slider, it would be up to the client to retrieve and store the initial temperature, and if the user cancels the client would call SetTemperature with the original value. This does introduce the possibility of overwriting a temperature that another client set in the meantime, but I argue that this is no more confusing than the scenario outlined earlier. The implementation is simpler, and the user has a greater chance of building a mental model of what is actually occurring.

The temperature slider question is also interesting. If the same client calls EnforceTemperature multiple times (each time the slider is moved), that client would be issued a new cookie for every EnforceTemperature call. This seems unwieldy.

Edit: I have thought about this some more, and the scenarios that arise using Get/SetLocation and Get/SetTemperature seem complicated too. Using Get/SetLocation and Get/SetTemperature would essentially over-ride the automatic setting of the location and temperature, so a mechanism to reverse the over-ride is needed. Which gets us back to the cookie-based approach. So in summary, the cookie based approach seems OK, but I'm curious how it would work if the user was operating a temperature slider. Perhaps you need an UpdateEnforcedTemperature/UpdateEnforcedLocation call that takes the cookie as an in parameter.

@ghost Unknown commented on the diff May 13, 2014
Makefile.am
@@ -33,6 +33,9 @@ SYSTEMD_USER_UNIT_IN_FILES = \
data/systemd/redshift.service.in \
data/systemd/redshift-gtk.service.in
+DBUS_SERVICE_IN_FILES = \
+ data/dbus-1/services/dk.jonls.redshift.Redshift.service.in
@ghost
ghost May 13, 2014

I'm not sure about the capitalisation of the second Redshift in the service file name. Nor the repetition of redshift. Why not just:
data/dbus-1/services/dk.jonls.redshift.service.in

@jonls
jonls May 15, 2014 Owner

I believe the file name here should match the well-known bus name of the Redshift service. The D-Bus spec provides an example where the well-known bus name is the same as the main interface name, and it seems that many other projects follow that convention so I copied that. The interface name is composed of a reverse-DNS part (dk.jonls.redshift) and a name (Redshift).

@jonls
Owner
jonls commented May 15, 2014

@kimdouglasmason Here's how the cookie mechanism works: The client first calls AcquireCookie so there is no new cookie given out on each new call to EnforceTemperature. EnforceTemperature essentially works as the UpdateEnforcedTemperature that you proposed.

I agree with you that there can be some confusion if multiple programs are trying to control redshift at the same time and I think users should be encouraged to only use one redshift GUI at a time.

The problem with EnforceLocation that you describe is solved by only allowing one program to enforce the location at a time. I think that is the only reasonable solution to avoid confusing the user. The second program that tries to control the location will simply fail with an error that there is already another program enforcing the temperature.

@ghost Unknown commented on the diff May 20, 2014
src/redshift-dbus.c
+
+/* Forced location */
+static gint32 forced_location_cookie = 0;
+static gdouble forced_lat = 0.0;
+static gdouble forced_lon = 0.0;
+
+/* Screen update timer */
+static guint screen_update_timer = 0;
+
+/* Geoclue proxy objects */
+static GDBusProxy *geoclue_manager = NULL;
+static GDBusProxy *geoclue_client = NULL;
+
+
+/* DBus service definition */
+static const gchar introspection_xml[] =
@ghost
ghost May 20, 2014

It might be cleaner to put this XML into an external file.

@ghost Unknown commented on the diff May 21, 2014
src/redshift-dbus.c
+ PERIOD_DAY,
+ PERIOD_NIGHT,
+ PERIOD_TRANSITION
+} period_t;
+
+static const gchar *period_names[] = {
+ "None", "Day", "Night", "Transition"
+};
+
+
+static GDBusNodeInfo *introspection_data = NULL;
+
+/* Cookies for programs wanting to interact though
+ the DBus service. */
+static GHashTable *cookies = NULL;
+static gint32 next_cookie = 1;
@ghost
ghost May 21, 2014

Why not make this a guint32? Cookies have no need to be negative, plus overflow behaviour for signed integers is undefined in C.

Not that it's a real problem... I couldn't imagine the system every issuing 2.1 billion cookies :).

@ghost
ghost commented May 21, 2014

In order to detect a client process terminating without releasing its cookies, you should be checking the NameOwnerChanged signal of org.freedesktop.DBus . As far as I can see, if a client process acquires a cookie and enforces the temperature, if it terminates then that temperature will remain enforced.

@xtaran
xtaran commented Aug 1, 2014

Redshift was such a nice and slim program. It did one job and it did it well, being a good example of the KISS (keep it simple & stupid) principle. Why adding more and more unnecessary bloat?

@maandree
Contributor
maandree commented Aug 1, 2014

People want to to things like have changing the temperature from a GUI.
Personally I rather see redshift add IPC that to have its own GUI.
This will also allow you to do things that is not built into to redshift, so
I think that this is the best solution.

@jonls
Owner
jonls commented Aug 2, 2014

@kimdouglasmason Hey sorry for the late reply and thanks for the suggestions. I agree that your idea of using NameChangeOwner is superior, I was not aware that this was possible when I wrote it.

@flying-sheep

D-Bus is extremely useful and will enable us to write GUIs without depending on compiling them in.

For everyone wanting the extremely minimal version, there are always compile time switches.

But why does this pull request depend on GLib? There is a plain C D-Bus API that we could use!

@jonls
Owner
jonls commented Nov 3, 2014

@flying-sheep The short answer is that the glib interface is much easier to use. As far as I know very few service providers actually use the C D-bus API. The first line in the documentation literally says "This manual documents the low-level D-Bus C API. If you use this low-level API directly, you're signing up for some pain".

@flying-sheep

i was hoping that we could create a minimal, framework-agnostic backend that can be either started via commandline with options or as a service, and has no runtime dependencies (like a GLib event loop)

when started via commandline, it just runs there in some terminal until killed or suspended via pkill -USR1 redshift. (for this use case it could be compiled without D-Bus)

when started as service, it would be most useful when controlled by a toolkit-dependent frontend via D-Bus. (since this is still very minimal, it could be the default compile options and the version to be included in linux distributions)

like it is now is still OK, i mean, most people have GLib anyway, and the rest can still be achieved.

@flying-sheep

so what’s going on with this? it would be very useful for everyone, no matter the implementation

@pik
pik commented May 3, 2015

I agree that the Enforce functionality can be a little weird if commands are going to fail silently - but reasonably they should be able to receive and print the error messages from dbus. For most users purposes I think enforce would not be necessary.

Perhaps the following behaviour would be reasonable from a user-perspective: Redshift-gtk connects to the dbus-service and settings are set according to the config-file or gui-wizard. If another program or command-line command changes the settings, the default behaviour is for Redshift-gtk to increment it's displayed profile (say from "profile1" to a temp-file-name, which shows the respective changes which are not part of the standard profile).
The gui view can then offer additional options - restore normal "profile1" state as well as enforce "profile1" state (ignoring third-party changes, if it can grab the cookie), or save the new changes (either over-riding "profile1") or with a new profile-name.

multimonitor branch.

Imho a single daemon/instance with different settings by monitor-name/number would be a much neater structure than multiple instances per monitor if running with --enable-dbus, it sounds like it might be tricky to get it to play nice for the non-dbus version though.

@jonls jonls referenced this pull request Jun 20, 2015
Closed

Current values #230

@flying-sheep

uh, still nothing? this is the #️⃣1️⃣ feature i’m waiting for 😞

This was referenced Jan 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment