bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Konrad Podczeck]

To reproduce:

Build a fresh Emacs app.

Start the app. The save icon is grayed out only after a mouse movement (which can be as small as you like).
 

Konrad

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Eli Zaretskii]

> From: Konrad Podczeck <konrad.podczeck@univie.ac.at>
> Date: Fri, 17 Mar 2023 08:55:54 +0100
> 
> To reproduce:
> 
> Build a fresh Emacs app.
> 
> Start the app. The save icon is grayed out only after a mouse movement (which can be as small as you like).

Doesn't happen here.

Please post the system and Emacs configuration information collected
by "M-x report-emacs-bug", so we could see what kind of platform you
use and how was Emacs configured.

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Konrad Podczeck]

The platform is the NS port. Emacs was not configured at al.



> Am 17.03.2023 um 09:27 schrieb Eli Zaretskii <eliz@gnu.org>:
> 
>> From: Konrad Podczeck <konrad.podczeck@univie.ac.at>
>> Date: Fri, 17 Mar 2023 08:55:54 +0100
>> 
>> To reproduce:
>> 
>> Build a fresh Emacs app.
>> 
>> Start the app. The save icon is grayed out only after a mouse movement (which can be as small as you like).
> 
> Doesn't happen here.
> 
> Please post the system and Emacs configuration information collected
> by "M-x report-emacs-bug", so we could see what kind of platform you
> use and how was Emacs configured.

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Eli Zaretskii]

> From: Konrad Podczeck <konrad.podczeck@univie.ac.at>
> Date: Fri, 17 Mar 2023 11:38:48 +0100
> 
> The platform is the NS port.

OK, so maybe this is NS-specific.  Can someone who has access to the
NS port please try reproducing this?

> Emacs was not configured at al.

??? How can this be true?  Emacs needs to be configured before it is
built.  So I think there's a misunderstanding here regarding the
meaning of "configured".  What I meant were the values of
system-configuration-options and system-configuration-features.

Thanks.

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Konrad Podczeck <konrad.podczeck@univie.ac.at>
>> Date: Fri, 17 Mar 2023 11:38:48 +0100
>> 
>> The platform is the NS port.
>
> OK, so maybe this is NS-specific.  Can someone who has access to the
> NS port please try reproducing this?
>

I can certainly reproduce an issue with the toolbar in the NS port.  I
think it's the same issue that the OP is describing.  Here's some steps
to reproduce it:

emacs -Q

The Save and the Cut(Kill) icons are enabled.

Resize the Emacs frame a little bit.

The Save and the Cut(Kill) icons are now disabled, as expected, because
the *scratch* buffer is not backed by a file, and there's no active
region.

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Eli Zaretskii]

> From: Daniel Martín <mardani29@yahoo.es>
> Cc: Konrad Podczeck <konrad.podczeck@univie.ac.at>,  62234@debbugs.gnu.org
> Date: Fri, 17 Mar 2023 14:11:43 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > OK, so maybe this is NS-specific.  Can someone who has access to the
> > NS port please try reproducing this?
> >
> 
> I can certainly reproduce an issue with the toolbar in the NS port.  I
> think it's the same issue that the OP is describing.  Here's some steps
> to reproduce it:
> 
> emacs -Q
> 
> The Save and the Cut(Kill) icons are enabled.
> 
> Resize the Emacs frame a little bit.
> 
> The Save and the Cut(Kill) icons are now disabled, as expected, because
> the *scratch* buffer is not backed by a file, and there's no active
> region.

Can you try figuring out why the button is enabled at startup?

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Daniel Martín <mardani29@yahoo.es> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Konrad Podczeck <konrad.podczeck@univie.ac.at>
>>> Date: Fri, 17 Mar 2023 11:38:48 +0100
>>> 
>>> The platform is the NS port.
>>
>> OK, so maybe this is NS-specific.  Can someone who has access to the
>> NS port please try reproducing this?
>>
>
> I can certainly reproduce an issue with the toolbar in the NS port.  I
> think it's the same issue that the OP is describing.  Here's some steps
> to reproduce it:
>
> emacs -Q
>
> The Save and the Cut(Kill) icons are enabled.
>
> Resize the Emacs frame a little bit.
>
> The Save and the Cut(Kill) icons are now disabled, as expected, because
> the *scratch* buffer is not backed by a file, and there's no active
> region.

If it helps, I don't see this at all on GNUstep.  Both buttons are
initially disabled.

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Po Lu <luangruo@yahoo.com> writes:

