bug#20193: 25.0.50; declarative type specification for D-Bus args

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Daiki Ueno]

[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]

Severity: wishlist

Hello Michael,

While using dbus.el, I found it a bit cumbersome to specify type symbols
for the arguments of dbus-call-method and dbus-send-signal.

Suppose a simple D-Bus signature "a{si}".  Since dbusbind treats positive
integers as uint32, one would need to write:

  (dbus-call-method :session
                    "org.example.Foo"
                    "/org/example/Foo"
                    "org.example.Foo"
                    "Test"
                    '(:array
                       (:dict-entry :string "a" :int32 1)
                       (:dict-entry :string "b" :int32 2)))

This is simple if the arguments are "fixed", but if one wants a wrapper,
she would need to write a loop by herself.

  (defun example-foo (alist)
    (dbus-call-method :session
                      "org.example.Foo"
                      "/org/example/Foo"
                      "org.example.Foo"
                      "Test"
                      (list :array
                            (mapcar (lambda (entry)
                                      (list :dict-entry
                                            :string (car entry)
                                            :int32 (cdr entry)))
                                    alist))))

So I would like to propose an alternative syntax, similar to the `:type'
keyword of `defcustom', like this:

  (dbus-call-method :session
                    "org.example.Foo"
                    "/org/example/Foo"
                    "org.example.Foo"
                    "Test"
                    '(:type (:array (:dict-entry :string :int32)))
                    '(("a" . 1) ("b" . 2)))

That is, if a type symbol is a cons cell and the car is :type, treat the
cdr as the type specification of the following value.

The the wrapper can be written as:

  (defun example-foo (alist)
    (dbus-call-method :session
                      "org.example.Foo"
                      "/org/example/Foo"
                      "org.example.Foo"
                      "Test"
                      '(:type (:array (:dict-entry :string :int32)))
                      alist))

Would this kind of change be considered?  As a proof-of-concept, I'm
attaching a small Elisp snippet that augments the argument value with
the given type information (though I guess it should be implemented in
the C level, to avoid duplication of signature computation code).

Comments appreciated.

Regards,
-- 
Daiki Ueno

[-- Attachment #2: dbus--annotate-arg.el --]
[-- Type: text/plain, Size: 1301 bytes --]

;; (setq lexical-binding t)

;; (dbus--annotate-arg '(:array :int32) '(1 2 3))
;; ;=> ((:array :int32 1 :int32 2 :int32 3))

;; (dbus--annotate-arg '(:array (:dict-entry :string :int32)) '(("a" . 1) ("b" . 2)))
;; ;=> ((:array (:dict-entry :string "a" :int32 1) (:dict-entry :string "b" :int32 2)))

;; (dbus--annotate-arg '(:struct :object-path (:array :signature) :string)
;;                     '("path" ("sig") "password"))
;; ;=> ((:struct :object-path "path" (:array :signature "sig") :string "password"))

(defun dbus--annotate-arg (type arg)
  (pcase type
    ((and basic (or :byte :boolean :int16 :uint16 :int32 :uint32
		    :double :string :object-path :signature :unix-fd))
     (list basic arg))
    (`(:array ,elttype)
     (list (cons :array (apply #'nconc
			       (mapcar (lambda (subarg)
					 (dbus--annotate-arg elttype subarg))
				       arg)))))
    (`(,(and symbol (or :variant :struct)) . ,memtypes)
     (list (cons symbol (apply #'nconc
			       (cl-mapcar
				(lambda (memtype subarg)
				  (dbus--annotate-arg memtype subarg))
				memtypes arg)))))
    (`(:dict-entry ,keytype ,valtype)
     (list (cons :dict-entry (nconc (dbus--annotate-arg keytype (car arg))
			      (dbus--annotate-arg valtype (cdr arg))))))
    (_ (error "Unknown type specification: %S" type))))

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Michael Albinus]

Daiki Ueno <ueno@gnu.org> writes:

> Hello Michael,

Hi,

> Suppose a simple D-Bus signature "a{si}".  Since dbusbind treats positive
> integers as uint32, one would need to write:
>
> So I would like to propose an alternative syntax, similar to the `:type'
> keyword of `defcustom', like this:
>
>   (dbus-call-method :session
>                     "org.example.Foo"
>                     "/org/example/Foo"
>                     "org.example.Foo"
>                     "Test"
>                     '(:type (:array (:dict-entry :string :int32)))
>                     '(("a" . 1) ("b" . 2)))

In the early design phase of dbus.el I have played with explicit
signatures for the arguments, like

  (dbus-call-method :session
                    "org.example.Foo"
                    "/org/example/Foo"
                    "org.example.Foo"
                    "Test"
                    :signature "a{si}"
                    '(("a" 1) ("b" 2)))

which is similar to your proposal, with other means. I haven't
implemented it, because I found it to strict for Lisp developers to
think in D-Bus signatures. It has only survived for marking the type of
an empty array.

Your proposal is closer to the developers, so it has charm. There is
also a minor difference how dict entries are expressed, but that could
be agreed. So I'm open to this, as an *alternative* option to the
existing spec. We don't want to break existing code.

> Would this kind of change be considered?  As a proof-of-concept, I'm
> attaching a small Elisp snippet that augments the argument value with
> the given type information (though I guess it should be implemented in
> the C level, to avoid duplication of signature computation code).

IIRC, this was another reason that I haven't followed the :signature
approach - it was harder to implement in dbusbind.c. But this is years
ago, maybe my memories are wrong.

Yes, it shall be implemented in dbusbind.c - would you like to try it?

Btw, one of your examples is wrong (or at least misleading). An empty
array must contain exactly one element of type signature. The case you
have shown here indicates, that '(:array :signature "sig") is an array
with exact one elemt, a signature. Although possible in D-Bus, this is
not possible in dbus.el (maybe a design flaw?). Your example could be
therefore changed to

;; (dbus--annotate-arg '(:struct :object-path (:array (:dict-entry string :int32)) :string)
;;                     '("path" nil "password"))
;; ;=> ((:struct :object-path "path" (:array :signature "{si}") :string "password"))

This would require additional mapping of the type symbols to signature
strings - something what exist in dbusbind.c already.

> Regards,

Best regards, Michael.

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Daiki Ueno]

Michael Albinus <michael.albinus@gmx.de> writes:

> In the early design phase of dbus.el I have played with explicit
> signatures for the arguments, like
>
>   (dbus-call-method :session
>                     "org.example.Foo"
>                     "/org/example/Foo"
>                     "org.example.Foo"
>                     "Test"
>                     :signature "a{si}"
>                     '(("a" 1) ("b" 2)))
>
> which is similar to your proposal, with other means. I haven't
> implemented it, because I found it to strict for Lisp developers to
> think in D-Bus signatures. It has only survived for marking the type of
> an empty array.
>
> Your proposal is closer to the developers, so it has charm. There is
> also a minor difference how dict entries are expressed, but that could
> be agreed. So I'm open to this, as an *alternative* option to the
> existing spec. We don't want to break existing code.

Certainly.  Do you have any preference on the alternative form?  My
example used the `:type' keyword, but it was a tentative plan and might
be too verbose for programmers.  Reusing `:signature' might be a better
idea as you mentioned (though it might be confusing with the current
meaning of `:signature' as a D-Bus type "g").

>> Would this kind of change be considered?  As a proof-of-concept, I'm
>> attaching a small Elisp snippet that augments the argument value with
>> the given type information (though I guess it should be implemented in
>> the C level, to avoid duplication of signature computation code).
>
> IIRC, this was another reason that I haven't followed the :signature
> approach - it was harder to implement in dbusbind.c. But this is years
> ago, maybe my memories are wrong.
>
> Yes, it shall be implemented in dbusbind.c - would you like to try it?

Sure.

> Btw, one of your examples is wrong (or at least misleading). An empty
> array must contain exactly one element of type signature. The case you
> have shown here indicates, that '(:array :signature "sig") is an array
> with exact one elemt, a signature. Although possible in D-Bus, this is
> not possible in dbus.el (maybe a design flaw?). Your example could be
> therefore changed to
>
> ;; (dbus--annotate-arg '(:struct :object-path (:array (:dict-entry
> string :int32)) :string)
> ;;                     '("path" nil "password"))
> ;; ;=> ((:struct :object-path "path" (:array :signature "{si}")
> :string "password"))
>
> This would require additional mapping of the type symbols to signature
> strings - something what exist in dbusbind.c already.

Oh, I see.  Thanks for the info.

Regards,
--
Daiki Ueno

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Michael Albinus]

Daiki Ueno <ueno@gnu.org> writes:

> Certainly.  Do you have any preference on the alternative form?  My
> example used the `:type' keyword, but it was a tentative plan and might
> be too verbose for programmers.  Reusing `:signature' might be a better
> idea as you mentioned (though it might be confusing with the current
> meaning of `:signature' as a D-Bus type "g").

I'm OK with :type, as you said it is similar to what defcustom
offers. :signature would be confusing indeed, when it means Lisp
objects instead of a signature string.

> Regards,
> --
> Daiki Ueno

Best regards, Michael.

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Daiki Ueno]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

Michael Albinus <michael.albinus@gmx.de> writes:

> Daiki Ueno <ueno@gnu.org> writes:
>
>> Certainly.  Do you have any preference on the alternative form?  My
>> example used the `:type' keyword, but it was a tentative plan and might
>> be too verbose for programmers.  Reusing `:signature' might be a better
>> idea as you mentioned (though it might be confusing with the current
>> meaning of `:signature' as a D-Bus type "g").
>
> I'm OK with :type, as you said it is similar to what defcustom
> offers. :signature would be confusing indeed, when it means Lisp
> objects instead of a signature string.

Sorry for the long delay.  I have tried to implement it (patch
attached).  It basically adds a couple of new functions:

- xd_type_spec_to_signature(char *, Lisp_Object)
- xd_append_arg_with_type_spec(Lisp_Object, Lisp_Object, DBusMessageIter *)

which are similar to xd_signature and xd_append_arg, but take a
Lisp_Object denoting an argument type.

Comments appreciated.

Regards,
-- 
Daiki Ueno

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-dbusbind-Add-alternative-form-for-compound-args.patch --]
[-- Type: text/x-patch, Size: 9504 bytes --]

From 064aaaadd84310e4fd9beaa3d5c0fb74a70c4e92 Mon Sep 17 00:00:00 2001
From: Daiki Ueno <ueno@gnu.org>
Date: Thu, 27 Aug 2015 17:06:19 +0900
Subject: [PATCH] dbusbind: Add alternative form for compound args

* src/dbusbind.c (xd_append_basic_arg): New function, split from
xd_append_arg.
(xd_append_arg): Use xd_append_arg.
(xd_type_spec_to_signature): New function.
(xd_append_arg_with_type_spec): New function.
(Fdbus_message_internal): Use xd_append_arg_with_type_spec,
instead of xd_append_arg.
(syms_of_dbusbind): Provide subfeature `:type'.
* doc/misc/dbus.texi (Type Conversion): Mention `:type' keyword.

Fixes: debbugs:20193
---
 doc/misc/dbus.texi |  17 ++++-
 src/dbusbind.c     | 199 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 204 insertions(+), 12 deletions(-)

diff --git a/doc/misc/dbus.texi b/doc/misc/dbus.texi
index 5dd8bf2..03feecc 100644
--- a/doc/misc/dbus.texi
+++ b/doc/misc/dbus.texi
@@ -1030,7 +1030,22 @@ corresponding D-Bus container.  @code{:array} is optional, because
 this is the default compound D-Bus type for a list.
 
 The objects being elements of the list are checked according to the
-D-Bus compound type rules.
+D-Bus compound type rules.  If those elements have a type different
+from the default type, they need to be prefixed with a type symbol.
+
+@lisp
+(dbus-call-method @dots{} :array '(:int32 @var{NAT-NUMBER} :int32 @var{NAT-NUMBER}))
+@end lisp
+
+There is an alternative form to specify the type of the following
+compound type argument, using the keyword @code{:type} followed by a
+type specifier, which denotes a compound type as a list of type symbols.
+
+The above example is equivalent to:
+
+@lisp
+(dbus-call-method @dots{} :type '(:array :int32) '(@var{NAT-NUMBER} @var{NAT-NUMBER}))
+@end lisp
 
 @itemize
 @item An array must contain only elements of the same D-Bus type.  It
diff --git a/src/dbusbind.c b/src/dbusbind.c
index be1b890..81b6c27 100644
--- a/src/dbusbind.c
+++ b/src/dbusbind.c
@@ -567,18 +567,9 @@ xd_extract_unsigned (Lisp_Object x, uintmax_t hi)
     args_out_of_range_3 (x, make_number (0), make_fixnum_or_float (hi));
 }
 
-/* Append C value, extracted from Lisp OBJECT, to iteration ITER.
-   DTYPE must be a valid DBusType.  It is used to convert Lisp
-   objects, being arguments of `dbus-call-method' or
-   `dbus-send-signal', into corresponding C values appended as
-   arguments to a D-Bus message.  */
 static void
-xd_append_arg (int dtype, Lisp_Object object, DBusMessageIter *iter)
+xd_append_basic_arg (int dtype, Lisp_Object object, DBusMessageIter *iter)
 {
-  char signature[DBUS_MAXIMUM_SIGNATURE_LENGTH];
-  DBusMessageIter subiter;
-
-  if (XD_BASIC_DBUS_TYPE (dtype))
   switch (dtype)
     {
     case DBUS_TYPE_BYTE:
@@ -703,7 +694,21 @@ xd_append_arg (int dtype, Lisp_Object object, DBusMessageIter *iter)
 	return;
       }
     }
+}
+
+/* Append C value, extracted from Lisp OBJECT, to iteration ITER.
+   DTYPE must be a valid DBusType.  It is used to convert Lisp
+   objects, being arguments of `dbus-call-method' or
+   `dbus-send-signal', into corresponding C values appended as
+   arguments to a D-Bus message.  */
+static void
+xd_append_arg (int dtype, Lisp_Object object, DBusMessageIter *iter)
+{
+  char signature[DBUS_MAXIMUM_SIGNATURE_LENGTH];
+  DBusMessageIter subiter;
 
+  if (XD_BASIC_DBUS_TYPE (dtype))
+    xd_append_basic_arg (dtype, object, iter);
   else /* Compound types.  */
     {
 
@@ -792,6 +797,161 @@ xd_append_arg (int dtype, Lisp_Object object, DBusMessageIter *iter)
     }
 }
 
+static void
+xd_type_spec_to_signature (char *signature, Lisp_Object spec)
+{
+  int dtype;
+
+  if (SYMBOLP (spec))
+    {
+      dtype = xd_symbol_to_dbus_type (spec);
+      if (!XD_BASIC_DBUS_TYPE (dtype))
+	wrong_type_argument (intern ("D-Bus"), spec);
+      sprintf (signature, "%c", dtype);
+    }
+  else /* Compound types.  */
+    {
+      char *subsig;
+      char x[DBUS_MAXIMUM_SIGNATURE_LENGTH];
+      Lisp_Object elt;
+
+      CHECK_CONS (spec);
+
+      dtype = xd_symbol_to_dbus_type (CAR (spec));
+      elt = CDR_SAFE (spec);
+
+      switch (dtype)
+	{
+	case DBUS_TYPE_ARRAY:
+	  sprintf (signature, "%c", dtype);
+	  if (NILP (elt))
+	    /* If the array is empty, DBUS_TYPE_STRING is the default
+	       element type.  */
+	    subsig = DBUS_TYPE_STRING_AS_STRING;
+	  else
+	    {
+	      xd_type_spec_to_signature (x, CAR_SAFE (elt));
+	      subsig = x;
+	    }
+	  xd_signature_cat (signature, subsig);
+	  break;
+
+	case DBUS_TYPE_VARIANT:
+	  sprintf (signature, "%c", dtype);
+	  break;
+
+	case DBUS_TYPE_STRUCT:
+	  sprintf (signature, "%c", DBUS_STRUCT_BEGIN_CHAR);
+	  while (!NILP (elt))
+	    {
+	      xd_type_spec_to_signature (x, CAR_SAFE (elt));
+	      xd_signature_cat (signature, x);
+	      elt = CDR_SAFE (elt);
+	    }
+	  xd_signature_cat (signature, DBUS_STRUCT_END_CHAR_AS_STRING);
+	  break;
+
+	case DBUS_TYPE_DICT_ENTRY:
+	  sprintf (signature, "%c", DBUS_DICT_ENTRY_BEGIN_CHAR);
+	  while (!NILP (elt))
+	    {
+	      xd_type_spec_to_signature (x, CAR_SAFE (elt));
+	      xd_signature_cat (signature, x);
+	      elt = CDR_SAFE (elt);
+	    }
+	  xd_signature_cat (signature, DBUS_DICT_ENTRY_END_CHAR_AS_STRING);
+	  break;
+
+	default:
+	  wrong_type_argument (intern ("D-Bus"), spec);
+	}
+    }
+}
+
+static void
+xd_append_arg_with_type_spec (Lisp_Object spec, Lisp_Object object,
+			      DBusMessageIter *iter)
+{
+  int dtype;
+
+  if (SYMBOLP (spec))
+    {
+      dtype = xd_symbol_to_dbus_type (spec);
+      if (!XD_BASIC_DBUS_TYPE (dtype))
+	wrong_type_argument (intern ("D-Bus"), spec);
+      xd_append_basic_arg (dtype, object, iter);
+    }
+  else /* Compound types.  */
+    {
+      char signature[DBUS_MAXIMUM_SIGNATURE_LENGTH];
+      DBusMessageIter subiter;
+      int subtype;
+      Lisp_Object subspec;
+      Lisp_Object elt;
+
+      CHECK_CONS (spec);
+
+      dtype = xd_symbol_to_dbus_type (CAR (spec));
+      elt = CDR_SAFE (spec);
+
+      /* Open new subiteration.  */
+      switch (dtype)
+	{
+	case DBUS_TYPE_ARRAY:
+	  if (NILP (elt))
+	    /* If the array is empty, DBUS_TYPE_STRING is the default
+	       element type.  */
+	    subspec = QCdbus_type_string;
+	  else
+	    subspec = CAR_SAFE (elt);
+	  xd_type_spec_to_signature (signature, subspec);
+	  if (!dbus_message_iter_open_container (iter, dtype,
+						 signature, &subiter))
+	    XD_SIGNAL3 (build_string ("Cannot open container"),
+			make_number (dtype), build_string (signature));
+	  break;
+
+	case DBUS_TYPE_VARIANT:
+	  /* A variant has just one element.  */
+	  subspec = CAR_SAFE (elt);
+	  xd_type_spec_to_signature (signature, subspec);
+
+	  if (!dbus_message_iter_open_container (iter, dtype,
+						 signature, &subiter))
+	    XD_SIGNAL3 (build_string ("Cannot open container"),
+			make_number (dtype), build_string (signature));
+	  break;
+
+	case DBUS_TYPE_STRUCT:
+	case DBUS_TYPE_DICT_ENTRY:
+	  /* These containers do not require a signature.  */
+	  subspec = CAR_SAFE (elt);
+	  xd_type_spec_to_signature (signature, subspec);
+
+	  if (!dbus_message_iter_open_container (iter, dtype, NULL, &subiter))
+	    XD_SIGNAL2 (build_string ("Cannot open container"),
+			make_number (dtype));
+	  break;
+
+	default:
+	  wrong_type_argument (intern ("D-Bus"), list2 (spec, object));
+	}
+
+      /* Loop over list elements.  */
+      while (!NILP (object))
+	{
+	  xd_append_arg_with_type_spec (subspec, CAR_SAFE (object), &subiter);
+
+	  object = CDR_SAFE (object);
+	}
+
+      /* Close the subiteration.  */
+      if (!dbus_message_iter_close_container (iter, &subiter))
+	XD_SIGNAL2 (build_string ("Cannot close container"),
+		    make_number (dtype));
+    }
+}
+
 /* Retrieve C value from a DBusMessageIter structure ITER, and return
    a converted Lisp object.  The type DTYPE of the argument of the
    D-Bus message must be a valid DBusType.  Compound D-Bus types
@@ -1443,6 +1603,13 @@ usage: (dbus-message-internal &rest REST)  */)
   /* Append parameters to the message.  */
   for (; count < nargs; ++count)
     {
+      if (EQ (args[count], QCdbus_type_type))
+	{
+	  xd_append_arg_with_type_spec (args[count+1], args[count+2], &iter);
+	  count += 2;
+	}
+      else
+	{
 	  dtype = XD_OBJECT_TO_DBUS_TYPE (args[count]);
 	  if (XD_DBUS_TYPE_P (args[count]))
 	    {
@@ -1466,6 +1633,7 @@ usage: (dbus-message-internal &rest REST)  */)
 
 	  xd_append_arg (dtype, args[count], &iter);
 	}
+    }
 
   if (!NILP (handler))
     {
@@ -1718,6 +1886,8 @@ init_dbusbind (void)
 void
 syms_of_dbusbind (void)
 {
+  Lisp_Object subfeatures = Qnil;
+
   defsubr (&Sdbus__init_bus);
   defsubr (&Sdbus_get_unique_name);
 
@@ -1759,6 +1929,9 @@ syms_of_dbusbind (void)
   DEFSYM (QCdbus_type_struct, ":struct");
   DEFSYM (QCdbus_type_dict_entry, ":dict-entry");
 
+  /* Lisp symbol to indicate explicit typing of the following parameter.  */
+  DEFSYM (QCdbus_type_type, ":type");
+
   /* Lisp symbols of objects in `dbus-registered-objects-table'.  */
   DEFSYM (QCdbus_registered_serial, ":serial");
   DEFSYM (QCdbus_registered_method, ":method");
@@ -1866,7 +2039,11 @@ be called when the D-Bus reply message arrives.  */);
   xd_registered_buses = Qnil;
   staticpro (&xd_registered_buses);
 
-  Fprovide (intern_c_string ("dbusbind"), Qnil);
+  /* Add subfeature `:type'.  */
+  subfeatures = pure_cons (pure_cons (QCdbus_type_type, pure_cons (Qt, Qnil)),
+			   subfeatures);
+
+  Fprovide (intern_c_string ("dbusbind"), subfeatures);
 
 }
 
-- 
2.4.3

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Daiki Ueno]

