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

Use pure JS node-dbus ? #6

Closed
YurySolovyov opened this Issue Aug 12, 2016 · 26 comments

Comments

Projects
None yet
4 participants
@YurySolovyov
Copy link
Collaborator

commented Aug 12, 2016

If all required features are there, I think it would be nice to switch into pure JS version to improve portability.

@emersion

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2016

You mean, switch to https://github.com/sidorares/node-dbus?

Yeah, that would be much better. What's more, the currently used dbus implementation makes fixing bugs like #1 very difficult.

@emersion

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2016

@YurySolovyov

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 12, 2016

I'd looked into this, but I have 0 experience with dbus and this repo does not have any tests, so I either need some guidance or need to add some tests

@emersion

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2016

I think the first thing to do would be to describe the org.mpris.MediaPlayer2 interface with an object like this: https://github.com/sidorares/node-dbus/blob/master/examples/service/server2.js#L15

Letters used are data types (yeah, it kind of sucks), here is the spec: https://dbus.freedesktop.org/doc/dbus-specification.html#type-system

It seems that for methods, the last item in the types list is the return value. All others are arguments.

Then, we'll need to implement org.mpris.MediaPlayer2 with JS functions. d-feet is a great tool to debug this.

Tests are a very good idea, but I don't really know how to implement them. Created an issue for that: #7

@YurySolovyov

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 13, 2016

actually, that module is not purely native too, instead, it uses abstract-socket as optional dependency (or claims to do so).

see sidorares/dbus-native#92

@acrisci

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

Yeah looked over the code and what it would take to patch the current dbus library, and I think it's easier just to write it out. Working on this now.

@acrisci

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

I found a bug in 0.2.5 of dbus-native that won't handle functions that return nothing, but it's fixed in the master branch.

@acrisci

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

Now I have found that the org.freedesktop.DBus.Properties interface is not fully implemented and not overridable in the dbus-native project which is a big problem.

@emersion

This comment has been minimized.

Copy link
Collaborator

commented Oct 23, 2018

Thanks for looking into this! Indeed we do need a full org.freedesktop.DBus.Properties implementation, do you know if a big piece of it is missing or if it's easy to add it?

@acrisci

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

There's an internal implementation of the properties interface that's different from the exported interface implementation. There's no way to emit events on the internal implementation, so we can't emit PropertiesChanged which is required to make playerctl work.

The Set method of properties is not implemented.

There's no way to make a property readonly either.

I'll do some probing PRs and see if the maintainer has a will to make this work.

@emersion

This comment has been minimized.

Copy link
Collaborator

commented Oct 23, 2018

Eh, these are pretty severe limitations. :\

@acrisci

This comment has been minimized.

Copy link
Member

commented Oct 24, 2018

Another problem with dbus-native is it has a very unusual and undocumented variant object format you'd need to write a converter for.

The current library node-dbus is no better. It completely ignores the types of the properties. There is no way to represent "1.0" as anything other than a byte which is needed for volume and rate.

I'm beginning to think the best way to fix this is to just write your own dbus service in C because there's not a good library for writing the services in node.

@acrisci

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

Hey, I'm redesigning the dbus-native service interface so it can work for this project.

https://github.com/acrisci/node-dbus-next

See a working example here.

Let me know what you think.

@emersion

This comment has been minimized.

Copy link
Collaborator

commented Nov 5, 2018

Looks great! Keep up the good work!

acrisci added a commit to acrisci/mpris-service that referenced this issue Nov 8, 2018

Migrate to dbus-next
Migration to dbus-next (a fork of dbus-native) is a rewrite, but fixes
some outstanding bugs in the project related to variant types.

The project is now transpiled to use the experimental decorator feature
that will be available in the language at some later time. gulpfile.js
contains build instructions. Build with `npm run build`. The dist/
folder contains what will be published on npm.

Interfaces are implemented as classes with decorators specifying the
properties of the member that is exported on the bus.

Update examples and add a new tracklist example.

Other bugfixes may have been a side effect of the rewrite.

fixes dbusjs#1
fixes dbusjs#6
fixes dbusjs#13

acrisci added a commit to acrisci/mpris-service that referenced this issue Nov 8, 2018

Migrate to dbus-next
Migration to dbus-next (a fork of dbus-native) is a rewrite, but fixes
some outstanding bugs in the project related to variant types.

The project is now transpiled to use the experimental decorator feature
that will be available in the language at some later time. gulpfile.js
contains build instructions. Build with `npm run build`. The dist/
folder contains what will be published on npm.

Interfaces are implemented as classes with decorators specifying the
properties of the member that is exported on the bus.

Update examples and add a new tracklist example.

Other bugfixes may have been a side effect of the rewrite.

fixes dbusjs#1
fixes dbusjs#6
fixes dbusjs#13

@emersion emersion closed this in #15 Mar 4, 2019

@martpie

This comment has been minimized.

Copy link

commented Mar 4, 2019

Can I say I love you guys?

@acrisci

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

😎

@martpie

This comment has been minimized.

Copy link

commented Mar 5, 2019

@emersion Can we expect a release soon?

@emersion

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2019

Yes. I wonder if this should be a new major release. @acrisci, thoughts?

@acrisci

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Yes definitely a new major release. I'm pretty confident it works ok.

Electron integration is what worries me a little. I've tested it in the Electron sample app and it works fine. However, I can't get it to work in GPMDP. I think they might have some security thing that's disabling the node interfaces. I'm still looking into it.

@acrisci

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Ok just figured it out. They use an invalid interface name (includes the - character). Once I change those to _ it works fine. Going to add some validation for that.

@emersion

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2019

Nice. I'll wait a little to get your fix in.

@acrisci

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

@emersion I have a branch of GPMDP with mpris functional for playerctl. Ready for release 👍 .

@emersion

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2019

2.0 released! 🎉

Thanks again for your hard work!

@acrisci

This comment has been minimized.

@martpie

This comment has been minimized.

Copy link

commented Mar 6, 2019

@acrisci When trying to use mpris-service in Electron 4, I get an error with BigInt

A JavaScript error occured in the main process
Uncaught Exception:
TypeError: JSBI.BigInt is not a function

I am a bit confused because Electron 4 is based on Node 10 so everything "should" work, any idea?

require('node-dbus-next').setBigIntCompat(true) does not seem to change anything

@acrisci

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

@martpie can you open a new issue with some more information on the error?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.