>> I can certainly reproduce an issue with the toolbar in the NS port.  I
>> think it's the same issue that the OP is describing.  Here's some steps
>> to reproduce it:
>>
>> emacs -Q
>>
>> The Save and the Cut(Kill) icons are enabled.
>>
>> Resize the Emacs frame a little bit.
>>
>> The Save and the Cut(Kill) icons are now disabled, as expected, because
>> the *scratch* buffer is not backed by a file, and there's no active
>> region.
>
> If it helps, I don't see this at all on GNUstep.  Both buttons are
> initially disabled.

I've browsed some old Apple documentation and I think the way the
toolbar validation is implemented in the NS port may not be totally
correct.
https://web.archive.org/web/20090405012930if_/http://developer.apple.com/documentation/Cocoa/Conceptual/Toolbars/Tasks/ValidatingTBItems.html#//apple_ref/doc/uid/20000753-1036447
states that "If an image item has a valid target/action pair, then the
toolbar will call NSToolbarItemValidation’s validateToolbarItem: on
target if the target implements it; otherwise the item is enabled by
default.".

So I've implemented the above idea in the attached patch and it fixes
the issue for me.  I've also removed the validateVisibleItems override,
which should not be needed (and it's probably incorrect, as Apple's
documentation says that if you override this method, you must call
super).

Could you check if the patch also works fine on GNUStep?  If not,
perhaps we could make the change conditional to Apple systems.  Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-toolbar-item-validation-in-the-NS-port.patch --]
[-- Type: text/x-patch, Size: 1642 bytes --]

From eabab73b0dca6cd7aff67427b4443fd08f3b9bcc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=ADn?= <mardani29@yahoo.es>
Date: Sat, 18 Mar 2023 01:47:32 +0100
Subject: [PATCH] Fix toolbar item validation in the NS port

* src/nsmenu.m ([EmacsToolbar validateVisibleItems]): Remove this
override, which is not really needed.  Also, it doesn't do the right
thing, because Apple's documentation states that this method, if
overriden, must call super.
* src/nsterm.m ([EmacsView validateToolbarItem:]): Implement this
NSToolbarItemValidation method instead, which is called by AppKit when
it needs to validate an image item.
---
 src/nsmenu.m | 8 --------
 src/nsterm.m | 5 +++++
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/src/nsmenu.m b/src/nsmenu.m
index 2c1f575bdf2..f7a401dc407 100644
--- a/src/nsmenu.m
+++ b/src/nsmenu.m
@@ -1346,14 +1346,6 @@ - (void) addDisplayItemWithImage: (EmacsImage *)img
   enablement = (enablement << 1) | (enabled == YES);
 }
 
-/* This overrides super's implementation, which automatically sets
-   all items to enabled state (for some reason).  */
-- (void)validateVisibleItems
-{
-  NSTRACE ("[EmacsToolbar validateVisibleItems]");
-}
-
-
 /* delegate methods */
 
 - (NSToolbarItem *)toolbar: (NSToolbar *)toolbar
diff --git a/src/nsterm.m b/src/nsterm.m
index 0e75cbf3f0f..8bb22c0f756 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -8573,6 +8573,11 @@ - (instancetype)toolbarClicked: (id)item
   return self;
 }
 
+-(BOOL)validateToolbarItem:(NSToolbarItem *)toolbarItem
+{
+    return [toolbarItem isEnabled];
+}
+
 
 - (instancetype)toggleToolbar: (id)sender
 {
-- 
2.34.1

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Daniel Martín <mardani29@yahoo.es> writes:

> So I've implemented the above idea in the attached patch and it fixes
> the issue for me.  I've also removed the validateVisibleItems override,
> which should not be needed (and it's probably incorrect, as Apple's
> documentation says that if you override this method, you must call
> super).
>
> Could you check if the patch also works fine on GNUStep?  If not,
> perhaps we could make the change conditional to Apple systems.  Thanks.
>
> From eabab73b0dca6cd7aff67427b4443fd08f3b9bcc Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Daniel=20Mart=C3=ADn?= <mardani29@yahoo.es>
> Date: Sat, 18 Mar 2023 01:47:32 +0100
> Subject: [PATCH] Fix toolbar item validation in the NS port
>
> * src/nsmenu.m ([EmacsToolbar validateVisibleItems]): Remove this
> override, which is not really needed.  Also, it doesn't do the right
> thing, because Apple's documentation states that this method, if
> overriden, must call super.
> * src/nsterm.m ([EmacsView validateToolbarItem:]): Implement this
> NSToolbarItemValidation method instead, which is called by AppKit when
> it needs to validate an image item.
> ---
>  src/nsmenu.m | 8 --------
>  src/nsterm.m | 5 +++++
>  2 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/src/nsmenu.m b/src/nsmenu.m
> index 2c1f575bdf2..f7a401dc407 100644
> --- a/src/nsmenu.m
> +++ b/src/nsmenu.m
> @@ -1346,14 +1346,6 @@ - (void) addDisplayItemWithImage: (EmacsImage *)img
>    enablement = (enablement << 1) | (enabled == YES);
>  }
>  
> -/* This overrides super's implementation, which automatically sets
> -   all items to enabled state (for some reason).  */
> -- (void)validateVisibleItems
> -{
> -  NSTRACE ("[EmacsToolbar validateVisibleItems]");
> -}
> -

