Repository: godbus/dbus (go)

Issue:
Move AddMatchSignal from BusObject to Conn
`BusObject` interface defines:
```
AddMatchSignal(iface, member string, options ...MatchOption) *Call
RemoveMatchSignal(iface, member string, options ...MatchOption) *Call
```

I think they should be moved to `*Conn` since they work only with `conn.BusObject()` but available for all objects and their paths are not used in the match expressions.

Also it'd be better to change the signature to `AddMatchSignal(options ...MatchOption)` because it's more flexible and can be used say to subscribe to all signals or to signals from a particular sender.

Of course existing methods have to remain and be marked as deprecated.

What do you think on this?

Pull request:
Add Conn.AddMatchSignal and Conn.RemoveMatchSignal
Fixes #154 

Patch:
diff --git a/conn.go b/conn.go
index be28971..cc79d2d 100644
--- a/conn.go
+++ b/conn.go
@@ -504,6 +504,25 @@ func (conn *Conn) sendReply(dest string, serial uint32, values ...interface{}) {
 	conn.sendMessage(msg)
 }
 
+// AddMatchSignal registers the given match rule to receive broadcast
+// signals based on their contents.
+func (conn *Conn) AddMatchSignal(options ...MatchOption) error {
+	options = append([]MatchOption{withMatchType("signal")}, options...)
+	return conn.busObj.Call(
+		"org.freedesktop.DBus.AddMatch", 0,
+		formatMatchOptions(options),
+	).Store()
+}
+
+// RemoveMatchSignal removes the first rule that matches previously registered with AddMatchSignal.
+func (conn *Conn) RemoveMatchSignal(options ...MatchOption) error {
+	options = append([]MatchOption{withMatchType("signal")}, options...)
+	return conn.busObj.Call(
+		"org.freedesktop.DBus.RemoveMatch", 0,
+		formatMatchOptions(options),
+	).Store()
+}
+
 // Signal registers the given channel to be passed all received signal messages.
 //
 // Multiple of these channels can be registered at the same time.
diff --git a/match.go b/match.go
new file mode 100644
index 0000000..086ee33
--- /dev/null
+++ b/match.go
@@ -0,0 +1,62 @@
+package dbus
+
+import (
+	"strings"
+)
+
+// MatchOption specifies option for dbus routing match rule. Options can be constructed with WithMatch* helpers.
+// For full list of available options consult
+// https://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-routing-match-rules
+type MatchOption struct {
+	key   string
+	value string
+}
+
+func formatMatchOptions(options []MatchOption) string {
+	items := make([]string, 0, len(options))
+	for _, option := range options {
+		items = append(items, option.key+"='"+option.value+"'")
+	}
+	return strings.Join(items, ",")
+}
+
+// WithMatchOption creates match option with given key and value
+func WithMatchOption(key, value string) MatchOption {
+	return MatchOption{key, value}
+}
+
+// doesn't make sense to export this option because clients can only
+// subscribe to messages with signal type.
+func withMatchType(typ string) MatchOption {
+	return WithMatchOption("type", typ)
+}
+
+// WithMatchSender sets sender match option.
+func WithMatchSender(sender string) MatchOption {
+	return WithMatchOption("sender", sender)
+}
+
+// WithMatchSender sets interface match option.
+func WithMatchInterface(iface string) MatchOption {
+	return WithMatchOption("interface", iface)
+}
+
+// WithMatchMember sets member match option.
+func WithMatchMember(member string) MatchOption {
+	return WithMatchOption("member", member)
+}
+
+// WithMatchObjectPath creates match option that filters events based on given path
+func WithMatchObjectPath(path ObjectPath) MatchOption {
+	return WithMatchOption("path", string(path))
+}
+
+// WithMatchPathNamespace sets path_namespace match option.
+func WithMatchPathNamespace(namespace ObjectPath) MatchOption {
+	return WithMatchOption("path_namespace", string(namespace))
+}
+
+// WithMatchDestination sets destination match option.
+func WithMatchDestination(destination string) MatchOption {
+	return WithMatchOption("destination", destination)
+}
diff --git a/object.go b/object.go
index 9309b9b..8acd7fc 100644
--- a/object.go
+++ b/object.go
@@ -38,41 +38,16 @@ func (o *Object) CallWithContext(ctx context.Context, method string, flags Flags
 	return <-o.createCall(ctx, method, flags, make(chan *Call, 1), args...).Done
 }
 
