Options menu is broken on CVS

Options menu is broken on CVS

[İsmail Dönmez]

Hi all,

Options menu no longer works with emacs from CVS. To reproduce open emacs 
press F10->O and you get :

Symbol's value as variable is void: menu-updating-frame

Looks like recent changes to lisp/menu-bar.el borked this though I am not 100% 
certain with that. So wondering if anyone can reproduce this?

Regards,
ismail

Re: Options menu is broken on CVS

[İsmail Dönmez]

Salı 06 Eylül 2005 20:15 tarihinde şunları yazmıştınız:
> You might try a bootstrap build. I had problems like this when I attempted
> to build otherwise.

A clean CVS checkout compiled with make bootstrap is showing the same problem.

Regards,
ismail

Options menu is broken on CVS

[Nick Roberts]

 > Options menu no longer works with emacs from CVS. To reproduce open emacs 
 > press F10->O and you get :
 > 
 > Symbol's value as variable is void: menu-updating-frame
 > 
 > Looks like recent changes to lisp/menu-bar.el borked this though I am not
 > 100% certain with that. So wondering if anyone can reproduce this?

I don't see this problem on GNU/Linux.

menu-updating-frame is a builtin variable.  If you use M-x report-emacs-bug
(its on the menu-bar!) we can see how your Emacs was built and perhaps why
menu-updating-frame has been seemingly left out.

Nick

Re: Options menu is broken on CVS

[İsmail Dönmez]

Salı 06 Eylül 2005 23:45 tarihinde, Nick Roberts şunları yazmıştı: 
> M-x report-emacs-bug
Here it goes :

This bug report will be sent to the Free Software Foundation,
not to your local site managers!
Please write in English if possible, because the Emacs maintainers
usually do not have translators to read other languages for them.

Your bug report will be posted to the emacs-pretest-bug@gnu.org mailing list.

Please describe exactly what actions triggered the bug
and the precise symptoms of the bug:



If emacs crashed, and you have the emacs process in the gdb debugger,
please include the output from the following gdb commands:
    `bt full' and `xbacktrace'.
If you would like to further debug the crash, please read the file
/usr/local/share/emacs/22.0.50/etc/DEBUG for instructions.


In GNU Emacs 22.0.50.1 (i686-pc-linux-gnu)
 of 2005-09-06 on pardus
