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

dbus synopsis #1134

Closed
beku opened this issue Nov 23, 2016 · 28 comments
Closed

dbus synopsis #1134

beku opened this issue Nov 23, 2016 · 28 comments

Comments

@beku
Copy link
Member

beku commented Nov 23, 2016

The dbus readme gives an example.
However, it is unclear for me on how to adapt to our needs.

What I want:

  • sync client views of settings across a user desktop session
  • obtain messages for key changes in our name space (org/freedesktop/openicc/...)
  • use Qt5DBus for Qt based GUI: Synnefo
  • observe in FLTK, (even not sure what is the best way there.)
@markus2330
Copy link
Contributor

Hi Kai-Uwe, thanks for reporting this issue.

@waht is working on this topic and hopefully will write some nice tutorials to help you with these questions when he is ready.

You requirements are quite straight-forward and I do not think we will have any issues in supporting them. Just out of interest: Is it also a requirement for you that dbus is used or are other bus systems okay, too?

In https://github.com/ElektraInitiative/libelektra/blob/master/src/plugins/dbus/receivemessage.c you also see how to receive a message in C.
@waht also looks into the issues of main/event loop integration.

But it is an ongoing topic and there are some issues: #955, #404 but the basic things should work.

@beku
Copy link
Member Author

beku commented Nov 23, 2016

Oh. Thanks.

What I am looking for is more specifically the mount dbus key_path synopsis.

dbus-monitor type='signal',interface='org.libelektra',path='/org/libelektra/configuration' # gives no messages atm

@markus2330
Copy link
Contributor

markus2330 commented Nov 23, 2016

You got the synopsis right and it works for me.

kdb mount file.dump /dbus dump dbus
kdb set user/dbus/y a  # should fire in session bus
kdb set system/dbus/y   # should fire in system bus

For me both dbus-monitor and the C code works:

dbus-monitor --system type='signal',interface='org.libelektra',path='/org/libelektra/configuration'
#> signal sender=:1.100875 -> dest=(null destination) serial=2 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyAdded
#>   string "user/dbus/x"
kdb testmod_dbus receive_system 
#> Notify received

Maybe you only tested with system and forgot to add --system? I clarified the docu.

markus2330 pushed a commit that referenced this issue Nov 23, 2016
@beku
Copy link
Member Author

beku commented Nov 23, 2016

Following appears not to mount successfully (appreciate a correct mount line ;)

kdb mount file.yajl org/freedesktop/openicc yajl dbus

mount-openicc says:

$KDB mount --resolver=resolver_fm_xhp_x color/settings/openicc.json /org/freedesktop/openicc yajl rename cut=org/freedesktop/openicc

expected is a DBus message from :

kdb set user/org/freedesktop/openicc/behaviour/effect_switch 1

@beku
Copy link
Member Author

beku commented Nov 23, 2016

I assume, the dbus plugin needs to be mounted to each file plugin, e.g. dump or yajl etc for obtaining dbus messages.

@markus2330
Copy link
Contributor

markus2330 commented Nov 23, 2016

Yes, mount-openicc does not have dbus included, you simply need to add "dbus" somewhere after the mountpoint, e.g. as last word.

Alternatively (and recommended) you could globally mount dbus:

kdb global-mount dbus

In either way you should receive a dbus message with your kdb set!

@beku
Copy link
Member Author

beku commented Nov 23, 2016

kdb global-mount dbus

helped. Thanks

Now the messages get sent. However for yajl, the whole DB seems to get erased (~20 delete + add lines)

signal sender=:1.93 -> dest=(null destination) serial=4994 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyDeleted
    string "user/org/freedesktop/openicc/org/freedesktop/openicc/profile/editing_gray"

Will try to get around the message flood.

@markus2330
Copy link
Contributor

Will try to get around the message flood.

