bug#41744: 27.0.91; Various D-Bus cleanups

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#41744: 27.0.91; Various D-Bus cleanups

Basil L. Contovounesios
X-Debbugs-Cc: Michael Albinus <[hidden email]>
Severity: minor
Tags: patch

While reading through the D-Bus manual and implementation, I noticed
some opportunities for small cleanups.

I propose these documentation cleanups for emacs-27:



And these implementation cleanups for master:



Are these changes acceptable?  WDYT?

Thanks,

--
Basil

In GNU Emacs 27.0.91 (build 12, x86_64-pc-linux-gnu, X toolkit, Xaw3d scroll bars)
 of 2020-06-06 built on thunk
Repository revision: c1f4c0bc839324d2049ae45a74381684a5727f60
Repository branch: blc/27/dbus
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Debian GNU/Linux bullseye/sid

Configured using:
 'configure 'CC=ccache gcc' 'CFLAGS=-O0 -g3 -ggdb -gdwarf-4'
 --config-cache --prefix=/home/blc/.local --program-suffix=27
 --enable-checking=yes,glyphs --enable-check-lisp-object-type
 --with-x-toolkit=lucid --with-file-notification=yes --with-x'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF
XFT ZLIB TOOLKIT_SCROLL_BARS LUCID X11 XDBE XIM MODULES THREADS
LIBSYSTEMD JSON PDUMPER LCMS2 GMP