Daiki Ueno <ueno@gnu.org> writes:

> Sorry for the long delay.  I have tried to implement it (patch
> attached).  It basically adds a couple of new functions:

Sorry, the patch didn't cleanly apply, as I created it with the diff
option -w.  The patch now resides in the scratch/dbusbind-type branch
for review.

By the way, for testing, I tend to think there could be a debugging
interface, which converts a Lisp expression to a D-Bus message and
vice-versa.

Thanks,
-- 
Daiki Ueno

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Michael Albinus]

Daiki Ueno <ueno@gnu.org> writes:

> Sorry, the patch didn't cleanly apply, as I created it with the diff
> option -w.  The patch now resides in the scratch/dbusbind-type branch
> for review.

No problem, I didn't start the review yet. I hope I could do it over the
weekend.

> By the way, for testing, I tend to think there could be a debugging
> interface, which converts a Lisp expression to a D-Bus message and
> vice-versa.

Good idea. Maybe it shall live in a file outside dbus.el; it's not
necessary to load this always when using D-Bus in Emacs.

> Thanks,

Best regards, Michael.

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Michael Albinus]

Daiki Ueno <ueno@gnu.org> writes:

> Sorry, the patch didn't cleanly apply, as I created it with the diff
> option -w.  The patch now resides in the scratch/dbusbind-type branch
> for review.