configured using `configure '--without-x''

Important settings:
  value of $LC_ALL: tr_TR.UTF-8
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: tr_TR.UTF-8
  locale-coding-system: utf-8
  default-enable-multibyte-characters: t

Major mode: Lisp Interaction

Minor modes in effect:
  show-paren-mode: t
  encoded-kbd-mode: t
  auto-compression-mode: t
  menu-bar-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  unify-8859-on-encoding-mode: t
  utf-translate-cjk-mode: t
  line-number-mode: t
  transient-mark-mode: t

Re: Options menu is broken on CVS

[Nick Roberts]

Re: Options menu is broken on CVS

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Wed, 7 Sep 2005 11:01:27 +1200
> Cc: emacs-devel@gnu.org
> 
> It seems to be due to this change:
> 
> 2004-03-13  Eli Zaretskii  <eliz@gnu.org>
> 
> 	* emacs.c (main): Call syms_of_xmenu only if HAVE_MENUS is defined.
> 
> I guess it hasn't been noticed before because, although people may use tmm,
> most use a version of Emacs thats compiled with X (or at least HAVE_MENUS).
> 
> I haven't reverted this change because it must be there for a reason.  Eli?

It _was_ for a reason.  The 2 related changes I committed at that time
were these:

2004-03-13  Eli Zaretskii  <eliz@gnu.org>

	* Makefile.in (XMENU_OBJ): Include xmenu.o if HAVE_MENUS is defined.

	* emacs.c (main): Call syms_of_xmenu only if HAVE_MENUS is defined.

I don't remember the details (you will probably find them in
emacs-devel or emacs-pretest-bug archives for that period of time),
but there was some build failure, I think on MacOS, which these
changes were part of fixing.

Please don't take out this change without understanding what it was
fixing in the first place.

Re: Options menu is broken on CVS

[İsmail Dönmez]

Çarşamba 07 Eylül 2005 07:16 tarihinde, Eli Zaretskii şunları yazmıştı: 
> > From: Nick Roberts <nickrob@snap.net.nz>
> > Date: Wed, 7 Sep 2005 11:01:27 +1200
> > Cc: emacs-devel@gnu.org
> >
> > It seems to be due to this change:
> >
> > 2004-03-13  Eli Zaretskii  <eliz@gnu.org>
> >
> > 	* emacs.c (main): Call syms_of_xmenu only if HAVE_MENUS is defined.
> >
> > I guess it hasn't been noticed before because, although people may use
> > tmm, most use a version of Emacs thats compiled with X (or at least
> > HAVE_MENUS).
> >
> > I haven't reverted this change because it must be there for a reason. 
> > Eli?
>
> It _was_ for a reason.  The 2 related changes I committed at that time
> were these:
>
> 2004-03-13  Eli Zaretskii  <eliz@gnu.org>
>
> 	* Makefile.in (XMENU_OBJ): Include xmenu.o if HAVE_MENUS is defined.
>
> 	* emacs.c (main): Call syms_of_xmenu only if HAVE_MENUS is defined.
>
> I don't remember the details (you will probably find them in
> emacs-devel or emacs-pretest-bug archives for that period of time),
> but there was some build failure, I think on MacOS, which these
> changes were part of fixing.
>
> Please don't take out this change without understanding what it was
> fixing in the first place.

Is there a way to fix this for non-X users of emacs ?

Regards,
ismail

Re: Options menu is broken on CVS

[Eli Zaretskii]

> From: =?utf-8?q?=C4=B0smail_D=C3=B6nmez?= <ismail@uludag.org.tr>
> Date: Wed, 7 Sep 2005 17:58:09 +0300
> 
> > I don't remember the details (you will probably find them in
> > emacs-devel or emacs-pretest-bug archives for that period of time),
> > but there was some build failure, I think on MacOS, which these
> > changes were part of fixing.
> >
> > Please don't take out this change without understanding what it was
> > fixing in the first place.
> 
> Is there a way to fix this for non-X users of emacs ?

Sorry, I don't understand: what is that ``this'' which you want to be
fixed?  If that's the tmm problem, it _will_ be fixed, we just need to
find a way to do that without breaking whatever it was that my change
back in 2004 fixed.

Re: Options menu is broken on CVS

[İsmail Dönmez]

Çarşamba 07 Eylül 2005 21:04 tarihinde, Eli Zaretskii şunları yazmıştı: 
> > From: =?utf-8?q?=C4=B0smail_D=C3=B6nmez?= <ismail@uludag.org.tr>
> > Date: Wed, 7 Sep 2005 17:58:09 +0300
> >
> > > I don't remember the details (you will probably find them in
> > > emacs-devel or emacs-pretest-bug archives for that period of time),
> > > but there was some build failure, I think on MacOS, which these
> > > changes were part of fixing.
> > >
> > > Please don't take out this change without understanding what it was
> > > fixing in the first place.
> >
> > Is there a way to fix this for non-X users of emacs ?
>
> Sorry, I don't understand: what is that ``this'' which you want to be
> fixed?  If that's the tmm problem, it _will_ be fixed, we just need to
> find a way to do that without breaking whatever it was that my change
> back in 2004 fixed.

Problem is F10->O no longer works in emacs configured with --without-x . And 
as far as I can tell its a more recent change than in 2004 as I am using 
emacs cvs for long and this problem is new.

Regards,
ismail

Re: Options menu is broken on CVS

[Nick Roberts]

 > Please don't take out this change without understanding what it was
 > fixing in the first place.

I think this is the right fix.  Actually I think the problem was caused by
Kim's change:

2004-03-11  Kim F. Storm  <storm@cua.dk>

	* Makefile.in:...
	(XMENU_OBJ) [HAVE_MENUS]: Move declaration to proper place.

The preamble for xmenu.c says:

/* X Communication module for terminals which understand the X protocol.

but xmenu.c is more general than that and compiles without X.  Even its
name is misleading (as with xdisp.c which also compiles without X).

Nick


*** Makefile.in.~1.313~	2005-09-08 09:56:13.000000000 +1200
--- Makefile.in	2005-09-08 09:51:50.000000000 +1200
*************** ALL_CFLAGS=-Demacs -DHAVE_CONFIG_H $(TOO
*** 304,319 ****
  #define LIB_X11_LIB -lX11
  #endif
  
  #ifdef HAVE_X_WINDOWS
  
  XOBJ= xterm.o xfns.o xselect.o xrdb.o fontset.o xsmfns.o fringe.o image.o
  
  #ifdef HAVE_MENUS
  
- #ifndef HAVE_CARBON
- XMENU_OBJ = xmenu.o
- #endif
- 
  #ifdef USE_GTK
  GTK_OBJ= gtkutil.o
  #endif
--- 304,319 ----
  #define LIB_X11_LIB -lX11
  #endif
  
+ #ifndef HAVE_CARBON
+ XMENU_OBJ = xmenu.o
+ #endif
+ 
  #ifdef HAVE_X_WINDOWS
  
  XOBJ= xterm.o xfns.o xselect.o xrdb.o fontset.o xsmfns.o fringe.o image.o
  
  #ifdef HAVE_MENUS
  
  #ifdef USE_GTK
  GTK_OBJ= gtkutil.o
  #endif


*** emacs.c.~1.374.~	2005-08-28 09:37:13.000000000 +1200
--- emacs.c	2005-09-08 09:51:30.000000000 +1200
*************** main (argc, argv
*** 1624,1637 ****
  #endif
  #endif /* HAVE_X_WINDOWS */
  
- #ifdef HAVE_MENUS
  #ifndef HAVE_NTGUI
  #ifndef MAC_OS
        /* Called before init_window_once for Mac OS Classic.  */
        syms_of_xmenu ();
  #endif
  #endif
- #endif
  
  #ifdef HAVE_NTGUI
        syms_of_w32term ();
--- 1624,1635 ----

Re: Options menu is broken on CVS

[Nick Roberts]

 > > > Is there a way to fix this for non-X users of emacs ?

If you have the X libraries you can compile with X and use it in terminal
mode (emacs -nw).

 > > Sorry, I don't understand: what is that ``this'' which you want to be
 > > fixed?  If that's the tmm problem, it _will_ be fixed, we just need to
 > > find a way to do that without breaking whatever it was that my change
 > > back in 2004 fixed.
 > 
 > Problem is F10->O no longer works in emacs configured with --without-x . And 
 > as far as I can tell its a more recent change than in 2004 as I am using 
 > emacs cvs for long and this problem is new.

Have you've always configured without X or just used Emacs in terminal mode?

Nick

Re: Options menu is broken on CVS

[Nick Roberts]

Re: Options menu is broken on CVS

[Richard M. Stallman]

    It seems to be due to this change:

    2004-03-13  Eli Zaretskii  <eliz@gnu.org>

	    * emacs.c (main): Call syms_of_xmenu only if HAVE_MENUS is defined.

    I guess it hasn't been noticed before because, although people may use tmm,
    most use a version of Emacs thats compiled with X (or at least HAVE_MENUS).

    I haven't reverted this change because it must be there for a reason.  Eli?

Eli is surely right that this change fixed a bug.  However, this shows
that the bug has to be fixed a different way.

Perhaps the best way to find out what the bug was is to revert the
change; then someone will see that bug again, and someone can write a
new fix for it.

Re: Options menu is broken on CVS

[Eli Zaretskii]

> From: "Richard M. Stallman" <rms@gnu.org>
> Date: Wed, 07 Sep 2005 22:41:46 -0400
> Cc: ismail@uludag.org.tr, emacs-devel@gnu.org
> 
> Perhaps the best way to find out what the bug was is to revert the
> change; then someone will see that bug again, and someone can write a
> new fix for it.

No, this is IMHO a bad way: the chances of that someone to see that
bug could be small, depending on what was the configuration in which
it was found back then.  It is much better to read the emacs-devel
discussions related to my change and understand what was the bug I
fixed.

Re: Options menu is broken on CVS

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Thu, 8 Sep 2005 10:04:25 +1200
> Cc: ismail@uludag.org.tr, emacs-devel@gnu.org
> 
>  > Please don't take out this change without understanding what it was
>  > fixing in the first place.
> 
> I think this is the right fix.

Please describe the reasons why you think this is the right fix.  (I'm
assuming you've read the discussions from 2004 that led to the
original changes.)

> Actually I think the problem was caused by Kim's change:
> 
> 2004-03-11  Kim F. Storm  <storm@cua.dk>
> 
> 	* Makefile.in:...
> 	(XMENU_OBJ) [HAVE_MENUS]: Move declaration to proper place.

That change was made for a reason as well: some problem on Carbon.  We
need to understand that problem, to be sure your change will not
reintroduce it.  I hope that the explanations I asked for above will
clarify this (I still didn't have time to re-read those past
discussions and retrace what problems we were trying to fix.)

In addition, we need to explain why the OP says he started to see the
problem only recently.

> The preamble for xmenu.c says:
> 
> /* X Communication module for terminals which understand the X protocol.
> 
> but xmenu.c is more general than that and compiles without X.  Even its
> name is misleading (as with xdisp.c which also compiles without X).

That's history: xmenu.c was originally written for X, but then menu
support was added to the DOS port and later came tmm.  If you think
the name xmenu tricked Kim and myself into thinking it's only for X,
that was not the reason.

Re: Options menu is broken on CVS

[Nick Roberts]

 > > I think this is the right fix.
 > 
 > Please describe the reasons why you think this is the right fix.

menu-updating-buffers is defined in syms_of_xmenu ().  Currently syms_of_xmenu
is only called in emacs.c if HAVE_MENUS is true.  menu-updating-buffers is
needed even if Emacs is configured without X (on GNU/Linux at least) but in
this case HAVE_MENUS is not defined.

xmenu.c is needed even HAVE_X_WINDOWS is not defined so I've moved it outside
the conditional requiring it.

 >                                                         (I'm
 > assuming you've read the discussions from 2004 that led to the
 > original changes.)

I might not have followed it all but your change seemed to cover Carbon Emacs
which it still does:

#ifndef HAVE_CARBON
XMENU_OBJ = xmenu.o
#endif

Now I've moved it outside #ifdef HAVE_X_WINDOWS you might need to add
another condition for when w32menu.o is used, I'm not sure. 

 > > Actually I think the problem was caused by Kim's change:
 > > 
 > > 2004-03-11  Kim F. Storm  <storm@cua.dk>
 > > 
 > > 	* Makefile.in:...
 > > 	(XMENU_OBJ) [HAVE_MENUS]: Move declaration to proper place.
 > 
 > That change was made for a reason as well: some problem on Carbon.  We
 > need to understand that problem, to be sure your change will not
 > reintroduce it.  I hope that the explanations I asked for above will
 > clarify this (I still didn't have time to re-read those past
 > discussions and retrace what problems we were trying to fix.)

I didn't find the discussion that led to this change.  It might have been
part of a general tidying process.

 > In addition, we need to explain why the OP says he started to see the
 > problem only recently.

I've tried to explain that in another post: more calls to menu-updating-frame
have been made in menu-bar.el (26/08/05).

 > > The preamble for xmenu.c says:
 > > 
 > > /* X Communication module for terminals which understand the X protocol.
 > > 
 > > but xmenu.c is more general than that and compiles without X.  Even its
 > > name is misleading (as with xdisp.c which also compiles without X).
 > 
 > That's history: xmenu.c was originally written for X, but then menu
 > support was added to the DOS port and later came tmm.  If you think
 > the name xmenu tricked Kim and myself into thinking it's only for X,
 > that was not the reason.

I don't have an opinion on whether you or Kim were tricked, just that the
description is misleading and that xmenu.c is needed even HAVE_X_WINDOWS is
not defined.

Nick

Re: Options menu is broken on CVS

[Richard M. Stallman]

    but xmenu.c is more general than that and compiles without X.  Even its
    name is misleading (as with xdisp.c which also compiles without X).

When I made the file name xdisp.c, I had not yet heard about X Windows.
I put in that file the code for the guts of display that I had rewritten.

Re: Options menu is broken on CVS

[İsmail Dönmez]

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

Perşembe 08 Eylül 2005 01:04 tarihinde şunları yazmıştınız:
>  > Please don't take out this change without understanding what it was
>  > fixing in the first place.
>
> I think this is the right fix.  Actually I think the problem was caused by
> Kim's change:
>

Your patch fixes the problem for me. I attached a diff -u version of it in 
case someone wants to apply it too.

Regards,
ismail


[-- Attachment #2: menu.diff --]
[-- Type: text/x-diff, Size: 1121 bytes --]

Index: src/Makefile.in
===================================================================
RCS file: /cvsroot/emacs/emacs/src/Makefile.in,v
retrieving revision 1.313
diff -u -r1.313 Makefile.in
--- src/Makefile.in	7 Aug 2005 12:33:16 -0000	1.313
+++ src/Makefile.in	8 Sep 2005 09:48:31 -0000
@@ -304,15 +304,15 @@
 #define LIB_X11_LIB -lX11
 #endif
 
+#ifndef HAVE_CARBON
+XMENU_OBJ = xmenu.o
+#endif
+
 #ifdef HAVE_X_WINDOWS
 
 XOBJ= xterm.o xfns.o xselect.o xrdb.o fontset.o xsmfns.o fringe.o image.o
 
 #ifdef HAVE_MENUS
-
-#ifndef HAVE_CARBON
-XMENU_OBJ = xmenu.o
-#endif
 
 #ifdef USE_GTK
 GTK_OBJ= gtkutil.o
Index: src/emacs.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/emacs.c,v
retrieving revision 1.374
diff -u -r1.374 emacs.c
--- src/emacs.c	27 Aug 2005 12:23:22 -0000	1.374
+++ src/emacs.c	8 Sep 2005 09:48:44 -0000
@@ -1624,12 +1624,10 @@
 #endif
 #endif /* HAVE_X_WINDOWS */
 
-#ifdef HAVE_MENUS
 #ifndef HAVE_NTGUI
 #ifndef MAC_OS
       /* Called before init_window_once for Mac OS Classic.  */
       syms_of_xmenu ();
-#endif
 #endif
 #endif
 

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Options menu is broken on CVS

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Thu, 8 Sep 2005 16:24:50 +1200
> Cc: ismail@uludag.org.tr, emacs-devel@gnu.org
> 
>  > > I think this is the right fix.
>  > 
>  > Please describe the reasons why you think this is the right fix.
> 
> menu-updating-buffers is defined in syms_of_xmenu ().  Currently syms_of_xmenu
> is only called in emacs.c if HAVE_MENUS is true.  menu-updating-buffers is
> needed even if Emacs is configured without X (on GNU/Linux at least) but in
> this case HAVE_MENUS is not defined.
> 
> xmenu.c is needed even HAVE_X_WINDOWS is not defined so I've moved it outside
> the conditional requiring it.

Thanks.

However, this is not what I asked; the reasons why symbols defined in
syms_of_xmenu are void unless xmenu.o is linked in are perfectly
clear.  Sorry for not being more clear.

What I meant is to have an explanation why your change solves the same
problem which caused Kim and myself to place XMENU_OBJ in a different
place, without breaking other ports and configurations.  That would
require to understand the original problem, which had something to do
with Carbon.

> Now I've moved it outside #ifdef HAVE_X_WINDOWS you might need to add
> another condition for when w32menu.o is used, I'm not sure. 

That's exactly the breakage I was afraid of.

>  > That change was made for a reason as well: some problem on Carbon.  We
>  > need to understand that problem, to be sure your change will not
>  > reintroduce it.  I hope that the explanations I asked for above will
>  > clarify this (I still didn't have time to re-read those past
>  > discussions and retrace what problems we were trying to fix.)
> 
> I didn't find the discussion that led to this change.  It might have been
> part of a general tidying process.

No, it was to solve a very specific issue with Carbon.  Okay, I will
try to dig into this over the weekend.  Solution for non-X builds will
have to wait until then.

Re: Options menu is broken on CVS

[Stefan Monnier]

>> Perhaps the best way to find out what the bug was is to revert the
>> change; then someone will see that bug again, and someone can write a
>> new fix for it.

> No, this is IMHO a bad way: the chances of that someone to see that
> bug could be small, depending on what was the configuration in which
> it was found back then.  It is much better to read the emacs-devel
> discussions related to my change and understand what was the bug I
> fixed.

Let's take it as a reminder that when we commit a fix, we should make sure
we describe the bug it fixes, either in comments in the code (when
possible/meaningful) or in the ChangeLog entry.  Of course, if the old code
was just plain wrong it's not necessary.


        Stefan

Re: Options menu is broken on CVS

[Eli Zaretskii]

> Cc: rms@gnu.org,  nickrob@snap.net.nz,  ismail@uludag.org.tr,
> 	  emacs-devel@gnu.org
> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 08 Sep 2005 17:02:05 -0400
> 
> Let's take it as a reminder that when we commit a fix, we should make sure
> we describe the bug it fixes, either in comments in the code (when
> possible/meaningful) or in the ChangeLog entry.

Alas, there's no machinery in GNU coding/development standards to
record such information.  In Emacs, we don't even have a bug
repository to which one can refer, like "see bug #12345" or some such
(a reference to the mail-list thread might serve as a replacement, but
only if the problem was discussed publicly in enough detail).  A
description of a bug with enough info to reproduce it later could take
a lot of text, and it's IMHO impractical to put all that text in the
source files or in the logs.

In addition, there are situations where you don't have a good place to
put a comment, e.g. if the fix was to _delete_ something.

> Of course, if the old code was just plain wrong it's not necessary.

``Plain wrong'' is in the eyes of the beholder.  Such judgment calls
are part of the problem: what's ``plain wrong and doesn't need any
explanation'' so some, is a mystery to others.

Re: Options menu is broken on CVS

[Eli Zaretskii]

> Date: Thu, 08 Sep 2005 22:33:45 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: ismail@uludag.org.tr, emacs-devel@gnu.org
> 
> >  > That change was made for a reason as well: some problem on Carbon.  We
> >  > need to understand that problem, to be sure your change will not
> >  > reintroduce it.  I hope that the explanations I asked for above will
> >  > clarify this (I still didn't have time to re-read those past
> >  > discussions and retrace what problems we were trying to fix.)
> > 
> > I didn't find the discussion that led to this change.  It might have been
> > part of a general tidying process.
> 
> No, it was to solve a very specific issue with Carbon.  Okay, I will
> try to dig into this over the weekend.  Solution for non-X builds will
> have to wait until then.

The relevant pieces of information are here:

  http://lists.gnu.org/archive/html/emacs-pretest-bug/2004-03/msg00132.html
  http://lists.gnu.org/archive/html/emacs-devel/2004-03/msg00231.html

and in the ensuing threads.  In essence, the events unrolled as
follows:

  . Kim moved XMENU_OBJ to a different place (I'm not quite sure why,
    but it had something to do with consolidating image support on
    different systems, see Kim's src/ChangeLog entries for
    2004-03-11).

  . This broke the --without-x build on Unix and GNU systems, since
    xmenu.o was not being linked in.  See the first thread above.

  . I fixed this on 2004-03-13 by adding XMENU_OBJ to the non-X
    branch, conditioned on HAVE_MENUS.

  . This broke the Carbon non-X build, since xmenu.o does not compile
    on Carbon.  (See the second thread above.)  The fix was to
    condition xmenu.o inclusion in the non-X case by HAVE_CARBON _not_
    being defined.

The result of all this was that xmenu.o was being linked in both in
the X and in the non-X branches, and in both cases it was conditioned
on HAVE_MENUS being defined and HAVE_CARBON not being defined (because
Carbon uses macmenu.o instead).

Now, your suggested change simply moves the first instance of xmenu.o
outside the X branch and also outside the HAVE_MENUS condition (and
does nothing for the second instance, btw).  Why is that the right
thing?  I think the non-X build on Unix and GNU systems doesn't define
HAVE_MENUS for a reason: without the proper X11 headers and libraries,
xmenu.c simply will not compile.

Moreover, I have a non-X Emacs built out of CVS of June 11 2005, and
F11->o works there without any problems, although I verified that
xmenu.o was not linked in (in fact, there's no xmenu.o at all there,
since xmenu.c was not compiled).

My interim conclusion is that the changes back in 2004 were not the
one that caused the current problem.  I will dig some more to
understand what is the real cause of that, and describe my findings
here.

Meanwhile, I don't think we should apply your suggested changes, since
compiling xmenu.c on a system without X headers and libraries might
fail due to unresolved externals.

Re: Options menu is broken on CVS

[Stefan Monnier]

>> Let's take it as a reminder that when we commit a fix, we should make sure
>> we describe the bug it fixes, either in comments in the code (when
>> possible/meaningful) or in the ChangeLog entry.

> Alas, there's no machinery in GNU coding/development standards to
> record such information.  In Emacs, we don't even have a bug
> repository to which one can refer, like "see bug #12345" or some such
> (a reference to the mail-list thread might serve as a replacement, but
> only if the problem was discussed publicly in enough detail).  A
> description of a bug with enough info to reproduce it later could take
> a lot of text,

Even if we had a bug repository, I'd prefer a longish text, so you don't
need to refer to the bug database to know what it is.  The only alternative
would be if the bug database is stored alongside the source files.

> In addition, there are situations where you don't have a good place to
> put a comment, e.g. if the fix was to _delete_ something.

You can replace the deleted code with the comment (of course, only if the
location of the comment makes sense: if you delete a whole function at the
toplevel, replacing it with a toplevel comment won't be much help), or you
can put the info in the ChangeLog.
It's a fundamental problem and there's not much we can do about that, really.

>> Of course, if the old code was just plain wrong it's not necessary.
> ``Plain wrong'' is in the eyes of the beholder.

Very much so,


        Stefan

Re: Options menu is broken on CVS

[Nick Roberts]

Eli Zaretskii writes:
 > Now, your suggested change simply moves the first instance of xmenu.o
 > outside the X branch and also outside the HAVE_MENUS condition (and
 > does nothing for the second instance, btw).  

I think the second instance could be removed.

 > Why is that the right thing?

It seems to work for -with/without-x and probably for proprietary systems
too.

 > I think the non-X build on Unix and GNU systems doesn't define
 > HAVE_MENUS for a reason: without the proper X11 headers and libraries,
 > xmenu.c simply will not compile.

I'm not sure what HAVE_MENUS means but non-X systems clearly have menus
(perhaps you've been tricked!). However, xmenu.c _does_ compile without
X11 headers and libraries, otherwise you couldn't make a non-X build with
it.

 > Moreover, I have a non-X Emacs built out of CVS of June 11 2005, and
 > F11->o works there without any problems, although I verified that
 > xmenu.o was not linked in (in fact, there's no xmenu.o at all there,
 > since xmenu.c was not compiled).

The OP said F10->O didn't work whatever that means.  F10->o does work because
it doesn't use menu-updating-frames.  Try, for example F10->f with your copy,
I don't think that will work.

 > My interim conclusion is that the changes back in 2004 were not the
 > one that caused the current problem.

I think you are wrong.

 > I will dig some more to understand what is the real cause of that, and
 > describe my findings here.

 > Meanwhile, I don't think we should apply your suggested changes, since
 > compiling xmenu.c on a system without X headers and libraries might
 > fail due to unresolved externals.

It solved the OP's problem.  I suggest that we do apply it as thats the only
way to test it.  If it is wrong, I am sure we will hear about it shortly.

Nick

Re: Options menu is broken on CVS

[Eli Zaretskii]

> Date: Fri, 09 Sep 2005 14:50:55 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: ismail@uludag.org.tr, emacs-devel@gnu.org
> 
> My interim conclusion is that the changes back in 2004 were not the
> one that caused the current problem.  I will dig some more to
> understand what is the real cause of that, and describe my findings
> here.

I debugged this.  It's like you said earlier in this thread:

  This change

  2005-08-26  David Reitter  <david.reitter@gmail.com>

	  * menu-bar.el (truncate-lines, write-file, print-buffer)
	  (ps-print-buffer-faces, ps-print-buffer, split-window):
	  Disable menu items when the frame they refer to is invisible, or when
	  they refer to a buffer and the minibuffer is selected.

  will break things if you try to access one of these functions from the menubar
  having configured --without-x.

The problem is indeed that this change caused the menus to reference
menu-updating-frame, which is not defined in the non-X version.

I installed a change that avoids referencing that variable on
non-multiframe displays.  Ismail, please check that the new version
works for you.

Re: Options menu is broken on CVS

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sat, 10 Sep 2005 01:12:41 +1200
> Cc: ismail@uludag.org.tr, emacs-devel@gnu.org
> 
>  > I think the non-X build on Unix and GNU systems doesn't define
>  > HAVE_MENUS for a reason: without the proper X11 headers and libraries,
>  > xmenu.c simply will not compile.
> 
> I'm not sure what HAVE_MENUS means but non-X systems clearly have menus
> (perhaps you've been tricked!).

HAVE_MENUS means menu support on the C level.  tmm doesn't do that, it
_emulates_ menus on the Lisp level.

> However, xmenu.c _does_ compile without
> X11 headers and libraries, otherwise you couldn't make a non-X build with
> it.

If the headers and libraries are present on the system, you could
build such a version allright.

>  > My interim conclusion is that the changes back in 2004 were not the
>  > one that caused the current problem.
> 
> I think you are wrong.

Well, I think my patch just installed proves that I am right.

>  > Meanwhile, I don't think we should apply your suggested changes, since
>  > compiling xmenu.c on a system without X headers and libraries might
>  > fail due to unresolved externals.
> 
> It solved the OP's problem.  I suggest that we do apply it as thats the only
> way to test it.  If it is wrong, I am sure we will hear about it shortly.

No need, as I fixed that differently.

Re: Options menu is broken on CVS

[David Kastrup]

Eli Zaretskii <eliz@gnu.org> writes:

> The problem is indeed that this change caused the menus to reference
> menu-updating-frame, which is not defined in the non-X version.
>
> I installed a change that avoids referencing that variable on
> non-multiframe displays.  Ismail, please check that the new version
> works for you.

Uh, ttys _are_ multiframe in some manner (one can create and delete
frames).  That's pure Lisp level, then?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Options menu is broken on CVS

[Stefan Monnier]

> The result of all this was that xmenu.o was being linked in both in
> the X and in the non-X branches, and in both cases it was conditioned
> on HAVE_MENUS being defined and HAVE_CARBON not being defined (because
> Carbon uses macmenu.o instead).

Of course, the better solution would be to merge xmenu.c and macmenu.c.


        Stefan

Re: Options menu is broken on CVS

[Eli Zaretskii]

> Cc: nickrob@snap.net.nz,  ismail@uludag.org.tr,  emacs-devel@gnu.org
> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 09 Sep 2005 14:12:04 -0400
> 
> > The result of all this was that xmenu.o was being linked in both in
> > the X and in the non-X branches, and in both cases it was conditioned
> > on HAVE_MENUS being defined and HAVE_CARBON not being defined (because
> > Carbon uses macmenu.o instead).
> 
> Of course, the better solution would be to merge xmenu.c and macmenu.c.

Agreed.  If that's possible, of course (I see that both w32menu.c and
macmenu.c hold quite a lot of code).

Re: Options menu is broken on CVS

[Eli Zaretskii]

> Cc: nickrob@snap.net.nz,  ismail@uludag.org.tr,  emacs-devel@gnu.org
> From: David Kastrup <dak@gnu.org>
> Date: Fri, 09 Sep 2005 17:47:35 +0200
> 
> > I installed a change that avoids referencing that variable on
> > non-multiframe displays.  Ismail, please check that the new version
> > works for you.
> 
> Uh, ttys _are_ multiframe in some manner (one can create and delete
> frames).

Yes, I know (I wrote large parts of that code ;-).  However, I meant
those displays that can show only one frame at a time (those for which
display-multi-frame-p returns nil).

> That's pure Lisp level, then?

No, the support for multiple tty frames is in frame.c (see
Fmake_terminal_frame and friends).

Re: Options menu is broken on CVS

[Nick Roberts]

 > HAVE_MENUS means menu support on the C level.  tmm doesn't do that, it
 > _emulates_ menus on the Lisp level.

OK.  I see now.

 > > However, xmenu.c _does_ compile without
 > > X11 headers and libraries, otherwise you couldn't make a non-X build with
 > > it.
 > 
 > If the headers and libraries are present on the system, you could
 > build such a version allright.

xmenu.c compiles without X libraries (--without-x doesn't link to them) but
now I think that the non-X build on Unix and GNU systems doesn't define
HAVE_MENUS because it shouldn't need xmenu.c

 > >  > Meanwhile, I don't think we should apply your suggested changes, since
 > >  > compiling xmenu.c on a system without X headers and libraries might
 > >  > fail due to unresolved externals.
 > > 
 > > It solved the OP's problem.  I suggest that we do apply it as thats the
 > > only way to test it.  If it is wrong, I am sure we will hear about it
 > > shortly.
 > 
 > No need, as I fixed that differently.

Yes, I think your change is generally right, non-X builds don't seem to use
menu-updating-frame (it's always nil if defined).

However you have missed some references to menu-updating-frame.  For example
F10->f doesn't work beacuse of:

(put 'dired 'menu-enable
     '(not (window-minibuffer-p (frame-selected-window menu-updating-frame))))

You might also need display-multi-frame-p in kill-this-buffer-enabled-p.

Nick

Re: Options menu is broken on CVS

[YAMAMOTO Mitsuharu]

>>>>> On Fri, 09 Sep 2005 16:46:25 +0300, Eli Zaretskii <eliz@gnu.org> said:

> The problem is indeed that this change caused the menus to reference
> menu-updating-frame, which is not defined in the non-X version.

Besides menu-updating-frame, tmm.el uses x-popup-menu (defined in
xmenu.c) in order to obtain keyboard equivalents.

           (condition-case nil
               (x-popup-menu nil choice) ; Get the shortcuts
             (error nil))

Because it is enclosed with condition-case, we don't see any errors
with it.  But keyboard equivalents are not shown with tmm for the
non-X version where xmenu.o is not linked.  I confirmed that they were
shown for the non-X version with the Nick's patch.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: Options menu is broken on CVS

[YAMAMOTO Mitsuharu]

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

>>>>> On Fri, 09 Sep 2005 21:18:45 +0300, Eli Zaretskii <eliz@gnu.org> said:

>> Of course, the better solution would be to merge xmenu.c and
>> macmenu.c.

> Agreed.  If that's possible, of course (I see that both w32menu.c
> and macmenu.c hold quite a lot of code).

The current macmenu.c is based on old xmenu.c (or w32menu.c) code.  I
have some backlog that makes macmenu.c synchronized with xmenu.c.
Shall I install it?

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

[-- Attachment #2: diff-macmenu.gz --]
[-- Type: application/octet-stream, Size: 9262 bytes --]

[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Options menu is broken on CVS

[Richard M. Stallman]

    You can replace the deleted code with the comment (of course, only if the
    location of the comment makes sense: if you delete a whole function at the
    toplevel, replacing it with a toplevel comment won't be much help), or you
    can put the info in the ChangeLog.
    It's a fundamental problem and there's not much we can do about that, really.

I'd rather have explanations in comments in the code than in
ChangeLog.  In the code, it is easier to see.  If the change was to delete
a function, and it needs an explanation, you could put the explanation where
the function used to be called.

In ChangeLog it can be useful to mention the date and sender of the
bug report.  That would be brief, but enough to find the discussion
straightforwardly.

Re: Options menu is broken on CVS

[Eli Zaretskii]

> From: "Richard M. Stallman" <rms@gnu.org>
> CC: eliz@gnu.org, nickrob@snap.net.nz, ismail@uludag.org.tr,
> 	emacs-devel@gnu.org
> Date: Sat, 10 Sep 2005 04:14:23 -0400
> 
> In ChangeLog it can be useful to mention the date and sender of the
> bug report.  That would be brief, but enough to find the discussion
> straightforwardly.

If the discussion was in a public mailing list, a URL of the message
that started the thread would IMHO be a better way of pointing to it.
Unfortunately, some bugs are discussed in private email.

Re: Options menu is broken on CVS

[Eli Zaretskii]

> Date: Sat, 10 Sep 2005 09:08:40 +0900
> From: YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> Cc: nickrob@snap.net.nz, ismail@uludag.org.tr, emacs-devel@gnu.org
> 
> Besides menu-updating-frame, tmm.el uses x-popup-menu (defined in
> xmenu.c) in order to obtain keyboard equivalents.
> 
>            (condition-case nil
>                (x-popup-menu nil choice) ; Get the shortcuts
>              (error nil))
> 
> Because it is enclosed with condition-case, we don't see any errors
> with it.  But keyboard equivalents are not shown with tmm for the
> non-X version where xmenu.o is not linked.  I confirmed that they were
> shown for the non-X version with the Nick's patch.

I don't think it's right to compile and link in xmenu.c on platforms
where menus are not supported on the C level.  If we want to show
keyboard equivalents in tmm, we should find another way of gleaning
them, without calling x-popup-menu.

Re: Options menu is broken on CVS

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sat, 10 Sep 2005 11:07:25 +1200
> Cc: ismail@uludag.org.tr, emacs-devel@gnu.org
> 
> xmenu.c compiles without X libraries (--without-x doesn't link to them)

What you say is not detailed enough, because xmenu.c has 2 different
parts (one with USE_X_TOOLKIT, the other without it), and some
additional preprocessor directives partition the code there still
more, depending on whether you use GTK, Lucid, Motif/LessTif, etc.
Which one of these variants did you try to compile?

At least the non-toolkit version of the functioon xmenu_show (the one
near the end of xmenu.c) uses functions like XMenuCreate,
XMenuAddPane, XMenuDestroy, etc., and constants like XM_SUCCESS and
XM_FAILURE, which will cause failures either during compilation or
during linking, if X headers and libraries are not available/linked
in.

> but now I think that the non-X build on Unix and GNU systems doesn't
> define HAVE_MENUS because it shouldn't need xmenu.c

True.

> However you have missed some references to menu-updating-frame.  For example
> F10->f doesn't work beacuse of:
> 
> (put 'dired 'menu-enable
>      '(not (window-minibuffer-p (frame-selected-window menu-updating-frame))))
> 
> You might also need display-multi-frame-p in kill-this-buffer-enabled-p.

Thanks for catching this, I will fix those ASAP.

Re: Options menu is broken on CVS

[Eli Zaretskii]

> Date: Sat, 10 Sep 2005 12:28:09 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: ismail@uludag.org.tr, emacs-devel@gnu.org
> 
> > However you have missed some references to menu-updating-frame.  For example
> > F10->f doesn't work beacuse of:
> > 
> > (put 'dired 'menu-enable
> >      '(not (window-minibuffer-p (frame-selected-window menu-updating-frame))))
> > 
> > You might also need display-multi-frame-p in kill-this-buffer-enabled-p.
> 
> Thanks for catching this, I will fix those ASAP.

Done.  I also took this opportunity to add to menu-bar.el 2
subroutines that check the menu frame for being live and visible, and
check that the selected window on that frame is not a minibuffer
window.  The `:enable' clauses of the menu bar items now use these two
subroutines instead of in-lining their guts.