0001-Clean-up-D-Bus-documentation.patch (91K) Download Attachment
0001-Various-dbus.el-cleanups.patch (25K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#41744: 27.0.91; Various D-Bus cleanups

Michael Albinus
"Basil L. Contovounesios" <[hidden email]> writes:

Hi Basil,

> While reading through the D-Bus manual and implementation, I noticed
> some opportunities for small cleanups.

Thanks a lot for your enourmous work!

> I propose these documentation cleanups for emacs-27:

In general, I agree with your changes. However, they are so large that I
don't know whether they shall go into the emacs-27 branch. To be decided
by Eli, I guess.

> diff --git a/doc/misc/dbus.texi b/doc/misc/dbus.texi

The changed formatting of the examples exceeds sometimes 80 columns,
maybe you can adapt the formatting.

> And these implementation cleanups for master:

This I will review once the first patch set has arrived in master.

> Thanks,

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

bug#41744: 27.0.91; Various D-Bus cleanups

Basil L. Contovounesios
Michael Albinus <[hidden email]> writes:

> "Basil L. Contovounesios" <[hidden email]> writes:
>
>> While reading through the D-Bus manual and implementation, I noticed
>> some opportunities for small cleanups.
>
> Thanks a lot for your enourmous work!
>
>> I propose these documentation cleanups for emacs-27:
>
> In general, I agree with your changes.

Thanks for reviewing.

> However, they are so large that I don't know whether they shall go
> into the emacs-27 branch. To be decided by Eli, I guess.

Sure.

>> diff --git a/doc/misc/dbus.texi b/doc/misc/dbus.texi
>
> The changed formatting of the examples exceeds sometimes 80 columns,
> maybe you can adapt the formatting.

None of my changes increase the width of either text or examples.  The
only example touched by the patch which already exceeds 80 columns is
this:

@lisp
(@var{integer} ((@var{string} @var{bool} @var{bool}) (@var{string} @var{bool} @var{bool}) @dots{}))
@end lisp

But I left it alone because the result is less than 60 columns wide and
looks nice to me:

     (INTEGER ((STRING BOOL BOOL) (STRING BOOL BOOL) ...))

Can you please point out which parts you want reformatted?

>> And these implementation cleanups for master:
>
> This I will review once the first patch set has arrived in master.

Sure, thanks,

--
Basil



Reply | Threaded
Open this post in threaded view
|

bug#41744: 27.0.91; Various D-Bus cleanups

Michael Albinus
"Basil L. Contovounesios" <[hidden email]> writes:

Hi Basil,

>> The changed formatting of the examples exceeds sometimes 80 columns,
>> maybe you can adapt the formatting.
>
> None of my changes increase the width of either text or examples.  The
> only example touched by the patch which already exceeds 80 columns is
> this:
>
> @lisp
> (@var{integer} ((@var{string} @var{bool} @var{bool}) (@var{string}
> @var{bool} @var{bool}) @dots{}))
> @end lisp
>
> But I left it alone because the result is less than 60 columns wide and
> looks nice to me:
>
>      (INTEGER ((STRING BOOL BOOL) (STRING BOOL BOOL) ...))
>
> Can you please point out which parts you want reformatted?

I'm speaking about (info "(dbus) Introspection")
See the example of dbus-introspect-get-attribute. I would move the
arguments of dbus-introspect-xml into the next line.

> Sure, thanks,

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

bug#41744: 27.0.91; Various D-Bus cleanups

Michael Albinus
In reply to this post by Michael Albinus
Eli Zaretskii <[hidden email]> writes:

>> > I propose these documentation cleanups for emacs-27:
>>
>> In general, I agree with your changes. However, they are so large that I
>> don't know whether they shall go into the emacs-27 branch. To be decided
>> by Eli, I guess.
>
> Documentation changes are inherently safe.  So if you (Michael) think
> the changes are for the better, go ahead and install on the release
> branch.  If you aren't sure, e.g. if you think that some of the
> changes might be controversial, then master it is.

IMHO, there are no controversial pieces.

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

bug#41744: 27.0.91; Various D-Bus cleanups

Basil L. Contovounesios
Michael Albinus <[hidden email]> writes:

> Eli Zaretskii <[hidden email]> writes:
>
>>> > I propose these documentation cleanups for emacs-27:
>>>
>>> In general, I agree with your changes. However, they are so large that I
>>> don't know whether they shall go into the emacs-27 branch. To be decided
>>> by Eli, I guess.
>>
>> Documentation changes are inherently safe.  So if you (Michael) think
>> the changes are for the better, go ahead and install on the release
>> branch.  If you aren't sure, e.g. if you think that some of the
>> changes might be controversial, then master it is.
>
> IMHO, there are no controversial pieces.

Thanks, I've therefore incorporated the line breaks you suggested and
pushed the first patch to emacs-27.

Clean up D-Bus documentation (bug#41744)
43ad7dc1af 2020-06-08 18:19:50 +0100
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=43ad7dc1af327486963e5e3a3ae8efdb454fd38d

I'll ping this bug again in probably a week's time when the changes have
propagated to master.

--
Basil



Reply | Threaded
Open this post in threaded view
|

bug#41744: 27.0.91; Various D-Bus cleanups

Basil L. Contovounesios
In reply to this post by Michael Albinus
Michael Albinus <[hidden email]> writes:

> "Basil L. Contovounesios" <[hidden email]> writes:
>
>> And these implementation cleanups for master:
>
> This I will review once the first patch set has arrived in master.

The doc patch for emacs-27 has now propagated to master, so here's a
rebased version of the patch for master.  WDYT?

Thanks,

--
Basil


0001-Various-dbus.el-cleanups-bug-41744.patch (25K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#41744: 27.0.91; Various D-Bus cleanups

Michael Albinus
"Basil L. Contovounesios" <[hidden email]> writes:

Hi Basil,

> The doc patch for emacs-27 has now propagated to master, so here's a
> rebased version of the patch for master.  WDYT?
>
> --- a/lisp/net/dbus.el
> +++ b/lisp/net/dbus.el
> @@ -169,7 +166,6 @@ dbus-ignore-errors
>
>  (define-obsolete-variable-alias 'dbus-event-error-hooks
>    'dbus-event-error-functions "24.3")

I believe we should remove this variable now.

> @@ -339,8 +335,8 @@ dbus-call-method
>
> +(define-obsolete-function-alias 'dbus-call-method-non-blocking
> +  #'dbus-call-method "24.3")

Dito.

Otherwise, it looks good to me (but I haven't tested comprehensively).

For a while, I have on my wishlist to declare a D-Bus event as a
defstruct, like this:

(cl-defstruct (dbus-event (:type list) :named)
  "A D-Bus event, coming from or sent to other applications."
  bus-name message-type serial-number
  service-name path-name interface-name member-name handler)

But I don't know whether it is worth the effort.

> Thanks,
>
> --
> Basil

Best regards, Michael.



Reply | Threaded
Open this post in threaded view
|

bug#41744: 27.0.91; Various D-Bus cleanups

Basil L. Contovounesios
In reply to this post by Basil L. Contovounesios
tags 41744 fixed
close 41744 28.1
quit

Michael Albinus <[hidden email]> writes:

>> --- a/lisp/net/dbus.el
>> +++ b/lisp/net/dbus.el
>> @@ -169,7 +166,6 @@ dbus-ignore-errors
>>
>>  (define-obsolete-variable-alias 'dbus-event-error-hooks
>>    'dbus-event-error-functions "24.3")
>
> I believe we should remove this variable now.
>
>> @@ -339,8 +335,8 @@ dbus-call-method
>>
>> +(define-obsolete-function-alias 'dbus-call-method-non-blocking
>> +  #'dbus-call-method "24.3")
>
> Dito.

Done.

> Otherwise, it looks good to me (but I haven't tested comprehensively).

Thanks.  I pushed it to master so we can get some help with the
comprehensive testing. ;)

Various dbus.el cleanups (bug#41744)
97d1f672ac 2020-06-18 12:20:48 +0100
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=97d1f672ac1529ac07a999405f630cb19a1010eb

> For a while, I have on my wishlist to declare a D-Bus event as a
> defstruct, like this:
>
> (cl-defstruct (dbus-event (:type list) :named)
>   "A D-Bus event, coming from or sent to other applications."
>   bus-name message-type serial-number
>   service-name path-name interface-name member-name handler)
>
> But I don't know whether it is worth the effort.

I'm not yet familiar with cl-defstruct so I'll put it on my wishlist
too. :)

Thanks again,

--
Basil



Reply | Threaded
Open this post in threaded view
|

bug#41744: 27.0.91; Various D-Bus cleanups

Michael Albinus
"Basil L. Contovounesios" <[hidden email]> writes:

>> The paragraph in etc/NEWS must be moved somewhere else; will do
>> it next time I'll trim that file.
>
> Sorry about that.  Where is the correct place?  Under "Incompatible Lisp
> Changes"?  Let me know if you'd rather I fixed it.

Pushed to master.

> Thanks,

Best regards, Michael.