I've reviewed it, looks OK to me. The only thing not working was the
provided subfeature; I've fixed it in the code. The same subfeature
shall be provided also for dbus.el; users don't care about dbusbind.c.

In dbus.texi I have fixed a small error in your example, and I have
added a note how to test the subfeature.

I've committed my changes to the branch. If nobody else objects, you
might merge it into master.

> By the way, for testing, I tend to think there could be a debugging
> interface, which converts a Lisp expression to a D-Bus message and
> vice-versa.

Additionally, it might be helpful if you could add some tests to
test/automated/dbus-tests.el.

> Thanks,

Best regards, Michael.

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Daiki Ueno]

Michael Albinus <michael.albinus@gmx.de> writes:

> Daiki Ueno <ueno@gnu.org> writes:
>
>> Sorry, the patch didn't cleanly apply, as I created it with the diff
>> option -w.  The patch now resides in the scratch/dbusbind-type branch
>> for review.
>
> I've reviewed it, looks OK to me. The only thing not working was the
> provided subfeature; I've fixed it in the code.

Thanks for the prompt review.  I was actually not sure about the
standard usage of subfeatures, and copied the logic from process.c,
where they are defined as a plist, so they can be tested as:

  (featurep 'make-network-process '(:server t))

instead of:

  (featurep 'make-network-process :server)

> The same subfeature shall be provided also for dbus.el; users don't
> care about dbusbind.c.

That is a good idea.

> In dbus.texi I have fixed a small error in your example, and I have
> added a note how to test the subfeature.
>
> I've committed my changes to the branch. If nobody else objects, you
> might merge it into master.

Thanks for all the fixups.

>> By the way, for testing, I tend to think there could be a debugging
>> interface, which converts a Lisp expression to a D-Bus message and
>> vice-versa.
>
> Additionally, it might be helpful if you could add some tests to
> test/automated/dbus-tests.el.

I am working on this, but it is turning to be non-trivial.  So, I have
pushed it to a separate branch scratch/dbusbind-type-tests, branched off
from scratch/dbusbind-type.

Regards,
-- 
Daiki Ueno

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Michael Albinus]