Re: Options menu is broken on CVS

[Richard M. Stallman]

    I don't think it's right to compile and link in xmenu.c on platforms
    where menus are not supported on the C level.  If we want to show
    keyboard equivalents in tmm, we should find another way of gleaning
    them, without calling x-popup-menu.

I see no reason to duplicate this code.  If x-popup-menu can operate
at run time, for this limited purpose, without a window system, it
should not be hard to make it do the same job when compiled without a
window system.

Re: Options menu is broken on CVS

[Richard M. Stallman]

    > In ChangeLog it can be useful to mention the date and sender of the
    > bug report.  That would be brief, but enough to find the discussion
    > straightforwardly.

    If the discussion was in a public mailing list, a URL of the message
    that started the thread would IMHO be a better way of pointing to it.

The date and sender are in the message--the URL is not.  It would be
too difficult to find the URLs and put them in the change log.
(Would you want to find the URLs for me?)

Re: Options menu is broken on CVS

[Eli Zaretskii]

> From: "Richard M. Stallman" <rms@gnu.org>
> CC: mituharu@math.s.chiba-u.ac.jp, nickrob@snap.net.nz,
> 	ismail@uludag.org.tr, emacs-devel@gnu.org
> Date: Sun, 11 Sep 2005 02:54:00 -0400
> 
> I see no reason to duplicate this code.  If x-popup-menu can operate
> at run time, for this limited purpose, without a window system, it
> should not be hard to make it do the same job when compiled without a
> window system.