-// MatchOption specifies option for dbus routing match rule. Options can be constructed with WithMatch* helpers.
-// For full list of available options consult
-// https://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-routing-match-rules
-type MatchOption struct {
-	key   string
-	value string
-}
-
-// WithMatchOption creates match option with given key and value
-func WithMatchOption(key, value string) MatchOption {
-	return MatchOption{key, value}
-}
-
-// WithMatchObjectPath creates match option that filters events based on given path
-func WithMatchObjectPath(path ObjectPath) MatchOption {
-	return MatchOption{"path", string(path)}
-}
-
-func formatMatchOptions(options []MatchOption) string {
-	items := make([]string, 0, len(options))
-	for _, option := range options {
-		items = append(items, option.key+"='"+option.value+"'")
-	}
-
-	return strings.Join(items, ",")
-}
-
 // AddMatchSignal subscribes BusObject to signals from specified interface,
 // method (member). Additional filter rules can be added via WithMatch* option constructors.
 // Note: To filter events by object path you have to specify this path via an option.
+//
+// Deprecated: use (*Conn) AddMatchSignal instead.
 func (o *Object) AddMatchSignal(iface, member string, options ...MatchOption) *Call {
 	base := []MatchOption{
-		{"type", "signal"},
-		{"interface", iface},
-		{"member", member},
+		withMatchType("signal"),
+		WithMatchInterface(iface),
+		WithMatchMember(member),
 	}
 
 	options = append(base, options...)
@@ -85,11 +60,13 @@ func (o *Object) AddMatchSignal(iface, member string, options ...MatchOption) *C
 
 // RemoveMatchSignal unsubscribes BusObject from signals from specified interface,
 // method (member). Additional filter rules can be added via WithMatch* option constructors
+//
+// Deprecated: use (*Conn) RemoveMatchSignal instead.
 func (o *Object) RemoveMatchSignal(iface, member string, options ...MatchOption) *Call {
 	base := []MatchOption{
-		{"type", "signal"},
-		{"interface", iface},
-		{"member", member},
+		withMatchType("signal"),
+		WithMatchInterface(iface),
+		WithMatchMember(member),
 	}
 
 	options = append(base, options...)


Test patch:
diff --git a/conn_test.go b/conn_test.go
index dfcf2e3..4ebf2a0 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -172,6 +172,65 @@ func TestCloseBeforeSignal(t *testing.T) {
 	}
 }
 
+func TestAddAndRemoveMatchSignal(t *testing.T) {
+	conn, err := SessionBusPrivate()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer conn.Close()
+
+	if err = conn.Auth(nil); err != nil {
+		t.Fatal(err)
+	}
+	if err = conn.Hello(); err != nil {
+		t.Fatal(err)
+	}
+
+	sigc := make(chan *Signal, 1)
+	conn.Signal(sigc)
+
+	// subscribe to a made up signal name and emit one of the type
+	if err = conn.AddMatchSignal(
+		WithMatchInterface("org.test"),
+		WithMatchMember("Test"),
+	); err != nil {
+		t.Fatal(err)
+	}
+	if err = conn.Emit("/", "org.test.Test"); err != nil {
+		t.Fatal(err)
+	}
+	if sig := waitSignal(sigc, "org.test.Test", time.Second); sig == nil {
+		t.Fatal("signal receive timed out")
+	}
+
+	// unsubscribe from the signal and check that is not delivered anymore
+	if err = conn.RemoveMatchSignal(
+		WithMatchInterface("org.test"),
+		WithMatchMember("Test"),
+	); err != nil {
+		t.Fatal(err)
+	}
+	if err = conn.Emit("/", "org.test.Test"); err != nil {
+		t.Fatal(err)
+	}
+	if sig := waitSignal(sigc, "org.test.Test", time.Second); sig != nil {
+		t.Fatalf("unsubscribed from %q signal, but received %#v", "org.test.Test", sig)
+	}
+}
+
+func waitSignal(sigc <-chan *Signal, name string, timeout time.Duration) *Signal {
+	for {
+		select {
+		case sig := <-sigc:
+			if sig.Name == name {
+				return sig
+			}
+		case <-time.After(timeout):
+			return nil
+		}
+	}
+}
+
 type server struct{}
 
 func (server) Double(i int64) (int64, *Error) {
diff --git a/match_test.go b/match_test.go
new file mode 100644
index 0000000..1e56b29
--- /dev/null
+++ b/match_test.go
@@ -0,0 +1,19 @@
+package dbus
+
+import "testing"
+
+func TestFormatMatchOptions(t *testing.T) {
+	opts := []MatchOption{
+		withMatchType("signal"),
+		WithMatchSender("org.bluez"),
+		WithMatchInterface("org.freedesktop.DBus.Properties"),
+		WithMatchMember("PropertiesChanged"),
+		WithMatchPathNamespace("/org/bluez/hci0"),
+	}
+	want := "type='signal',sender='org.bluez'," +
+		"interface='org.freedesktop.DBus.Properties'," +
+		"member='PropertiesChanged',path_namespace='/org/bluez/hci0'"
+	if have := formatMatchOptions(opts); have != want {
+		t.Fatalf("formatMatchOptions(%v) = %q, want %q", opts, have, want)
+	}
+}
diff --git a/object_test.go b/object_test.go
index f04e367..b821e7b 100644
--- a/object_test.go
+++ b/object_test.go
@@ -91,7 +91,13 @@ func TestObjectSignalHandling(t *testing.T) {
 	}
 
 	obj := bus.Object(name, path)
-	obj.AddMatchSignal(iface, "Heartbeat", WithMatchObjectPath(obj.Path()))
+	if err := bus.AddMatchSignal(
+		WithMatchInterface(iface),
+		WithMatchMember("Heartbeat"),
+		WithMatchObjectPath(path),
+	); err != nil {
+		t.Fatal(err)
+	}
 
 	ch := make(chan *Signal, 5)
 	bus.Signal(ch)
diff --git a/server_interfaces_test.go b/server_interfaces_test.go
index ca27b78..62be1fd 100644
--- a/server_interfaces_test.go
+++ b/server_interfaces_test.go
@@ -165,7 +165,7 @@ func (t *tester) DeliverSignal(iface, name string, signal *Signal) {
 	t.sigs <- signal
 }
 
-func (t *tester) AddSignal(iface, name string) {
+func (t *tester) AddSignal(iface, name string) error {
 	t.subSigsMu.Lock()
 	if i, ok := t.subSigs[iface]; ok {
 		i[name] = struct{}{}
@@ -174,8 +174,7 @@ func (t *tester) AddSignal(iface, name string) {
 		t.subSigs[iface][name] = struct{}{}
 	}
 	t.subSigsMu.Unlock()
-	t.conn.BusObject().(*Object).AddMatchSignal(
-		iface, name)
+	return t.conn.AddMatchSignal(WithMatchInterface(iface), WithMatchMember(name))
 }
 
 func (t *tester) Close() {
@@ -422,9 +421,16 @@ func TestHandlerSignal(t *testing.T) {
 	if err != nil {
 		t.Errorf("Unexpected error: %s", err)
 	}
-	tester.AddSignal("com.github.godbus.dbus.Tester", "sig1")
-	conn.Emit("/com/github/godbus/tester",
-		"com.github.godbus.dbus.Tester.sig1", "foo")
+	if err = tester.AddSignal("com.github.godbus.dbus.Tester", "sig1"); err != nil {
+		t.Fatal(err)
+	}
+	if err = conn.Emit(
+		"/com/github/godbus/tester",
+		"com.github.godbus.dbus.Tester.sig1",
+		"foo",
+	); err != nil {
+		t.Fatal(err)
+	}
 	select {
 	case sig := <-tester.sigs:
 		if sig.Body[0] != "foo" {