Daiki Ueno <ueno@gnu.org> writes:

Hi,

> I was actually not sure about the standard usage of subfeatures, and
> copied the logic from process.c, where they are defined as a plist, so
> they can be tested as:
>
>   (featurep 'make-network-process '(:server t))
>
> instead of:
>
>   (featurep 'make-network-process :server)

featurep has the restriction, that you can test only one subfeature per
call. That's why they have created just one "subfeature", being a list.

> I am working on this, but it is turning to be non-trivial.  So, I have
> pushed it to a separate branch scratch/dbusbind-type-tests, branched off
> from scratch/dbusbind-type.

Fortunately, I had some time to look on this today. I've committed some
changes to dbusbind.c, all of them rather cosmetical. And I'm asking
myself, whether we shall rename `dbus-message-internal' and
`dbus-message-internal-to-lisp' to `dbus--message-*', in order to
emphasize their internal nature.

I have added two tests to `dbus-test04-create-message-parameters', both
fail. The first one must pass; this feature works in the master
branch. For the second new test I'm not sure whether this is possible
(the documentation doesn't speak about), but it looks natural to me.

Could you, pls, check what's up here?

> Regards,

Best regards, Michael.

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Daiki Ueno]

Michael Albinus <michael.albinus@gmx.de> writes:

>> I was actually not sure about the standard usage of subfeatures, and
>> copied the logic from process.c, where they are defined as a plist, so
>> they can be tested as:
>>
>>   (featurep 'make-network-process '(:server t))
>
> featurep has the restriction, that you can test only one subfeature per
> call. That's why they have created just one "subfeature", being a list.

Thanks for the explanation.

>> I am working on this, but it is turning to be non-trivial.  So, I have
>> pushed it to a separate branch scratch/dbusbind-type-tests, branched off
>> from scratch/dbusbind-type.
>
> Fortunately, I had some time to look on this today. I've committed some
> changes to dbusbind.c, all of them rather cosmetical.

Nice fixes, thanks!

> And I'm asking myself, whether we shall rename `dbus-message-internal'
> and `dbus-message-internal-to-lisp' to `dbus--message-*', in order to
> emphasize their internal nature.

That sounds like a good idea.

> I have added two tests to `dbus-test04-create-message-parameters', both
> fail. The first one must pass; this feature works in the master
> branch.

This was caused by a double registration of `:signature' symbol.  Fixed as:
http://git.savannah.gnu.org/cgit/emacs.git/commit/?h=scratch/dbusbind-type-tests&id=777848833cc9ff40411b78ad107e755172a881b8

> For the second new test I'm not sure whether this is possible (the
> documentation doesn't speak about), but it looks natural to me.

Yes, this was actually a bug because of missing checks on the number of
required arguments after `:type'.  Fixed as:
http://git.savannah.gnu.org/cgit/emacs.git/commit/?h=scratch/dbusbind-type-tests&id=def5829c0769b142b3cc0d69a9ad58935a9f237f

Regards,
-- 
Daiki Ueno

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Michael Albinus]

Daiki Ueno <ueno@gnu.org> writes:

>> For the second new test I'm not sure whether this is possible (the
>> documentation doesn't speak about), but it looks natural to me.
>
> Yes, this was actually a bug because of missing checks on the number of
> required arguments after `:type'.  Fixed as:
> http://git.savannah.gnu.org/cgit/emacs.git/commit/?h=scratch/dbusbind-type-tests&id=def5829c0769b142b3cc0d69a9ad58935a9f237f

Well, dbus-test.el passes now, thanks. But there are still some cases
I'm not so happy with:

plist-get
 (dbus--test-create-message-with-args
  '(:array)
  '(:array :signature "u")
  :type '(:array :uint32)
  nil)
 :signature)