I have no idea if this will work without some non-trivial re-shuffling
of the code in x-popup-menu.  Just looking at the source, it seems to
invoke functions that need X (or its emulation).  Someone will have to
work on refactoring the code before we can do what you suggest.

Re: Options menu is broken on CVS

[Eli Zaretskii]

> From: "Richard M. Stallman" <rms@gnu.org>
> CC: monnier@iro.umontreal.ca, nickrob@snap.net.nz, emacs-devel@gnu.org
> Date: Sun, 11 Sep 2005 02:54:10 -0400
> 
> (Would you want to find the URLs for me?)

Given the date and the Subject, it should be very easy to do that,
using the mail-archive Search command.

Re: Options menu is broken on CVS

[Richard M. Stallman]

    > (Would you want to find the URLs for me?)

    Given the date and the Subject, it should be very easy to do that,
    using the mail-archive Search command.

For me, this is not merely hard, but impossible.  I do my work without
internet access, most of the time.

Re: Options menu is broken on CVS

[Richard M. Stallman]

    I have no idea if this will work without some non-trivial re-shuffling
    of the code in x-popup-menu.  Just looking at the source, it seems to
    invoke functions that need X (or its emulation).  Someone will have to
    work on refactoring the code before we can do what you suggest.

The following changes seem to be enough to make x-popup-menu compile
and link properly.  And the keyboard shortcuts do appear.