I think this needs to be kept on GNUstep.

>  /* delegate methods */
>  
>  - (NSToolbarItem *)toolbar: (NSToolbar *)toolbar
> diff --git a/src/nsterm.m b/src/nsterm.m
> index 0e75cbf3f0f..8bb22c0f756 100644
> --- a/src/nsterm.m
> +++ b/src/nsterm.m
> @@ -8573,6 +8573,11 @@ - (instancetype)toolbarClicked: (id)item
>    return self;
>  }
>  
> +-(BOOL)validateToolbarItem:(NSToolbarItem *)toolbarItem
> +{
> +    return [toolbarItem isEnabled];
> +}
> +

This does no harm on GNUstep.  However, please indent with two spaces,
and place spaces after -, :, and ):

- (BOOL) validateToolbarItem: (NSToolBarItem *) toolbarItem
{
  return [toolbarItem isEnabled];
}

Otherwise, LGTM.  Thanks.

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Po Lu <luangruo@yahoo.com> writes:

>> +-(BOOL)validateToolbarItem:(NSToolbarItem *)toolbarItem
>> +{
>> +    return [toolbarItem isEnabled];
>> +}
>> +
>
> This does no harm on GNUstep.  However, please indent with two spaces,
> and place spaces after -, :, and ):
>
> - (BOOL) validateToolbarItem: (NSToolBarItem *) toolbarItem
> {
>   return [toolbarItem isEnabled];
> }
>
> Otherwise, LGTM.  Thanks.

OK, I've attached a new version of the patch.  Konrad, could you check
if with this patch the Emacs toolbar works as you expect?  Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-toolbar-item-validation-in-the-NS-port.patch --]
[-- Type: text/x-patch, Size: 833 bytes --]

From b994036d9475e2ae4f4adea8b3a29eff054e354a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=ADn?= <mardani29@yahoo.es>
Date: Sat, 18 Mar 2023 01:47:32 +0100
Subject: [PATCH] Fix toolbar item validation in the NS port

* src/nsterm.m ([EmacsView validateToolbarItem:]): Implement this
NSToolbarItemValidation method, which is called by AppKit when it
needs to validate an image item in a toolbar.
---
 src/nsterm.m | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/nsterm.m b/src/nsterm.m
index 0e75cbf3f0f..46007ec4fcb 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -8573,6 +8573,10 @@ - (instancetype)toolbarClicked: (id)item
   return self;
 }
 
+- (BOOL) validateToolbarItem: (NSToolbarItem *) toolbarItem
+{
+  return [toolbarItem isEnabled];
+}
 
 - (instancetype)toggleToolbar: (id)sender
 {
-- 
2.34.1

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Konrad Podczeck]

After playing around a little bit, I confirm that Emacs now works as expected.

Thanks for your effort,

konrad

> Am 18.03.2023 um 11:25 schrieb Daniel Martín <mardani29@yahoo.es>:
> 
> Po Lu <luangruo@yahoo.com> writes:
> 
>>> +-(BOOL)validateToolbarItem:(NSToolbarItem *)toolbarItem
>>> +{
>>> +    return [toolbarItem isEnabled];
>>> +}
>>> +
>> 
>> This does no harm on GNUstep.  However, please indent with two spaces,
>> and place spaces after -, :, and ):
>> 
>> - (BOOL) validateToolbarItem: (NSToolBarItem *) toolbarItem
>> {
>>  return [toolbarItem isEnabled];
>> }
>> 
>> Otherwise, LGTM.  Thanks.
> 
> OK, I've attached a new version of the patch.  Konrad, could you check
> if with this patch the Emacs toolbar works as you expect?  Thanks.
> 
> <0001-Fix-toolbar-item-validation-in-the-NS-port.patch>

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Eli Zaretskii]