|- "asauau"

I would expect "asauaub"

(plist-get
 (dbus--test-create-message-with-args
  '(:array)
  :type '(:array :uint32)
  '(:array :signature "u")
  '())
 :signature)

|- Debugger entered--Lisp error: (wrong-type-argument numberp :array)

How to specify an empty array, if it isn't the last argument? Maybe like this:

(plist-get
 (dbus--test-create-message-with-args
  '(:array)
  '(:array :signature "u")
  :type '(:array :uint32) nil
  nil)
 :signature)

|- "asauaub"

(plist-get
 (dbus--test-create-message-with-args
  '(:array)
  :type '(:array :uint32) nil
  '(:array :signature "u")
  nil)
 :signature)

|- "asauaub"

Maybe you could be a little bit more verbose about, and adapt the code?
When there is a :type argument, there must *always* be two additional
arguments? Or shall we allow the sloppy writing of one argument (the
type spec), when it is the last in the call, and it is an empty list?
This must be documented, then.

As you see, even I (who has tried to understand the new syntax) am a
little bit confused.

> Regards,

Best regards, Michael.

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Stefan Monnier]

> Thanks for the prompt review.  I was actually not sure about the
> standard usage of subfeatures, and copied the logic from process.c,

subfeatures should be conceptually a set, represented concretely as a list.