Note that no change was needed in xmenu.c itself.  Apparently
someone already prepared that file for such use.


*** emacs.c	27 Aug 2005 22:41:25 -0400	1.374
--- emacs.c	12 Sep 2005 07:28:32 -0400	
***************
*** 1624,1635 ****
  #endif
  #endif /* HAVE_X_WINDOWS */
  
- #ifdef HAVE_MENUS
  #ifndef HAVE_NTGUI
  #ifndef MAC_OS
        /* Called before init_window_once for Mac OS Classic.  */
        syms_of_xmenu ();
- #endif
  #endif
  #endif
  
--- 1624,1633 ----

*** Makefile.in	07 Aug 2005 13:30:34 -0400	1.313
--- Makefile.in	12 Sep 2005 07:37:47 -0400	
***************
*** 310,319 ****
  
  #ifdef HAVE_MENUS
  
- #ifndef HAVE_CARBON
- XMENU_OBJ = xmenu.o
- #endif
- 
  #ifdef USE_GTK
  GTK_OBJ= gtkutil.o
  #endif
--- 310,315 ----
***************
*** 449,457 ****
  LIBX= $(LIBXMENU) LD_SWITCH_X_SITE -lX10 LIBX10_MACHINE LIBX10_SYSTEM
  #endif /* not HAVE_X11 */
  #else /* not HAVE_X_WINDOWS */
