GNU bug report logs -
#50752
28.0.50; easy-menu-define lowers the menu-bar key
Previous Next
Reported by: "Shuguang Sun" <shuguang79 <at> qq.com>
Date: Thu, 23 Sep 2021 08:47:02 UTC
Severity: normal
Tags: patch
Found in version 28.0.50
Fixed in version 28.1
Done: Stefan Kangas <stefan <at> marxist.se>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 50752 in the body.
You can then email your comments to 50752 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 23 Sep 2021 08:47:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
"Shuguang Sun" <shuguang79 <at> qq.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Thu, 23 Sep 2021 08:47:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi,
I'm using the package pdf-tools to view PDF files. However the popu-menu does't work in recent emacs snapshot. I find it is due the automaticly lowering the menu-bar by easy-menu-deine. Is it a feature or bug?
Of pdf-tools (from melpa or from http://github.com/vedang/pdf-tools/), in the pdf-misc.el it defines
```
(easy-menu-define nil pdf-misc-menu-bar-minor-mode-map
"Menu for PDF Tools."
`("PDF Tools"
["Go Backward" pdf-history-backward
:visible (bound-and-true-p pdf-history-minor-mode)
:active (and (bound-and-true-p pdf-history-minor-mode)
(not (pdf-history-end-of-history-p)))]
```
And the responding function
```
(defun pdf-misc-popup-context-menu (event)
"Popup a context menu at position determined by EVENT."
(interactive "@e")
(popup-menu
(cons 'keymap
(cddr (lookup-key pdf-misc-menu-bar-minor-mode-map
[menu-bar PDF\ Tools])))))
```
However, it can't find the key of "PDF\ Tools". Instead, I find in the `pdf-misc-menu-bar-minor-mode-map`, that,
```
(keymap
(menu-bar keymap
(pdf\ tools menu-item #1="PDF Tools"
(keymap #1#
(Go\ Backward menu-item "Go Backward" pdf-history-backward :enable
```
It becomes lower case (`pdf\ tools`).
If I change the case, the function works again:
```(defun pdf-misc-popup-context-menu (event)
"Popup a context menu at position determined by EVENT."
(interactive "@e")
(popup-menu
(cons 'keymap
(cddr (lookup-key pdf-misc-menu-bar-minor-mode-map
[menu-bar pdf\ tools])))))
```
Best Regards,
Shuguang Sun
In GNU Emacs 28.0.50 (build 1, x86_64-w64-mingw32)
of 2021-09-21 built on YJ190169-SSG
Repository revision: 5b962a7ad8d0acfe40a41ce139059b9c8e46f666
Repository branch: master
Windowing system distributor 'Microsoft Corp.', version 10.0.19043
System Description: Microsoft Windows 10 Pro (v10.0.2009.19043.1237)
Configured using:
'configure --without-pop --with-native-image-api
--with-native-compilation --without-compress-install
'--program-transform-name=s/^ctags$/ctags.emacs/''
Configured features:
ACL DBUS GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES
NATIVE_COMP NOTIFY W32NOTIFY PDUMPER PNG RSVG SOUND THREADS TIFF
TOOLKIT_SCROLL_BARS XPM ZLIB
[Message part 2 (text/html, inline)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 23 Sep 2021 17:16:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> However, it can't find the key of "PDF\ Tools". Instead, I find in the
> `pdf-misc-menu-bar-minor-mode-map`, that,
>
> It becomes lower case (`pdf\ tools`).
It seems this problem is caused by the recent commit
e5392d38ac27c4cf1674997ab38a453877e65109.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 23 Sep 2021 21:46:01 GMT)
Full text and
rfc822 format available.
Message #11 received at 50752 <at> debbugs.gnu.org (full text, mbox):
"Shuguang Sun" <shuguang79 <at> qq.com> writes:
> If I change the case, the function works again:
> ```(defun pdf-misc-popup-context-menu (event)
> "Popup a context menu at position determined by EVENT."
> (interactive "@e")
> (popup-menu
> (cons 'keymap
> (cddr (lookup-key pdf-misc-menu-bar-minor-mode-map
> [menu-bar pdf\ tools])))))
Yes, the names have changed in Emacs 28, so external packages that alter
the menus like this have to be adjusted. But I wonder whether we could
make `lookup-key' be case-insensitive in this case...
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 23 Sep 2021 22:29:01 GMT)
Full text and
rfc822 format available.
Message #14 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Previous discussion: https://lists.gnu.org/r/emacs-devel/2021-03/msg00031.html
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 12 Oct 2021 22:24:02 GMT)
Full text and
rfc822 format available.
Message #17 received at 50752 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
tags 50752 + patch
thanks
Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> Yes, the names have changed in Emacs 28, so external packages that alter
> the menus like this have to be adjusted. But I wonder whether we could
> make `lookup-key' be case-insensitive in this case...
As Glenn points out, we discussed this here:
https://lists.gnu.org/r/emacs-devel/2021-03/msg00031.html
We also discussed it here:
https://lists.gnu.org/r/emacs-devel/2021-03/msg00014.html
I had an incomplete patch in the works, but I dropped the ball here.
Please find attached an updated and complete patch that should fix the
above issue. In addition to the new tests, I have tested it locally,
and without the patch, I can reproduce the issue reported here by
Shuguang, i.e. I get this error:
popup-menu: Empty menu
With the patch, the menu is correctly displayed, as is expected. This
is tested with pdf-tools-20211004.514 installed from MELPA, which should
be equivalent to the latest version in the git repository at:
http://github.com/vedang/pdf-tools
Review and additional testing is very welcome.
[0001-Be-more-allowing-when-looking-for-menu-bar-items.patch (text/x-diff, attachment)]
Added tag(s) patch.
Request was from
Stefan Kangas <stefan <at> marxist.se>
to
control <at> debbugs.gnu.org.
(Tue, 12 Oct 2021 22:24:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 13 Oct 2021 11:29:01 GMT)
Full text and
rfc822 format available.
Message #22 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Stefan Kangas <stefan <at> marxist.se> writes:
> Review and additional testing is very welcome.
Looks good to me. I've tested it now in Emacs 28, and I don't see any
regressions.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 13 Oct 2021 12:00:03 GMT)
Full text and
rfc822 format available.
Message #25 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Tue, 12 Oct 2021 15:22:59 -0700
> Cc: Shuguang Sun <shuguang79 <at> qq.com>, 50752 <at> debbugs.gnu.org
>
> + for (int i = 0; i < key_len; i++)
> + {
> + Lisp_Object lc_key = Fdowncase (Fsymbol_name (AREF (key, i)));
> + ASET (new_key, i, Fintern (lc_key, Qnil));
> + }
Beware: downcase uses the current buffer's case-table. Is that
something we want here, or could it be undesirable in some cases?
> + found = lookup_key_1 (keymap, new_key, accept_default);
> +
> + if (!NILP (found) && !NUMBERP (found))
> + goto end;
> +
> + /* If we still don't have a match, let's convert any spaces in
> + our lowercased string into dashes, e.g. "foo bar baz" to
> + "foo-bar-baz". */
> + for (int i = 0; i < key_len; i++)
> + {
> + Lisp_Object lc_key = Fdowncase (Fsymbol_name (AREF (key, i)));
Can't we reuse the results of the original downcasing, instead of
doing that again?
> + USE_SAFE_ALLOCA;
> + ptrdiff_t size = SCHARS (lc_key), n;
> + if (INT_MULTIPLY_WRAPV (size, MAX_MULTIBYTE_LENGTH, &n))
> + n = PTRDIFF_MAX;
> + unsigned char *dst = SAFE_ALLOCA (n);
> + unsigned char *o = dst;
> + ptrdiff_t j = 0, j_byte = 0, chars = 0;
> +
> + while (j < SCHARS (lc_key))
> + {
> + int ch = fetch_string_char_advance (lc_key, &j, &j_byte);
> + if (ch == ' ')
> + *o = '-';
> + else
> + *o = ch;
> + chars++;
This will only work with plain-ASCII characters in lc_key (but then
you don't need fetch_string_char_advance, you can access the bytes one
by one). You need to use CHAR_STRING instead.
> + int len;
> + string_char_and_length (o, &len);
> + o += len;
This is overhead. You already know the length of the multibyte
string, because fetch_string_char_advance reports it back to you via
j_byte. So just use that.
> diff --git a/test/src/keymap-tests.el b/test/src/keymap-tests.el
> index 68b42c346c..8f3dff2acb 100644
> --- a/test/src/keymap-tests.el
> +++ b/test/src/keymap-tests.el
> @@ -124,6 +124,17 @@ keymap-lookup-key/too-long
> ;; (ert-deftest keymap-lookup-key/accept-default ()
> ;; ...)
>
> +(ert-deftest keymap-lookup-key/mixed-case ()
> + (let ((map (make-keymap)))
> + (define-key map [menu-bar foo bar] 'foo)
> + (should (eq (lookup-key map [menu-bar foo bar]) 'foo))
> + (should (eq (lookup-key map [menu-bar Foo Bar]) 'foo))))
> +
> +(ert-deftest subr-test-lookup-keymap/with-spaces ()
> + (let ((map (make-keymap)))
> + (define-key map [menu-bar foo-bar] 'foo)
> + (should (eq (lookup-key map [menu-bar Foo\ Bar]) 'foo))))
> +
> (ert-deftest describe-buffer-bindings/header-in-current-buffer ()
> "Header should be inserted into the current buffer.
> https://debbugs.gnu.org/39149#31"
Please add tests where the symbols use non-ASCII characters.
Also, what about the existing calls to Flookup_key from C: do they all
need to go through the added processing, or could some of them be
satisfied by calling lookup_key_1?
This change needs a NEWS entry.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 13 Oct 2021 12:05:01 GMT)
Full text and
rfc822 format available.
Message #28 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Beware: downcase uses the current buffer's case-table. Is that
> something we want here, or could it be undesirable in some cases?
An upper case I in Turkish would be translated to ı, which would not be
what we wanted, I think.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 13 Oct 2021 12:20:02 GMT)
Full text and
rfc822 format available.
Message #31 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> An upper case I in Turkish would be translated to ı, which would not be
> what we wanted, I think.
Could you explain why you think that?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 13 Oct 2021 12:59:02 GMT)
Full text and
rfc822 format available.
Message #34 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Stefan Kangas <stefan <at> marxist.se> writes:
> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
>
>> An upper case I in Turkish would be translated to ı, which would not be
>> what we wanted, I think.
>
> Could you explain why you think that?
If the menu name the package tried to change was
"PIF Tools"
it's now `pif tools' after being changed to use easy-menu-define, I
think? So the downcase would work fine for everybody -- except people
that have a Turkish locale, where the downcase would yield `pıf tools',
and we'd have no match after all.
I think.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 13 Oct 2021 15:27:02 GMT)
Full text and
rfc822 format available.
Message #37 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> it's now `pif tools' after being changed to use easy-menu-define, I
> think? So the downcase would work fine for everybody -- except people
> that have a Turkish locale, where the downcase would yield `pıf tools',
> and we'd have no match after all.
I didn't see that we were discussing a regular "I" character (bad font
in this client), but yes that is obviously incorrect.
Do we have an alternative to downcase, or should we just ensure that
it uses a standard case-table? Could that lead to any other problems?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 13 Oct 2021 15:43:02 GMT)
Full text and
rfc822 format available.
Message #40 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Stefan Kangas <stefan <at> marxist.se> writes:
> Do we have an alternative to downcase, or should we just ensure that
> it uses a standard case-table? Could that lead to any other problems?
But then non-ASCII characters wouldn't downcase correctly. :-)
Since we're just trying to be backwards compatible, perhaps it would
make sense to try downcase twice -- once with the current case-table and
once with the standard one and see whether either matches?
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 13 Oct 2021 16:10:02 GMT)
Full text and
rfc822 format available.
Message #43 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Wed, 13 Oct 2021 17:26:33 +0200
> Cc: Eli Zaretskii <eliz <at> gnu.org>, Shuguang Sun <shuguang79 <at> qq.com>, 50752 <at> debbugs.gnu.org
>
> Do we have an alternative to downcase, or should we just ensure that
> it uses a standard case-table? Could that lead to any other problems?
We could use the equivalent of
(get-char-code-property ?I 'lowercase)
If the above returns nil, it means the lower-case variant is the
character itself.
In C, this means to use uniprop_table, like bidi.c and casefiddle.c
do. This accesses the database generated from UnicodeData.txt.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Fri, 15 Oct 2021 06:00:01 GMT)
Full text and
rfc822 format available.
Message #46 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> Date: Wed, 13 Oct 2021 14:59:03 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: shuguang79 <at> qq.com, larsi <at> gnus.org, 50752 <at> debbugs.gnu.org
>
> > + USE_SAFE_ALLOCA;
> > + ptrdiff_t size = SCHARS (lc_key), n;
> > + if (INT_MULTIPLY_WRAPV (size, MAX_MULTIBYTE_LENGTH, &n))
> > + n = PTRDIFF_MAX;
> > + unsigned char *dst = SAFE_ALLOCA (n);
> > + unsigned char *o = dst;
> > + ptrdiff_t j = 0, j_byte = 0, chars = 0;
> > +
> > + while (j < SCHARS (lc_key))
> > + {
> > + int ch = fetch_string_char_advance (lc_key, &j, &j_byte);
> > + if (ch == ' ')
> > + *o = '-';
> > + else
> > + *o = ch;
> > + chars++;
>
> This will only work with plain-ASCII characters in lc_key (but then
> you don't need fetch_string_char_advance, you can access the bytes one
> by one). You need to use CHAR_STRING instead.
Thinking more about this, you don't need all these complications with
fetch_string_char_advance and CHAR_STRING. Since all you need is
replace ' ' with '-', you can walk the string data byte by byte,
because UTF-8 encoding makes sure no other byte of any multibyte
sequence will ever include a 7-bit byte equal to an ASCII single-byte
character. So just checking the bytes for equality to ' ' is enough.
Thus, you could make a copy of the symbol's name, then walk that copy
byte by byte looking for space characters and replacing them.
Moreover, you could check up front, using 'strstr', whether the
symbol's name includes any space characters, and if not, short-circuit
the entire second attempt.
These two measures should make the code faster and easier to program
and understand.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Fri, 15 Oct 2021 18:35:01 GMT)
Full text and
rfc822 format available.
Message #49 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> These two measures should make the code faster and easier to program
> and understand.
Thanks, this really helps!
I plan to continue looking into the issue this weekend.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 03:20:02 GMT)
Full text and
rfc822 format available.
Message #52 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Also, what about the existing calls to Flookup_key from C: do they all
> need to go through the added processing, or could some of them be
> satisfied by calling lookup_key_1?
AFAICT, they need to go through the added processing, as the items could
be extended menu items in all cases.
(I will send separately a new patch that should fix the rest of your
comments.)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 03:23:02 GMT)
Full text and
rfc822 format available.
Message #55 received at 50752 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> Stefan Kangas <stefan <at> marxist.se> writes:
>
>> Do we have an alternative to downcase, or should we just ensure that
>> it uses a standard case-table? Could that lead to any other problems?
>
> But then non-ASCII characters wouldn't downcase correctly. :-)
>
> Since we're just trying to be backwards compatible, perhaps it would
> make sense to try downcase twice -- once with the current case-table and
> once with the standard one and see whether either matches?
So I've tried this approach in the attached patch, but I couldn't get it
to work. I'm probably doing something wrong, given that I've never so
much as glanced at language environments and case tables before this.
Eli Zaretskii <eliz <at> gnu.org> writes:
> We could use the equivalent of
>
> (get-char-code-property ?I 'lowercase)
>
> If the above returns nil, it means the lower-case variant is the
> character itself.
>
> In C, this means to use uniprop_table, like bidi.c and casefiddle.c
> do. This accesses the database generated from UnicodeData.txt.
I didn't try this approach, mostly because it sounds more difficult to
implement than what Lars said. I think? Wouldn't it amount to
basically re-implementing Fdowncase? Sorry, I didn't look too closely
at this. Perhaps this would be the better approach.
If anyone has any preferences or further ideas here, that would be much
appreciated, otherwise I'll keep investigating.
The attached patch is what I have so far. It's obviously not yet
finished, but all tests pass except for the one for "I->i" conversion in
the Turkish language environment.
[0001-Be-more-allowing-when-looking-for-menu-bar-items.patch (text/x-diff, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 03:41:01 GMT)
Full text and
rfc822 format available.
Message #58 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Stefan Kangas <stefan <at> marxist.se> writes:
> The attached patch is what I have so far. It's obviously not yet
> finished, but all tests pass except for the one for "I->i" conversion in
> the Turkish language environment.
I set up a Turkish lang environment to test, and I'm just getting very
odd results all over from downcase.
After generating the locale, I said:
LANG=tr_TR.UTF-8 ./src/emacs -Q
and
(downcase "İ i I ı")
=> "i̇ i ı ı"
(upcase "İ i I ı")
=> "İ İ I I"
So everything here looks totally fine. I is downcased to ı as supposed.
But look!
(downcase "I")
=> "1"
Here it's downcasing
name: LATIN CAPITAL LETTER I
to
name: DIGIT ONE
!?
But if I put something else that's non-ASCII into the string, then it
downcases correctly:
(downcase "I é")
=> "ı é"
So if the string is multibyte, it downcases correctly, but if not, it...
turns a capital I into the digit 1?
(downcase "ABCDEFGHIJKLMNOPQRSTUVWXYZ")
=> "abcdefgh1jklmnopqrstuvwxyz"
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 03:53:01 GMT)
Full text and
rfc822 format available.
Message #61 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> So if the string is multibyte, it downcases correctly, but if not, it...
> turns a capital I into the digit 1?
>
> (downcase "ABCDEFGHIJKLMNOPQRSTUVWXYZ")
> => "abcdefgh1jklmnopqrstuvwxyz"
I guess it's from this:
static Lisp_Object
do_casify_unibyte_string (struct casing_context *ctx, Lisp_Object obj)
{
ptrdiff_t i, size = SCHARS (obj);
int ch, cased;
obj = Fcopy_sequence (obj);
for (i = 0; i < size; i++)
{
ch = make_char_multibyte (SREF (obj, i));
cased = case_single_character (ctx, ch);
if (ch == cased)
continue;
cased = make_char_unibyte (cased);
/* If the char can't be converted to a valid byte, just don't
change it. */
if (SINGLE_BYTE_CHAR_P (cased))
SSET (obj, i, cased);
}
return obj;
}
Which just looks wrong to me. The logic in Fdowncase seems to assume
that a unibyte string can never be downcased to a multibyte one, but
that make_char_unibyte is picking out the lowest eight bits of the char
we've downcased to and puts that into the downcased string.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 11:44:02 GMT)
Full text and
rfc822 format available.
Message #64 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Mon, 18 Oct 2021 20:22:18 -0700
> Cc: Eli Zaretskii <eliz <at> gnu.org>, Shuguang Sun <shuguang79 <at> qq.com>, 50752 <at> debbugs.gnu.org
>
> If anyone has any preferences or further ideas here, that would be much
> appreciated, otherwise I'll keep investigating.
>
> The attached patch is what I have so far. It's obviously not yet
> finished, but all tests pass except for the one for "I->i" conversion in
> the Turkish language environment.
Don't give up, you are close.
> + memcpy (dst, SSDATA (lc_key), SBYTES (lc_key));
> + for (int i = 0; i < SBYTES (lc_key); ++i)
> + {
> + if (*(dst + i) == ' ')
> + *(dst + i) = '-';
> + }
If you want to use an index to walk the string data, as you did above,
please use dst[i] instead of *(dst + i); the latter is correct, but
ugly and un-C-ish. Or you could use a pointer to walk, like this:
unsigned char *p = dst, *dst_end = dst + SBYTES (lc_key);
for ( ; p < dst_end; p++)
{
if (*p == ' ')
*p = '-';
}
> > We could use the equivalent of
> >
> > (get-char-code-property ?I 'lowercase)
> >
> > If the above returns nil, it means the lower-case variant is the
> > character itself.
> >
> > In C, this means to use uniprop_table, like bidi.c and casefiddle.c
> > do. This accesses the database generated from UnicodeData.txt.
>
> I didn't try this approach, mostly because it sounds more difficult to
> implement than what Lars said. I think? Wouldn't it amount to
> basically re-implementing Fdowncase? Sorry, I didn't look too closely
> at this. Perhaps this would be the better approach.
It shouldn't be hard. You need to call uniprop_table to get a
char-table:
Lisp_Object unicode_case_table = uniprop_table (intern ("lowercase"));
which you then reference with
int low_ch = XFIXNUM (CHAR_TABLE_REF (unicode_case_table, ch));
to get the codepoint of the lower-case character that corresponds to
the (possibly upper-case) character whose codepoint is CH. Then
downcasing a string boils down to a loop that fetches characters one
by one with fetch_string_char_advance and then stores the lower-case
characters, obtained as above, with CHAR_STRING. (It's a bit more
complicated than that, because CHAR_TABLE_REF can return nil for the
characters that are either already lower-case or don't have case
variants. And the uniprop_table call should be done once, at startup
time, or upon first usage, and stored in a staticpro'd variable, see
bidi_initialize for an example.)
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 11:57:01 GMT)
Full text and
rfc822 format available.
Message #67 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Date: Tue, 19 Oct 2021 05:52:15 +0200
> Cc: Shuguang Sun <shuguang79 <at> qq.com>, 50752 <at> debbugs.gnu.org
>
> Which just looks wrong to me. The logic in Fdowncase seems to assume
> that a unibyte string can never be downcased to a multibyte one, but
> that make_char_unibyte is picking out the lowest eight bits of the char
> we've downcased to and puts that into the downcased string.
What would you do instead? It's a conundrum with no easy solutions.
The original string could include non-ASCII bytes.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 12:09:01 GMT)
Full text and
rfc822 format available.
Message #70 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> What would you do instead? It's a conundrum with no easy solutions.
> The original string could include non-ASCII bytes.
First of all, I think the code is just plain buggy:
for (i = 0; i < size; i++)
{
ch = make_char_multibyte (SREF (obj, i));
cased = case_single_character (ctx, ch);
if (ch == cased)
continue;
cased = make_char_unibyte (cased);
/* If the char can't be converted to a valid byte, just don't
change it. */
if (SINGLE_BYTE_CHAR_P (cased))
SSET (obj, i, cased);
}
That make_char_unibyte makes SINGLE_BYTE_CHAR_P always return true,
doesn't it? Uhm... No, what happens to raw bytes... Oh, yeah,
case_single_character doesn't work for raw bytes, so the "continue" is
always taken for those.
So the code is all kinds of confused, I think.
Second of all -- to handle this specific, very unusual case (i.e.,
downcasing a ascii character gets us a non-ascii character), when we
detect this issue, we should just abort this loop and call
do_casify_multibyte_string instead.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 12:18:02 GMT)
Full text and
rfc822 format available.
Message #73 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> /* If the char can't be converted to a valid byte, just don't
> change it. */
> if (SINGLE_BYTE_CHAR_P (cased))
> SSET (obj, i, cased);
[...]
> So the code is all kinds of confused, I think.
(I.e., I think the probable intention here is that when downcasing
something, and you get something that's multibyte, you shouldn't alter
the string -- but it does alter the string: It makes the "I" into the
digit "1".)
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 12:38:02 GMT)
Full text and
rfc822 format available.
Message #76 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: stefan <at> marxist.se, shuguang79 <at> qq.com, 50752 <at> debbugs.gnu.org
> Date: Tue, 19 Oct 2021 14:07:59 +0200
>
> Second of all -- to handle this specific, very unusual case (i.e.,
> downcasing a ascii character gets us a non-ascii character), when we
> detect this issue, we should just abort this loop and call
> do_casify_multibyte_string instead.
You mean, you want to special-case Turkish (and a couple of similar)
locale? That might be possible, but the more general problem will
still be left more or less intact. And then we need to consider how
many places don't expect to get multibyte string when they downcase a
unibyte one.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 12:46:01 GMT)
Full text and
rfc822 format available.
Message #79 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> You mean, you want to special-case Turkish (and a couple of similar)
> locale?
Nope. The loop can just check whether we get a multibyte result when
downcasing unibyte (ASCII), and then defer to the multibyte version to
do the computation.
This will basically only happen when using a Turkish (and a couple
similar) locales, though, so the general performance impact will be
approximately nil.
> And then we need to consider how many places don't expect to get
> multibyte string when they downcase a unibyte one.
That's a consideration, but I don't think we'll find out until we try.
(My guess is that nothing will be affected.)
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 13:25:01 GMT)
Full text and
rfc822 format available.
Message #82 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Lars Ingebrigtsen <larsi <at> gnus.org> writes:
> Nope. The loop can just check whether we get a multibyte result when
> downcasing unibyte (ASCII), and then defer to the multibyte version to
> do the computation.
I.e., this change, which even passes all the case tests:
diff --git a/src/casefiddle.c b/src/casefiddle.c
index a7a2541490..b46d42f5e2 100644
--- a/src/casefiddle.c
+++ b/src/casefiddle.c
@@ -310,11 +310,12 @@ do_casify_unibyte_string (struct casing_context *ctx, Lisp_Object obj)
cased = case_single_character (ctx, ch);
if (ch == cased)
continue;
- cased = make_char_unibyte (cased);
- /* If the char can't be converted to a valid byte, just don't
- change it. */
- if (SINGLE_BYTE_CHAR_P (cased))
- SSET (obj, i, cased);
+ /* If downcasing changed an ASCII character into a non-ASCII
+ character (this can happen in some locales, like the Turkish
+ "I"), use the multibyte algorithm. */
+ if (SINGLE_BYTE_CHAR_P (ch) && !SINGLE_BYTE_CHAR_P (cased))
+ return do_casify_multibyte_string (ctx, obj);
+ SSET (obj, i, make_char_unibyte (cased));
}
return obj;
}
It's a wonder that Emacs works at all in a Turkish environment -- I
instrumented the function and made it spit out every time it translated
"I" to "1" during startup, and it was a huge stream of
Got multi: jISX0208.1983-0
Got multi: bIG5-0
Got multi: jISX0208.1983-0
Got multi: jISX0208.1983-0
Got multi: bIG5-0
Got multi: jISX0208.1983-0
Got multi: bIG5-0
Got multi: jISX0208.1983-0
etc. But I guess it doesn't trip up anything really vital.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 15:42:02 GMT)
Full text and
rfc822 format available.
Message #85 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: stefan <at> marxist.se, shuguang79 <at> qq.com, 50752 <at> debbugs.gnu.org
> Date: Tue, 19 Oct 2021 14:45:23 +0200
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > You mean, you want to special-case Turkish (and a couple of similar)
> > locale?
>
> Nope. The loop can just check whether we get a multibyte result when
> downcasing unibyte (ASCII), and then defer to the multibyte version to
> do the computation.
>
> This will basically only happen when using a Turkish (and a couple
> similar) locales, though, so the general performance impact will be
> approximately nil.
I'd actually prefer to ignore any locale-specific behavior that take
an ASCII character and downcases (or upcases) it to a non-ASCII one,
in the unibyte case. IOW, if changing a case of an ASCII character
from a unibyte string produces a non-ASCII multibyte character,
disregard any locale-specific case-conversion and use US ASCII rules.
That at least has the chance of not adversely affecting anything,
whereas producing non-ASCII multibyte string instead is IMO much
worse.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 15:58:01 GMT)
Full text and
rfc822 format available.
Message #88 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> I'd actually prefer to ignore any locale-specific behavior that take
> an ASCII character and downcases (or upcases) it to a non-ASCII one,
> in the unibyte case. IOW, if changing a case of an ASCII character
> from a unibyte string produces a non-ASCII multibyte character,
> disregard any locale-specific case-conversion and use US ASCII rules.
> That at least has the chance of not adversely affecting anything,
> whereas producing non-ASCII multibyte string instead is IMO much
> worse.
For the vast majority of cases, that would indeed produce the result
that's probably expected. (I mean, somebody doing (downcase "LATIN-1")
in code doesn't want the locale, anyway.)
But it means that downcasing a string in a buffer and saying (downcase
"THAT SAME STRING") gives us different results.
But perhaps that's the least bad solution?
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 16:02:02 GMT)
Full text and
rfc822 format available.
Message #91 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: stefan <at> marxist.se, shuguang79 <at> qq.com, 50752 <at> debbugs.gnu.org
> Date: Tue, 19 Oct 2021 15:24:12 +0200
>
> + /* If downcasing changed an ASCII character into a non-ASCII
> + character (this can happen in some locales, like the Turkish
> + "I"), use the multibyte algorithm. */
> + if (SINGLE_BYTE_CHAR_P (ch) && !SINGLE_BYTE_CHAR_P (cased))
> + return do_casify_multibyte_string (ctx, obj);
> + SSET (obj, i, make_char_unibyte (cased));
Like I said elsewhere, I'd prefer to use ASCII rules in that case.
It's safer, and definitely less surprising. Whoever wants Turkish
rules will need to submit multibyte strings, even for ASCII
characters.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 16:13:01 GMT)
Full text and
rfc822 format available.
Message #94 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: stefan <at> marxist.se, shuguang79 <at> qq.com, 50752 <at> debbugs.gnu.org
> Date: Tue, 19 Oct 2021 17:57:09 +0200
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > I'd actually prefer to ignore any locale-specific behavior that take
> > an ASCII character and downcases (or upcases) it to a non-ASCII one,
> > in the unibyte case. IOW, if changing a case of an ASCII character
> > from a unibyte string produces a non-ASCII multibyte character,
> > disregard any locale-specific case-conversion and use US ASCII rules.
> > That at least has the chance of not adversely affecting anything,
> > whereas producing non-ASCII multibyte string instead is IMO much
> > worse.
>
> For the vast majority of cases, that would indeed produce the result
> that's probably expected. (I mean, somebody doing (downcase "LATIN-1")
> in code doesn't want the locale, anyway.)
>
> But it means that downcasing a string in a buffer and saying (downcase
> "THAT SAME STRING") gives us different results.
If the buffer is unibyte, the results will be the same.
> But perhaps that's the least bad solution?
I think so, yes. _If_ we are going to change the current behavior,
that is.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 16:16:02 GMT)
Full text and
rfc822 format available.
Message #97 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> I think so, yes. _If_ we are going to change the current behavior,
> that is.
Well, the current behaviour is clearly a bug. So it should be fixed in
one way or another.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 16:22:01 GMT)
Full text and
rfc822 format available.
Message #100 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> But perhaps that's the least bad solution?
>
> I think so, yes.
As for the implementation... do you happen to know whether there are
any locales that makes up/downcase behave differently from the "C"
locale on ASCII characters, but does produce ASCII characters?
I'm just wondering whether the unibyte version of down/upcase should
just explicitly be documented to only do "C" locale stuff on unibyte
strings? That's easy to explain, at least, but I don't know what the
practical repercussions would be.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 16:32:02 GMT)
Full text and
rfc822 format available.
Message #103 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: stefan <at> marxist.se, shuguang79 <at> qq.com, 50752 <at> debbugs.gnu.org
> Date: Tue, 19 Oct 2021 18:21:35 +0200
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> But perhaps that's the least bad solution?
> >
> > I think so, yes.
>
> As for the implementation... do you happen to know whether there are
> any locales that makes up/downcase behave differently from the "C"
> locale on ASCII characters, but does produce ASCII characters?
Not off the top of my head, no.
> I'm just wondering whether the unibyte version of down/upcase should
> just explicitly be documented to only do "C" locale stuff on unibyte
> strings? That's easy to explain, at least, but I don't know what the
> practical repercussions would be.
This would be an unnecessary loss of functionality. Right now, one
can arrange a case-conversion table for raw bytes, and it will work
with the current code. We don't have to lose that, although the
utility of it is questionable.
I think we can leave the behavior with raw bytes as it is, and still
document this use case as being meant for ASCII conversions, leaving
the subtle case of raw bytes ... subtle.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 17:13:01 GMT)
Full text and
rfc822 format available.
Message #106 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> I think we can leave the behavior with raw bytes as it is, and still
> document this use case as being meant for ASCII conversions, leaving
> the subtle case of raw bytes ... subtle.
Yup.
The following patch seems to do the trick, and should be conservative
enough for emacs-28, I think.
diff --git a/src/casefiddle.c b/src/casefiddle.c
index a7a2541490..edc9dec2d7 100644
--- a/src/casefiddle.c
+++ b/src/casefiddle.c
@@ -297,6 +297,16 @@ do_casify_multibyte_string (struct casing_context *ctx, Lisp_Object obj)
return obj;
}
+static int
+ascii_casify_character (bool downcase, int c)
+{
+ Lisp_Object cased = CHAR_TABLE_REF (downcase?
+ uniprop_table (Qlowercase) :
+ uniprop_table (Quppercase),
+ c);
+ return FIXNATP (cased) ? XFIXNAT (cased) : c;
+}
+
static Lisp_Object
do_casify_unibyte_string (struct casing_context *ctx, Lisp_Object obj)
{
@@ -310,11 +320,12 @@ do_casify_unibyte_string (struct casing_context *ctx, Lisp_Object obj)
cased = case_single_character (ctx, ch);
if (ch == cased)
continue;
- cased = make_char_unibyte (cased);
- /* If the char can't be converted to a valid byte, just don't
- change it. */
- if (SINGLE_BYTE_CHAR_P (cased))
- SSET (obj, i, cased);
+ /* If down/upcasing changed an ASCII character into a non-ASCII
+ character (this can happen in some locales, like the Turkish
+ "I"), downcase using the ASCII char table. */
+ if (SINGLE_BYTE_CHAR_P (ch) && !SINGLE_BYTE_CHAR_P (cased))
+ cased = ascii_casify_character (ctx->flag == CASE_DOWN, ch);
+ SSET (obj, i, make_char_unibyte (cased));
}
return obj;
}
@@ -651,6 +662,8 @@ syms_of_casefiddle (void)
DEFSYM (Qbounds, "bounds");
DEFSYM (Qidentity, "identity");
DEFSYM (Qtitlecase, "titlecase");
+ DEFSYM (Qlowercase, "lowercase");
+ DEFSYM (Quppercase, "uppercase");
DEFSYM (Qspecial_uppercase, "special-uppercase");
DEFSYM (Qspecial_lowercase, "special-lowercase");
DEFSYM (Qspecial_titlecase, "special-titlecase");
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 17:38:02 GMT)
Full text and
rfc822 format available.
Message #109 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: stefan <at> marxist.se, shuguang79 <at> qq.com, 50752 <at> debbugs.gnu.org
> Date: Tue, 19 Oct 2021 19:12:20 +0200
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > I think we can leave the behavior with raw bytes as it is, and still
> > document this use case as being meant for ASCII conversions, leaving
> > the subtle case of raw bytes ... subtle.
>
> Yup.
>
> The following patch seems to do the trick, and should be conservative
> enough for emacs-28, I think.
I wouldn't start such experiments on the release branch. It's not an
urgent problem.
> +static int
> +ascii_casify_character (bool downcase, int c)
> +{
> + Lisp_Object cased = CHAR_TABLE_REF (downcase?
> + uniprop_table (Qlowercase) :
> + uniprop_table (Quppercase),
> + c);
> + return FIXNATP (cased) ? XFIXNAT (cased) : c;
> +}
uniprop_table is somewhat expensive, and is definitely an overkill for
converting pure ASCII characters. Why not just set/reset the 0x20
bit?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 18:22:01 GMT)
Full text and
rfc822 format available.
Message #112 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> The following patch seems to do the trick, and should be conservative
>> enough for emacs-28, I think.
>
> I wouldn't start such experiments on the release branch. It's not an
> urgent problem.
Yeah, it's pretty obscure.
> uniprop_table is somewhat expensive, and is definitely an overkill for
> converting pure ASCII characters. Why not just set/reset the 0x20
> bit?
I thought it made sense to be conservative here and do it "properly",
instead of open-coding an A-Za-z upcase/downcase ourselves. And doesn't
uniprop_table just return the table from char-code-property-alist here?
So it's an assq from a short list.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 19 Oct 2021 21:56:01 GMT)
Full text and
rfc822 format available.
Message #115 received at 50752 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
> Don't give up, you are close.
Thank you! Your feedback so far has been extremely useful and much
appreciated.
Based on your comments, I have been able to come up with the attached
patch. It bootstraps and all tests pass.
Please let me know what you think.
[0001-Be-more-allowing-when-looking-for-menu-bar-items.patch (text/x-diff, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 20 Oct 2021 07:46:01 GMT)
Full text and
rfc822 format available.
Message #118 received at 50752 <at> debbugs.gnu.org (full text, mbox):
I'm still seeing some oddities in the case stuff tr_TR.UTF-8.
If you have
india
ındıa
in a (multibyte) buffer and hit `M-u M-u', I get
İNDİA
INDIA
which is expected. However, `M-c M-c' gets me
India
Indıa
I tried googling whether dotted lower-case i should be upcased
differently when capitalising, but I couldn't find anything...
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 20 Oct 2021 11:29:01 GMT)
Full text and
rfc822 format available.
Message #121 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: stefan <at> marxist.se, shuguang79 <at> qq.com, 50752 <at> debbugs.gnu.org
> Date: Tue, 19 Oct 2021 20:21:42 +0200
>
> > uniprop_table is somewhat expensive, and is definitely an overkill for
> > converting pure ASCII characters. Why not just set/reset the 0x20
> > bit?
>
> I thought it made sense to be conservative here and do it "properly",
> instead of open-coding an A-Za-z upcase/downcase ourselves.
Why is it a problem to do it ourselves? It's a simple and very
efficient operation, and it will always be correct.
> And doesn't
> uniprop_table just return the table from char-code-property-alist here?
> So it's an assq from a short list.
It returns a char-table (so not really a short list), and it loads a
file when first called. It isn't incorrect, just too heavy for such a
simple job.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 20 Oct 2021 11:57:02 GMT)
Full text and
rfc822 format available.
Message #124 received at 50752 <at> debbugs.gnu.org (full text, mbox):
This all seems rather complicated to me.
It's very late to comment, but was the approach of adding an optional
argument to easy-menu-define that specifies the precise string to intern
(in place of "(downcase (car menu))"), and using that argument where
needed in those Emacs files that were converted to use easymenu,
rejected?
Basically the last paragraph from
https://lists.gnu.org/r/emacs-devel/2021-03/msg00046.html
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 20 Oct 2021 12:25:02 GMT)
Full text and
rfc822 format available.
Message #127 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: stefan <at> marxist.se, shuguang79 <at> qq.com, 50752 <at> debbugs.gnu.org
> Date: Wed, 20 Oct 2021 09:45:44 +0200
>
> I'm still seeing some oddities in the case stuff tr_TR.UTF-8.
>
> If you have
>
> india
> ındıa
>
> in a (multibyte) buffer and hit `M-u M-u', I get
>
> İNDİA
> INDIA
>
> which is expected. However, `M-c M-c' gets me
>
> India
> Indıa
>
> I tried googling whether dotted lower-case i should be upcased
> differently when capitalising, but I couldn't find anything...
Michal, could you perhaps help us out here?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 20 Oct 2021 13:00:02 GMT)
Full text and
rfc822 format available.
Message #130 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Tue, 19 Oct 2021 14:54:59 -0700
> Cc: larsi <at> gnus.org, shuguang79 <at> qq.com, 50752 <at> debbugs.gnu.org
>
> +/* Char table for the backwards-compatibility part in Flookup_key. */
> +Lisp_Object unicode_case_table;
Should be static, I guess.
> + /* Initialize the unicode case table, if it wasn't already. */
> + if (NILP (unicode_case_table))
> + unicode_case_table = uniprop_table (intern ("lowercase"));
This is okay, but the call to staticpro should be immediately after
you call uniprop_table, so that you protect the value of the table as
it was created. It won't work to staticpro it in syms_of_keymap,
which runs in temacs at build time.
> + if (!STRING_MULTIBYTE (key_item))
> + {
> + new_item = Fdowncase (key_item);
> + }
Style: a single statement in a block doesn't need braces.
And I guess the above assumes we resolve the issue with Turkish
locales when unibyte strings are downcased?
Otherwise, LGTM (but I didn't try applying and running the code).
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 21 Oct 2021 02:46:01 GMT)
Full text and
rfc822 format available.
Message #133 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
>> I thought it made sense to be conservative here and do it "properly",
>> instead of open-coding an A-Za-z upcase/downcase ourselves.
>
> Why is it a problem to do it ourselves? It's a simple and very
> efficient operation, and it will always be correct.
Hopefully.
>> And doesn't
>> uniprop_table just return the table from char-code-property-alist here?
>> So it's an assq from a short list.
>
> It returns a char-table (so not really a short list), and it loads a
> file when first called. It isn't incorrect, just too heavy for such a
> simple job.
These case tables are predefined (they're in `char-code-property-alist'
on startup with -Q), so calling uniprop_table here doesn't load
anything, and it doesn't cons anything -- it just does an assq on
char-code-property-alist and returns the char table.
Unless I'm misreading the code completely.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 21 Oct 2021 07:27:01 GMT)
Full text and
rfc822 format available.
Message #136 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: stefan <at> marxist.se, shuguang79 <at> qq.com, 50752 <at> debbugs.gnu.org
> Date: Thu, 21 Oct 2021 04:45:36 +0200
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> And doesn't
> >> uniprop_table just return the table from char-code-property-alist here?
> >> So it's an assq from a short list.
> >
> > It returns a char-table (so not really a short list), and it loads a
> > file when first called. It isn't incorrect, just too heavy for such a
> > simple job.
>
> These case tables are predefined (they're in `char-code-property-alist'
> on startup with -Q)
Right, we do that when dumping (loading charprop.el does it).
Still, I think using Unicode properties is overkill for this.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 21 Oct 2021 13:05:03 GMT)
Full text and
rfc822 format available.
Message #139 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Still, I think using Unicode properties is overkill for this.
Sure. And having a fast and efficient ASCII-only up/downcaser may be
generally useful.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Sun, 24 Oct 2021 20:12:02 GMT)
Full text and
rfc822 format available.
Message #142 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Glenn Morris <rgm <at> gnu.org> writes:
> It's very late to comment, but was the approach of adding an optional
> argument to easy-menu-define that specifies the precise string to intern
> (in place of "(downcase (car menu))"), and using that argument where
> needed in those Emacs files that were converted to use easymenu,
> rejected?
I think at this point, the main counter-argument is that we have one of
the fixes ready, whereas the other one isn't. I have been mulling your
message over the past couple of days, and I can't really see that your
proposed solution is significantly better. The chosen solution allows
us to side-step the issue with a small and localized change.
Furthermore, the approach we have chosen here is IMO not that
complicated, at least not on a basic level. Written in Lisp, it would
be very straight-forward indeed, but it turned out that with the
structure of our code it was easier to just write it in C.
So I would suggest pushing my fix to emacs-28, so that we can give it as
much testing as possible. At this stage, it feels important to make
sure there is some fix on the emacs-28 branch so that we can start
testing it and gather feedback.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Mon, 25 Oct 2021 13:07:01 GMT)
Full text and
rfc822 format available.
Message #145 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Stefan Kangas <stefan <at> marxist.se> writes:
> So I would suggest pushing my fix to emacs-28, so that we can give it as
> much testing as possible. At this stage, it feels important to make
> sure there is some fix on the emacs-28 branch so that we can start
> testing it and gather feedback.
Yup. (And I think your fix is the right one.)
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Mon, 25 Oct 2021 13:20:02 GMT)
Full text and
rfc822 format available.
Message #148 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: Glenn Morris <rgm <at> gnu.org>, Eli Zaretskii <eliz <at> gnu.org>,
> 50752 <at> debbugs.gnu.org, shuguang79 <at> qq.com
> Date: Mon, 25 Oct 2021 15:06:15 +0200
>
> Stefan Kangas <stefan <at> marxist.se> writes:
>
> > So I would suggest pushing my fix to emacs-28, so that we can give it as
> > much testing as possible. At this stage, it feels important to make
> > sure there is some fix on the emacs-28 branch so that we can start
> > testing it and gather feedback.
>
> Yup. (And I think your fix is the right one.)
Sorry, I see no reason to put this on the release branch.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Mon, 25 Oct 2021 13:23:02 GMT)
Full text and
rfc822 format available.
Message #151 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Sorry, I see no reason to put this on the release branch.
It fixes real interoperability problems -- there are packages out there
that break with the current emacs-28 because of the previous
easy-menu-define changes. Stefan's patch unbreaks those, but should
otherwise not affect operation.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Mon, 25 Oct 2021 13:52:02 GMT)
Full text and
rfc822 format available.
Message #154 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: stefan <at> marxist.se, rgm <at> gnu.org, 50752 <at> debbugs.gnu.org, shuguang79 <at> qq.com
> Date: Mon, 25 Oct 2021 15:21:56 +0200
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > Sorry, I see no reason to put this on the release branch.
>
> It fixes real interoperability problems -- there are packages out there
> that break with the current emacs-28 because of the previous
> easy-menu-define changes. Stefan's patch unbreaks those, but should
> otherwise not affect operation.
Can we come up with a much simpler and safer variant that could
perhaps solve 85% of the problem? like perhaps only the letter-case,
and disregarding the Turkish issue somehow? Then I could agree to
have that on the release branch. The code on master is complex and
non-trivial, so having that on the release branch scares me.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Mon, 25 Oct 2021 13:56:02 GMT)
Full text and
rfc822 format available.
Message #157 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Can we come up with a much simpler and safer variant that could
> perhaps solve 85% of the problem? like perhaps only the letter-case,
> and disregarding the Turkish issue somehow? Then I could agree to
> have that on the release branch.
Sure, I think that's what Stefan went for at the start (and which I was
thinking of, too), but it sounded like you wanted this to be more
ambitious.
I think just downcasing ASCII would get use more than 99%, really.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Mon, 25 Oct 2021 14:14:01 GMT)
Full text and
rfc822 format available.
Message #160 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: stefan <at> marxist.se, rgm <at> gnu.org, 50752 <at> debbugs.gnu.org, shuguang79 <at> qq.com
> Date: Mon, 25 Oct 2021 15:55:03 +0200
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > Can we come up with a much simpler and safer variant that could
> > perhaps solve 85% of the problem? like perhaps only the letter-case,
> > and disregarding the Turkish issue somehow? Then I could agree to
> > have that on the release branch.
>
> Sure, I think that's what Stefan went for at the start (and which I was
> thinking of, too), but it sounded like you wanted this to be more
> ambitious.
>
> I think just downcasing ASCII would get use more than 99%, really.
Then let's go with this variant on emacs-28, and leave the full
solution for master.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 26 Oct 2021 08:39:02 GMT)
Full text and
rfc822 format available.
Message #163 received at 50752 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
>> I think just downcasing ASCII would get use more than 99%, really.
>
> Then let's go with this variant on emacs-28, and leave the full
> solution for master.
OK. I'm not sure if you just mean my previous patch, but without Lars'
fixes for the Turkish language environment, or if you mean a stripped
down version of my patch.
So I have attached two versions of the patch, one minimal one and the
one you've already seen with the additional fixes for language
environments.
Please let me know which of these is acceptable for emacs-28, and I will
push it as soon as possible.
Thanks in advance.
[minimal-0001-Be-more-allowing-when-looking-for-menu-bar-items.patch (text/x-diff, attachment)]
[full-0001-Be-more-allowing-when-looking-for-menu-bar-items.patch (text/x-diff, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 26 Oct 2021 13:05:02 GMT)
Full text and
rfc822 format available.
Message #166 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Tue, 26 Oct 2021 01:38:32 -0700
> Cc: rgm <at> gnu.org, 50752 <at> debbugs.gnu.org, shuguang79 <at> qq.com
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> >> I think just downcasing ASCII would get use more than 99%, really.
> >
> > Then let's go with this variant on emacs-28, and leave the full
> > solution for master.
>
> OK. I'm not sure if you just mean my previous patch, but without Lars'
> fixes for the Turkish language environment, or if you mean a stripped
> down version of my patch.
Unfortunately, I meant neither of these two, because they both are
quite non-trivial. I meant a much simpler patch which only downcases
ASCII letters, and that's all. Such a change doesn't need to call
Fdowncase, and definitely doesn't need to futz with multibyte
characters. It should be a simple copy and a single loop downcasing
the characters. (The NEWS entry for emacs-28 should thus say that we
only handle this simple class of problems.)
The log message should say "don't merge to master".
Sorry for not expressing myself clearly enough.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Tue, 26 Oct 2021 20:25:02 GMT)
Full text and
rfc822 format available.
Message #169 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Unfortunately, I meant neither of these two, because they both are
> quite non-trivial. I meant a much simpler patch which only downcases
> ASCII letters, and that's all. Such a change doesn't need to call
> Fdowncase, and definitely doesn't need to futz with multibyte
> characters. It should be a simple copy and a single loop downcasing
> the characters. (The NEWS entry for emacs-28 should thus say that we
> only handle this simple class of problems.)
OK, let me just double check that you mean to memcpy and then just loop
over the memory byte by bite and update them like so:
int new_ch = XFIXNUM (CHAR_TABLE_REF (Vascii_downcase_table, ch))
Or do you mean something even simpler like this?
if (ch >= 'A' && ch <= 'Z')
new_ch = c + ('A' - 'a');
> The log message should say "don't merge to master".
Hmm. If we can live with a simpler fix on emacs-28, can we not live
with it on master as well?
Or at least live with it until we can evaluate if the simpler fix did
the job well enough?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Wed, 27 Oct 2021 14:01:02 GMT)
Full text and
rfc822 format available.
Message #172 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Tue, 26 Oct 2021 22:24:10 +0200
> Cc: larsi <at> gnus.org, rgm <at> gnu.org, 50752 <at> debbugs.gnu.org, shuguang79 <at> qq.com
>
> OK, let me just double check that you mean to memcpy and then just loop
> over the memory byte by bite and update them like so:
>
> int new_ch = XFIXNUM (CHAR_TABLE_REF (Vascii_downcase_table, ch))
>
> Or do you mean something even simpler like this?
>
> if (ch >= 'A' && ch <= 'Z')
> new_ch = c + ('A' - 'a');
The latter.
> > The log message should say "don't merge to master".
>
> Hmm. If we can live with a simpler fix on emacs-28, can we not live
> with it on master as well?
On master, I'd prefer to have the full solution, which you already
coded. There's no need to give up a complete solution there. It just
is too complex to safely backport it to the release branch, so Emacs
28 will have to do with a 99% solution, not 100%.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 28 Oct 2021 05:30:02 GMT)
Full text and
rfc822 format available.
Message #175 received at 50752 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> gnu.org> writes:
>> Or do you mean something even simpler like this?
>>
>> if (ch >= 'A' && ch <= 'Z')
>> new_ch = c + ('A' - 'a');
>
> The latter.
OK, I guess that should be the attached. I hope I got it right.
[0001-Be-more-allowing-when-looking-for-menu-bar-items.patch (text/x-diff, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 28 Oct 2021 07:34:02 GMT)
Full text and
rfc822 format available.
Message #178 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Wed, 27 Oct 2021 22:29:48 -0700
> Cc: larsi <at> gnus.org, rgm <at> gnu.org, 50752 <at> debbugs.gnu.org, shuguang79 <at> qq.com
>
> >> if (ch >= 'A' && ch <= 'Z')
> >> new_ch = c + ('A' - 'a');
> >
> > The latter.
>
> OK, I guess that should be the attached. I hope I got it right.
Yes, thanks. But you still didn't say "don't merge to master", for
some reason.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 28 Oct 2021 08:07:02 GMT)
Full text and
rfc822 format available.
Message #181 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Yes, thanks. But you still didn't say "don't merge to master", for
> some reason.
AFAICT, the commit message in the patch I sent does say so, right at the end.
Is it not in the correct format?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 28 Oct 2021 09:37:02 GMT)
Full text and
rfc822 format available.
Message #184 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Thu, 28 Oct 2021 01:06:45 -0700
> Cc: larsi <at> gnus.org, rgm <at> gnu.org, 50752 <at> debbugs.gnu.org, shuguang79 <at> qq.com
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > Yes, thanks. But you still didn't say "don't merge to master", for
> > some reason.
>
> AFAICT, the commit message in the patch I sent does say so, right at the end.
It does indeed, sorry for being blind.
> Is it not in the correct format?
Not wrong, just not easy to read.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 28 Oct 2021 10:50:02 GMT)
Full text and
rfc822 format available.
Message #187 received at 50752 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> Not wrong, just not easy to read.
Indeed, it's sort of easy to miss. Noam seems to have used this more
visible format before that I chose to copy:
Don't merge to master. This is a safe-for-release fix for Bug#40727.
(I also moved it to the third line for good measure.)
With that change, this fix is now pushed to emacs-28 (commit
0f8417d597). Should I go ahead and push the patch for master as well?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 28 Oct 2021 12:50:01 GMT)
Full text and
rfc822 format available.
Message #190 received at 50752 <at> debbugs.gnu.org (full text, mbox):
> From: Stefan Kangas <stefan <at> marxist.se>
> Date: Thu, 28 Oct 2021 06:49:12 -0400
> Cc: larsi <at> gnus.org, rgm <at> gnu.org, 50752 <at> debbugs.gnu.org, shuguang79 <at> qq.com
>
> With that change, this fix is now pushed to emacs-28 (commit
> 0f8417d597). Should I go ahead and push the patch for master as well?
The more thorough one, which also supported spaces instead of dashes?
Yes, I think so.
Thanks.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#50752; Package
emacs.
(Thu, 28 Oct 2021 20:45:01 GMT)
Full text and
rfc822 format available.
Message #193 received at 50752 <at> debbugs.gnu.org (full text, mbox):
close 50752 28.1
thanks
Eli Zaretskii <eliz <at> gnu.org> writes:
> The more thorough one, which also supported spaces instead of dashes?
> Yes, I think so.
OK, now done (commit 2671ea0de8). I'm consequently closing this bug
report, and marking it as done in "28.1" (as the recipe in the original
bug report was fixed in that version).
BTW, thanks for the assertion fix on emacs-28. I had somehow forgotten
to re-enable the assertion flags after disabling them to try something
out the other day. I'll try to remember to double check that next time.
bug marked as fixed in version 28.1, send any further explanations to
50752 <at> debbugs.gnu.org and "Shuguang Sun" <shuguang79 <at> qq.com>
Request was from
Stefan Kangas <stefan <at> marxist.se>
to
control <at> debbugs.gnu.org.
(Thu, 28 Oct 2021 20:45:02 GMT)
Full text and
rfc822 format available.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Fri, 26 Nov 2021 12:24:06 GMT)
Full text and
rfc822 format available.
This bug report was last modified 2 years and 148 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.