bug#12993: Wrong icon for Cygw32-Emacs

[Daniel Colascione]

On 12/10/12 6:08 AM, Eli Zaretskii wrote:
>> Date: Mon, 10 Dec 2012 15:40:18 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: 12993@debbugs.gnu.org, angelo.graziosi@alice.it
>>
>>> I've pushed a change to the emacs-24 branch that should resolve the
>>> problem. We weren't compiling Windows resources into the cygw32 Emacs
>>> binary.
>>
>> Why are such changes committed to the release branch?  This is a minor
>> feature, certainly not a bugfix

It looks more like a bugfix to me. Setting the window icon is
something we should have always done, and that we didn't do so is
problem the cygw32 build had versus the nt build, with which it should
have feature parity. It's wrong and generated user complaints. That
sounds like a bug to me. I'd also mentioned on the mailing list before
that I intended to fix this problem before release.

> The more I look at the changes, the less I like them.  E.g., how do we
> know that the various Windows utilities that access the resources will
> DTRT with forward slashes?  Any references on the subject?

The resources stored in the binary don't contain paths. (I checked
with a hex editor to make sure.)

> And what is this hunk about:
> 
>     === modified file 'src/unexw32.c'
>     --- a/src/unexw32.c       2012-10-17 19:02:44 +0000
>     +++ b/src/unexw32.c       2012-12-10 07:11:21 +0000
>     @@ -85,13 +85,6 @@
> 
>      PIMAGE_SECTION_HEADER heap_section;
> 
>     -#ifdef HAVE_NTGUI
>     -extern HINSTANCE hinst;
>     -HINSTANCE hprevinst = NULL;
>     -LPSTR lpCmdLine = "";
>     -int nCmdShow = 0;
>     -#endif /* HAVE_NTGUI */
>     -
>      /* Startup code for running on NT.  When we are running as the dumped
>       version, we need to bootstrap our heap and .bss section into our
>       address space before we can actually hand off control to the startup
>     @@ -121,15 +114,6 @@
>        /* Prevent Emacs from being locked up (eg. in batch mode) when
>         accessing devices that aren't mounted (eg. removable media drives).  
> */
>        SetErrorMode (SEM_FAILCRITICALERRORS);
>     -
>     -  /* Invoke the NT CRT startup routine now that our housecleaning
>     -     is finished.  */
>     -#ifdef HAVE_NTGUI
>     -  /* determine WinMain args like crt0.c does */
>     -  hinst = GetModuleHandle (NULL);
>     -  lpCmdLine = GetCommandLine ();
>     -  nCmdShow = SW_SHOWDEFAULT;
>     -#endif
>        mainCRTStartup ();
>      }
> 
> What do you know about lpCmdLine and nCmdShow, and "what crt0.c does"
> with them, to be sure they can be removed?

I removed dead code. Those variables are never used; the C runtime (to
which we dynamically link anyway) handles this initialization
internally. (We appear to have a separate bug with respect to
nCmdShow: create a shortcut to runemacs.exe and set it to start
minimized or start maximized. Emacs starts with a normal window.)

The setting of hinst specifically now happens in cache_system_info, so
we don't need to do it in _start, which doesn't run for Cygwin builds.

I tested the change, and it didn't cause any functional regressions
related to command-line parsing.

> And this hunk breaks the MS-DOS build:

*sigh* The MS-DOS build breaks when the wind blows the wrong way.

Does the build work inside DOSBox? I'll have to start testing the
build in that environment before pushing.

> Any reasons why not revert this changeset and apply it to the trunk
> instead?

The missing icon annoys users. Do we really want to ship that way?

signature.asc
Description: OpenPGP digital signature