This is a issue in combination with json and/or rename, let us keep the issue open to fix this. (Might be related to #404)

Are you interested in every single key change or would it be enough to get a single message that something in the backend changed? This was our previous behavior, @machinekoder from machinekit.io changed it in #385. But we can easily add a flag to have both behaviour (suppress individual key changes).

Avoiding problematic updates (that can occur when an application reloads multiple time before the other application is finished updating) is the main goal of what @waht is looking into. Maybe he will come up with a better solution anyway.

Btw. does the system/session bus separation (you were initially confused by) make any sense?

@machinekoder
Copy link
Contributor

A single message for multiple changes would be suitable for my use-case too.

@beku
Copy link
Member Author

beku commented Nov 23, 2016

Well, the many yajl messages for one key change case could be improved. However it is generally good application behaviour to go relaxed with too many messages. I will put a shortest update interval somewhere in the GUI apps.

@beku
Copy link
Member Author

beku commented Nov 24, 2016

Here a small example for QDBusConnection:

Place this in your Qt class header:

public slots:
  void configChanged( QString msg );

put this in your Qt class, e.g. the constructor

if( QDBusConnection::sessionBus().connect( QString(), "/org/libelektra/configuration", "org.libelektra", QString(),
                                       this, SLOT( configChanged( QString ) )) )
    fprintf(stderr, "=================== Done connect\n" );

Here comes the org.libelektra signals:

void SynnefoApp::configChanged( QString msg )
{
  fprintf( stdout, "config changed: %s\n", msg.toLocal8Bit().data() );
};

markus2330 pushed a commit that referenced this issue Nov 24, 2016
@markus2330
Copy link
Contributor

Thank you, I added your example!

Well, the many yajl messages for one key change case could be improved.

If we go back to a single message per backend-change these bugs would go away anyway.

The idea of #385 was to directly have the information of individual key changes and directly update configuration values. Something like #828, however, might to be superior anyway. (Every application calculates what changed on their own.)

From my side it would be okay to do a small release with the bug fixes we added recently and with changes you need for openicc?

So: should we change back to a single message per default and have every change as configuration option or remove this functionality completely?

@beku
Copy link
Member Author

beku commented Nov 24, 2016

Synnefo and KDE's KolorManager are now pinged from the org.libelektra DBus interface for default profile, policy, settings and device profile changes :-D

A huge thanks to the libelektra developers, who built a system, which takes off the work from a merely API user.

(At the moment the apps do not parse the message details. They just clear the caches, reload the data models and update the views.)

@beku
Copy link
Member Author

beku commented Nov 24, 2016

@markus2330

So: should we change back to a single message per default and have every change as configuration option or remove this functionality completely?

My implementation just sees the first message, waits for a quarter of a second and then do update after the storm. That is working fine. No wish from my side ATM. The messages are some thousands, which can not be processed efficiently and get ignored.

@markus2330
Copy link
Contributor

The messages are some thousands, which can not be processed efficiently.

Do you mean "If they would be some thousands" or are they actually some thousands? How large are these files?

@beku
Copy link
Member Author

beku commented Nov 24, 2016

The messages are some thousands.

signal sender=:1.93 -> dest=(null destination) serial=79042 path=/org/libelektra/configuration; interface=org.libelektra; member=KeyDeleted
   string "user/org/freedesktop/openicc/org/freedesktop/openicc/profile/assumed_rgb"

Above a example message. It is the 79042 nth from a few set calls to libelektra.

@markus2330
Copy link
Contributor

Thank you, sounds like a very interesting setup! Can you point @waht to the code (once available) so that he can analyse it?

@beku
Copy link
Member Author

beku commented Nov 24, 2016

@waht Below is the link to the commit for Synnefo (just round the corner ;-)
oyranos-cms/Synnefo@103a3a9

The commit covers the three GUI modules, which can run separately - Settings, Devices and Information. In Synnefo and KM they are part of one GUI. Previously was code there in to poll for config changes. But it was eating CPU resources :-/ You can see it removed and replaced with the DBus callback. Basically the first DBus message blocks further messages by setting acceptDBusUpdate = false; Then a QTimer::singleShot(250, this, SLOT( update() )); is set up. The MyClass::update() function does the data model read in + GUI update. Then it sets acceptDBusUpdate = true; and new events can come in.

As KolorManager runs on top of libOyranosSynnefo, libelektra DBus support is used as well in KDE.

Btw. it would IMHO make qt-gui more useable being a realtime viewer. And of course I like the app. As well the dbus plugin becomes more attention by you Elektra developers and possibly packagers too.

@markus2330
Copy link
Contributor

Thanks for the info!

make qt-gui more useable being a realtime viewer. And of course I like the app.

Yes, and according to your patch adding support for doing so is really quite easy! Maybe @0003088 finds some time to do it?

@0003088
Copy link
Contributor

0003088 commented Nov 24, 2016

I will have a look at it but I cannot promise any delivery date.

@0003088
Copy link
Contributor

0003088 commented Nov 28, 2016

@markus2330 could you give me a hint how I would be able to test the functionality? Are there test cases or something?

@markus2330
Copy link
Contributor

The easiest way is that you actually kdb global-mount dbus and do kdb set user/guitest/something. Then qt-gui should get a dbus signal and reload the config.

The blog post of @beku gives you information of how to implement the signal/slots http://www.oyranos.org/2016/11/watching-org-libelektra-with-qt/

And src/plugins/dbus/README.md gives extensive information about how to trigger and check for dbus signals.

If there are any further questions do not hesitate to ask.

@0003088
Copy link
Contributor

0003088 commented Nov 29, 2016

Hmm, it's not that simple I'm afraid (not the connecting/receiving part, that works fine) but how to react to the signal. Due to the internal structure of the gui (a second hierarchy that's basically independent from the 'system' hierarchy until synchronized) I need to synchronize and refresh the view to reflect the change in kdb. If I change a key in the gui without synchronizing and then the same key in kdb I receive an error "Ours: CONFLICT_MODIFY, Theirs CONFLICT_MODIFY" and the change is not visible in the gui, only after a restart of the gui. It only works if the keys in the gui are not changed or if changes in the gui have been synchronized.

@markus2330
Copy link
Contributor

markus2330 commented Nov 29, 2016

It only works if the keys in the gui are not changed or if changes in the gui have been synchronized.

I did not expect otherwise and I think that is okay. Can you add a option to toggle between these behaviours? (something like notification/sync).

A related interesting feature could be a autosync mode, i.e., sync after every edit in the editor.

Per default I would suggest to only sync on explicit request (as it is now). (EDIT: And in this mode we expect the user to use the gui as dbus viewer only)

@0003088
Copy link
Contributor

0003088 commented Dec 1, 2016

I have put a checkbox in the settings menu, if it is checked the sync button as well as all edit commands are disabled (it should be possible to hide all menus and buttons, maybe that would be even better?) Is that what you thought of? It is unchecked by default on every start (do you think it's better if the gui remembers the last setting?) But I'm really unsure regarding terminology. How to name the button. To make the current the mode visible I thought of putting it next to the editor name in the titlebar, like "Elektra Editor (DBus Mode)" or something like that?

@markus2330
Copy link
Contributor

I have put a checkbox in the settings menu, if it is checked the sync button as well as all edit commands are disabled (it should be possible to hide all menus and buttons, maybe that would be even better?) Is that what you thought of?

I think this is what @beku describes, and customers obviously come first!

I thought more of doing synchronizing all the time (every time the user edits something and always when something in the outside world happens) to minimize the chance of synchronization conflicts but still allow config changes. Obviously, this would be a much more dangerous mode. For Example: Would undo still work when some of the changes actually are done from outside? So maybe we are better off with a separated viewer/editor (as you did now) anyway.

Do you think it's better if the gui remembers the last setting?

Yes, I think this could be useful. Seems like @beku uses it more as a viewer. (@beku please correct me if I am wrong)

To make the current the mode visible I thought of putting it next to the editor name in the titlebar, like "Elektra Editor (DBus Mode)" or something like that?

Maybe "Viewer Mode"?

I am looking forward to a PR!

@beku
Copy link
Member Author

beku commented Dec 1, 2016

Seems like @beku uses it more as a viewer.

Synnefo is a live settings GUI. The UI applies changes immediately. Circular signals where initially a problem. Now when the data model changes, any signal/callback is ignored until the view update is finished.

Undo for qt-gui is interesting, especially if presented as time line. A undo implementation needs perhaps a notion of state. The state snapshot could be triggered by UI and by external events. (I do not plan so for Synnefo, as it would be overkill there.)

@markus2330 markus2330 mentioned this issue Jan 21, 2017
5 tasks
@markus2330
Copy link
Contributor

Thanks for all the great input! I think we can close the issue now!

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

No branches or pull requests

5 participants