> From: Daniel Martín <mardani29@yahoo.es>
> Cc: Eli Zaretskii <eliz@gnu.org>,  62234@debbugs.gnu.org,  Konrad Podczeck
>  <konrad.podczeck@univie.ac.at>
> Date: Sat, 18 Mar 2023 11:25:51 +0100
> 
> Po Lu <luangruo@yahoo.com> writes:
> 
> >> +-(BOOL)validateToolbarItem:(NSToolbarItem *)toolbarItem
> >> +{
> >> +    return [toolbarItem isEnabled];
> >> +}
> >> +
> >
> > This does no harm on GNUstep.  However, please indent with two spaces,
> > and place spaces after -, :, and ):
> >
> > - (BOOL) validateToolbarItem: (NSToolBarItem *) toolbarItem
> > {
> >   return [toolbarItem isEnabled];
> > }
> >
> > Otherwise, LGTM.  Thanks.
> 
> OK, I've attached a new version of the patch.  Konrad, could you check
> if with this patch the Emacs toolbar works as you expect?  Thanks.

Thanks, installed on the emacs-29 branch, and closing the bug.

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Konrad Podczeck]

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

Thanks!

Konrad

> Am 23.03.2023 um 09:10 schrieb Eli Zaretskii <eliz@gnu.org>:
> 
>> From: Daniel Martín <mardani29@yahoo.es <mailto:mardani29@yahoo.es>>
>> Cc: Eli Zaretskii <eliz@gnu.org <mailto:eliz@gnu.org>>,  62234@debbugs.gnu.org <mailto:62234@debbugs.gnu.org>,  Konrad Podczeck
>> <konrad.podczeck@univie.ac.at <mailto:konrad.podczeck@univie.ac.at>>
>> Date: Sat, 18 Mar 2023 11:25:51 +0100
>> 
>> Po Lu <luangruo@yahoo.com> writes:
>> 
>>>> +-(BOOL)validateToolbarItem:(NSToolbarItem *)toolbarItem
>>>> +{
>>>> +    return [toolbarItem isEnabled];
>>>> +}
>>>> +
>>> 
>>> This does no harm on GNUstep.  However, please indent with two spaces,
>>> and place spaces after -, :, and ):
>>> 
>>> - (BOOL) validateToolbarItem: (NSToolBarItem *) toolbarItem
>>> {
>>>  return [toolbarItem isEnabled];
>>> }
>>> 
>>> Otherwise, LGTM.  Thanks.
>> 
>> OK, I've attached a new version of the patch.  Konrad, could you check
>> if with this patch the Emacs toolbar works as you expect?  Thanks.
> 
> Thanks, installed on the emacs-29 branch, and closing the bug.


[-- Attachment #2: Type: text/html, Size: 3173 bytes --]

bug#62234: Toolbar does not work properly (with Emacs 29 or 30)

[Konrad Podczeck]

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



>> Emacs was not configured at al.
> 
> ??? How can this be true?  Emacs needs to be configured before it is
> built.  So I think there's a misunderstanding here regarding the
> meaning of "configured".  What I meant were the values of
> system-configuration-options and system-configuration-features.
> 
After download, I did

./autogen.sh

./configure 

make

make install.

Resulted in:

In GNU Emacs 29.0.60 (build 1, aarch64-apple-darwin22.3.0, NS
 appkit-2299.40 Version 13.2.1 (Build 22D68)) of 2023-03-17 built on
 iMac-von-Konrad
Repository revision: a2222b9a9bfa039d66f836f06762ddea1544df11
Repository branch: emacs-29
Windowing system distributor 'Apple', version 10.3.2299
System Description:  macOS 13.2.1

Configured features:
ACL DBUS GNUTLS JSON LCMS2 LIBXML2 MODULES NOTIFY KQUEUE NS PDUMPER PNG
SQLITE3 THREADS TOOLKIT_SCROLL_BARS TREE_SITTER WEBP ZLIB

Important settings:
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  blink-cursor-mode: t
  buffer-read-only: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message yank-media puny dired
dired-loaddefs rfc822 mml mml-sec epa derived epg rfc6068 epg-config
gnus-util text-property-search time-date mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils company-autoloads
auctex-autoloads tex-site info dash-autoloads s-autoloads
parsebib-autoloads pdf-tools-autoloads package browse-url url url-proxy
url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util mailcap url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs password-cache json subr-x
map byte-opt gv bytecomp byte-compile url-vars cl-loaddefs cl-lib rmc
iso-transl tooltip cconv eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/ns-win ns-win
ucs-normalize mule-util term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind kqueue cocoa ns
lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 63746 4764)
 (symbols 48 7643 0)
 (strings 32 22662 1921)
 (string-bytes 1 694177)
 (vectors 16 15938)
 (vector-slots 8 221783 10774)
 (floats 8 27 32)
 (intervals 56 221 0)
 (buffers 984 11))


[-- Attachment #2: Type: text/html, Size: 15803 bytes --]