- #if defined(HAVE_MENUS) && !defined(HAVE_CARBON)
- XMENU_OBJ = xmenu.o
- #endif
  #endif /* not HAVE_X_WINDOWS */
  
  LIBSOUND= @LIBSOUND@
--- 445,450 ----
***************
*** 577,583 ****
  
  /* lastfile must follow all files
     whose initialized data areas should be dumped as pure by dump-emacs.  */
! obj=    dispnew.o frame.o scroll.o xdisp.o $(XMENU_OBJ) window.o \
  	charset.o coding.o category.o ccl.o \
  	cm.o term.o xfaces.o $(XOBJ) $(GTK_OBJ)\
  	emacs.o keyboard.o macros.o keymap.o sysdep.o \
--- 570,576 ----
  
  /* lastfile must follow all files
     whose initialized data areas should be dumped as pure by dump-emacs.  */
! obj=    dispnew.o frame.o scroll.o xdisp.o xmenu.o window.o \
  	charset.o coding.o category.o ccl.o \
  	cm.o term.o xfaces.o $(XOBJ) $(GTK_OBJ)\
  	emacs.o keyboard.o macros.o keymap.o sysdep.o \
***************
*** 596,602 ****
     These go in the DOC file on all machines
     in case they are needed there.  */
  SOME_MACHINE_OBJECTS = sunfns.o dosfns.o msdos.o \
