Adding battery support on Cygwin

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

Adding battery support on Cygwin

Ken Brown-6
The Cygwin-w32 currently supports battery status via the function
w32fns.c:Fw32_battery_status.  The X11 and nox builds don't have this
support, and Cygwin lacks the facilities used on unix-like systems to
provide it (/proc/apm, etc.).  But it turns out to be easy to compile
and use Fw32_battery_status on all Cygwin builds, simply by pulling it
out of w32fns.c into a new file.

The attached patch does this.  OK to push?  If so, to which branch?

Ken

0001-Add-battery-support-to-all-Cygwin-builds.patch (18K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding battery support on Cygwin

Eli Zaretskii
> From: Ken Brown <[hidden email]>
> Date: Fri, 12 Jan 2018 21:33:33 -0500
>
> The Cygwin-w32 currently supports battery status via the function
> w32fns.c:Fw32_battery_status.  The X11 and nox builds don't have this
> support, and Cygwin lacks the facilities used on unix-like systems to
> provide it (/proc/apm, etc.).  But it turns out to be easy to compile
> and use Fw32_battery_status on all Cygwin builds, simply by pulling it
> out of w32fns.c into a new file.
>
> The attached patch does this.  OK to push?

Yes, but see some comments below.

> If so, to which branch?

Master, of course.  And please add a NEWS entry.

> * src/w32fns.c (Fw32_battery_status): Move to a new file,
> src/w32battery.c.

Let's use a better name, because this feature is not the last one to
be used both in the w32 and Cygwin/X builds.  How about w32common.c?
Or, if this is too similar to the (unrelated) w32common.h, how about
w32cygwinx.c (since cygw32.c is already taken)?

> * src/w32battery.h: New file, containing prototype of
> syms_of_w32battery.

We don't need a new header file just to have a single prototype in
it.  You can put this prototype in lisp.h (we already have quite a few
of syms_of_* there).

> +if test "${HAVE_W32}" = "no" && test "${opsys}" = "cygwin"; then
> +  W32_LIBS="$W32_LIBS -lkernel32"
> +  W32_OBJ="$W32_OBJ w32battery.o"
> +fi

This looks like W32_OBJ and W32_LIBS have some values in the Cygwin
non-w32 build, but AFAIU those symbols are actually empty in that
case, and the above is the only place where they get any content.  So
I'd say just W32_OBJ="w32battery.o" etc.

> -#ifdef HAVE_NTGUI
> +#if defined HAVE_NTGUI
>        syms_of_w32term ();
>        syms_of_w32fns ();
>        syms_of_w32menu ();
>        syms_of_fontset ();
>  #endif /* HAVE_NTGUI */

Why was this change necessary?

Thanks.

Reply | Threaded
Open this post in threaded view
|

Re: Adding battery support on Cygwin

Ken Brown-6
On 1/13/2018 3:11 AM, Eli Zaretskii wrote:

>> From: Ken Brown <[hidden email]>
>> Date: Fri, 12 Jan 2018 21:33:33 -0500
>>
>> The Cygwin-w32 currently supports battery status via the function
>> w32fns.c:Fw32_battery_status.  The X11 and nox builds don't have this
>> support, and Cygwin lacks the facilities used on unix-like systems to
>> provide it (/proc/apm, etc.).  But it turns out to be easy to compile
>> and use Fw32_battery_status on all Cygwin builds, simply by pulling it
>> out of w32fns.c into a new file.
>>
>> The attached patch does this.  OK to push?
>
> Yes, but see some comments below.

Thanks for the review.  I've pushed it to master with the changes you
suggested.

Ken