> where they are defined as a plist

That'd be a bug (that happens to be harmless if each value in the plist
is nil or t, since it's like adding nil and t to set of subfeatures).

>   (featurep 'make-network-process '(:server t))

Bad idea.

>   (featurep 'make-network-process :server)

That would be right.


        Stefan

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Daiki Ueno]

Michael Albinus <michael.albinus@gmx.de> writes:

> Daiki Ueno <ueno@gnu.org> writes:
>
>>> For the second new test I'm not sure whether this is possible (the
>>> documentation doesn't speak about), but it looks natural to me.
>>
>> Yes, this was actually a bug because of missing checks on the number of
>> required arguments after `:type'.  Fixed as:
>> http://git.savannah.gnu.org/cgit/emacs.git/commit/?h=scratch/dbusbind-type-tests&id=def5829c0769b142b3cc0d69a9ad58935a9f237f
>
> Well, dbus-test.el passes now, thanks. But there are still some cases
> I'm not so happy with:
>
> plist-get
>  (dbus--test-create-message-with-args
>   '(:array)
>   '(:array :signature "u")
>   :type '(:array :uint32)
>   nil)
>  :signature)
>
> |- "asauau"
>
> I would expect "asauaub"

Right, thanks for pointing that.

> Maybe you could be a little bit more verbose about, and adapt the code?
> When there is a :type argument, there must *always* be two additional
> arguments?

That makes sense.  Moreover, I personally prefer not to mix
implicit and explicit typing.  So, I am currently thinking to collect
type specifiers for all arguments as a list and put it in front of the
actual arguments, like this:

(dbus-message-internal ...
  :timeout 100
  :type '((:array :string) (:array :uint32) (:array :uint32) :boolean)
  '("a") '(1) '(2) t)

How does that sound?

> As you see, even I (who has tried to understand the new syntax) am a
> little bit confused.

Yes, I am realizing how helpful it is to write unit tests, to smoke out
such pitfalls before landing the feature :-)

Regards,
-- 
Daiki Ueno

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Michael Albinus]

Daiki Ueno <ueno@gnu.org> writes:

Hi,

>> Well, dbus-test.el passes now, thanks. But there are still some cases
>> I'm not so happy with:
>>
>> plist-get
>>  (dbus--test-create-message-with-args
>>   '(:array)
>>   '(:array :signature "u")
>>   :type '(:array :uint32)
>>   nil)
>>  :signature)
>>
>> |- "asauau"
>>
>> I would expect "asauaub"
>
> Right, thanks for pointing that.

Thinking about, it seems to be OK. Ah :type keyword must always be
followed by two other arguments, and so it takes nil as the empty array
values, instead of a boolean false. Whether we allow to be sloppy, and
to add a missing nil as empty value list at the end of the arguments, is
something to be decided (and documented).

>> Maybe you could be a little bit more verbose about, and adapt the code?
>> When there is a :type argument, there must *always* be two additional
>> arguments?
>
> That makes sense.

As said above.

> Moreover, I personally prefer not to mix implicit and explicit typing.
> So, I am currently thinking to collect type specifiers for all
> arguments as a list and put it in front of the actual arguments, like
> this:
>
> (dbus-message-internal ...
>   :timeout 100
>   :type '((:array :string) (:array :uint32) (:array :uint32) :boolean)
>   '("a") '(1) '(2) t)
>
> How does that sound?

I see two disadvantages:

- You couldn't use some functions any longer, like
  (dbus-string-to-byte-array "Hello world") mixed with :type prefixed
  arguments. Of course one could add a second function which returns
  just the value list, but is it really helpful? It makes everything
  more complex.

- You would loose the simplification of default types. A list is always
  an array of strings, a string is a string, a natural number is a
  uint32, and so on. You would be forced to write down the type
  explicitely for every argument.

And we could simply use signatures then. Something like

(dbus-message-internal ...
  :timeout 100
  :type "asauaub"
  '("a") '(1) '(2) t)

>> As you see, even I (who has tried to understand the new syntax) am a
>> little bit confused.
>
> Yes, I am realizing how helpful it is to write unit tests, to smoke out
> such pitfalls before landing the feature :-)

Yep. On my todo list is also to study the D-Bus tests at
<http://cgit.freedesktop.org/dbus/dbus-test/>. Maybe we could adapt
something from there.

> Regards,

Best regards, Michael.

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> Thinking about, it seems to be OK. Ah :type keyword must always be
> followed by two other arguments, and so it takes nil as the empty array
> values, instead of a boolean false. Whether we allow to be sloppy, and
> to add a missing nil as empty value list at the end of the arguments, is
> something to be decided (and documented).

Skimming through this thread, it looks like everybody thought that this
was a good idea?  But the branch hasn't received any updates since 2015,
and has not been merged...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#20193: 25.0.50; declarative type specification for D-Bus args

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

Hi Lars,

>> Thinking about, it seems to be OK. Ah :type keyword must always be
>> followed by two other arguments, and so it takes nil as the empty array
>> values, instead of a boolean false. Whether we allow to be sloppy, and
>> to add a missing nil as empty value list at the end of the arguments, is
>> something to be decided (and documented).
>
> Skimming through this thread, it looks like everybody thought that this
> was a good idea?  But the branch hasn't received any updates since 2015,
> and has not been merged...

I'm kind of undecided how to continue. It would make sense to merge this
only if Daiki Ueno is still interested in. Or somebody else makes it fit
for merge (I don't know, whether it still applies after 5 years).

Best regards, Michael.