!   xterm.o xfns.o xmenu.o xselect.o xrdb.o xsmfns.o fringe.o image.o \
    mac.o macterm.o macfns.o macmenu.o macselect.o fontset.o \
    w32.o w32bdf.o w32console.o w32fns.o w32heap.o w32inevt.o \
    w32menu.o w32proc.o w32reg.o w32select.o w32term.o w32xfns.o
--- 589,595 ----
     These go in the DOC file on all machines
     in case they are needed there.  */
  SOME_MACHINE_OBJECTS = sunfns.o dosfns.o msdos.o \
!   xterm.o xfns.o xselect.o xrdb.o xsmfns.o fringe.o image.o \
    mac.o macterm.o macfns.o macmenu.o macselect.o fontset.o \
    w32.o w32bdf.o w32console.o w32fns.o w32heap.o w32inevt.o \
    w32menu.o w32proc.o w32reg.o w32select.o w32term.o w32xfns.o


*** xdisp.c	08 Sep 2005 20:35:28 -0400	1.1050
--- xdisp.c	12 Sep 2005 07:22:30 -0400	
***************
*** 10009,10020 ****
--- 10009,10022 ----
  	  if (FRAME_WINDOW_P (it->f)
  	      && WINDOW_LEFT_FRINGE_WIDTH (it->w) > 0)
  	    {
+ #ifdef HAVE_WINDOW_SYSTEM
  	      if (val = Fget (var, Qoverlay_arrow_bitmap), SYMBOLP (val))
  		{
  		  int fringe_bitmap;
  		  if ((fringe_bitmap = lookup_fringe_bitmap (val)) != 0)
  		    return make_number (fringe_bitmap);
  		}
+ #endif
  	      return make_number (-1); /* Use default arrow bitmap */
  	    }
  	  return overlay_arrow_string_or_property (var);