Bug 165949 - Wrong solution(for asia) to strip ampersand characters from top-level menu items in SVN:825353
Summary: Wrong solution(for asia) to strip ampersand characters from top-level menu it...
Status: RESOLVED FIXED
Alias: None
Product: konsole
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Konsole Developer
URL:
Keywords:
: 166138 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-07-07 16:52 UTC by Liang Qi
Modified: 2008-07-18 14:41 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch which uses a regular expression to remove " (&Letters)" from menu items as well as just '&' (728 bytes, patch)
2008-07-09 21:33 UTC, Robert Knight
Details
Patch which uses a translateable regular expression to remove accelerators from menu items (971 bytes, patch)
2008-07-10 21:26 UTC, Robert Knight
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Liang Qi 2008-07-07 16:52:56 UTC
Version:            (using Devel)
Installed from:    Compiled sources
OS:                Linux

In Bug 162209, SVN 825353, it is not acceptable that the change for "Strip ampersand characters from the text of standard top-level menu items". 

It's a "dirty" solution only for Western languages.

In Western languages, the shortcut key should be "O" in "&Open", after your solution, "Open" is OK.

But for East Asia languages, at least for us, Chinese(including simplified and traditional), Japanese and Korean should be same, it is not acceptable. We use "打开(&O)" for "&Open", after your solution, the users will confused about it, "打开(O)". It disables the shortcut key, but still displays the key for shortcut.

Here is a screen shot:
http://img391.imageshack.us/img391/8725/konsolemenubaruu9.png

Thanks for Nihui reporting this bug in kde-china mailing list.
http://mail.kde.org/pipermail/kde-china/2008-July/001835.html
Comment 1 Robert Knight 2008-07-07 18:03:25 UTC
Thank-you for bringing this up.  I didn't know that accelerators were represented differently in non-Western languages.

I'll have to try the more complex approach of fixing XMLGUI so that I can put <text> tags in the konsoleui.rc file which over-ride the standard text for the File/View/Edit/Help etc. entries defined in uistandards.rc in kdelibs.

Unfortunately this will require these entries in konsoleui.rc to be translated for each locale - is this going to be a problem for KDE 4.1?  
Comment 2 Liang Qi 2008-07-07 21:04:43 UTC
Maybe you can write a script to copy the text items from uistandards.rc to konsoleui.rc. Then send a notice mail to kde-i18n-doc mailing list. If just remove some accel keys, it should not be a problem. 

When KDE 4.1 release? I just backed from vacation.
Comment 3 Robert Knight 2008-07-08 01:24:50 UTC
> When KDE 4.1 release? I just backed from vacation. 

July 29th.  We need to sort this out this week.
Comment 4 Robert Knight 2008-07-09 18:28:54 UTC
*** Bug 166138 has been marked as a duplicate of this bug. ***
Comment 5 Robert Knight 2008-07-09 18:31:07 UTC
Hi Liang,

From your description of how accelerators are represented in CJK languages I could just modify the code to strip out anything in parenthesies - for KDE 4.1 at least.
Comment 6 Robert Knight 2008-07-09 21:33:47 UTC
Created attachment 25995 [details]
Patch which uses a regular expression to remove " (&Letters)" from menu items as well as just '&'
Comment 7 Robert Knight 2008-07-09 21:35:29 UTC
Hi Liang,Yukko,

Please test the patch in comment #6.  If that works for you then I will commit it for KDE 4.1.  

The 'proper' fix requires changes to kdelibs which could conceivably have an impact on other applications and therefore needs to be done at the start of the development cycle.

Comment 8 nihui 2008-07-10 06:42:37 UTC
hi
I've tested the patch in comment #6 in KDE 4.1 rc1.
but this bug still exists  :(

http://img503.imageshack.us/img503/3684/konsolemenubar2zq5.png

nihui
Comment 9 Robert Knight 2008-07-10 07:00:30 UTC
> I've tested the patch in comment #6 in KDE 4.1 rc1.

To clarify - the patch isn't in KDE 4.1 RC1, you have to apply it to the source.  I tested it by changing the text of one of the existing entries to "Text (&T)".  
Comment 10 Chusslove Illich 2008-07-10 20:57:19 UTC
The regex in #6 patch should not have the leading \s before the parenthesis, as there are no spaces in CJK; otherwise it works, checked here on Japanese-localized Konsole.

Another solution would be to let translators handle extra accelerator removal, by providing a "filtering" message, e.g.:

  ...
  itemText.remove('&');
  itemText = i18nc("Menu title in main menu", "%1", itemText);
  menuItem->setText(itemText);
  ...

after which translators can script modifications to the text being filtered (amounts to single function call). This is the solution I provided e.g. for toolbar icon labels, to be sure not to clash with something else in some unrelated language, and to allow solutions per language if needed.

I can't say which of these two is better. At any rate, for 4.2 perhaps we should add a KLocale::stripAccelerator function -- this is already third out-of-kdelibs case where such a need arose (implementation wise it could use either of the above methods).
Comment 11 Robert Knight 2008-07-10 21:26:14 UTC
Created attachment 26023 [details]
Patch which uses a translateable regular expression to remove accelerators from menu items

Second revision of patch.  Remove leading \s from regular expression and make
the pattern string translateable in case the default causes problems in another
language.
Comment 12 Robert Knight 2008-07-10 21:31:06 UTC
> At any rate, for 4.2 perhaps we should add a KLocale::stripAccelerator
> function -- this is already third out-of-kdelibs case where such a need
> arose (implementation wise it could use either of the above methods)

I think that would be useful.  Do you want to take care of it?
Comment 13 Chusslove Illich 2008-07-10 21:42:37 UTC
#11 works fine too; and regex as message is probably the best implementation for KLocale-centralized custom removal. I will take care of it, sooner or later -- long road to 4.2 :)
Comment 14 nihui 2008-07-11 05:37:04 UTC
:D

the second patch fixed it!
Comment 15 Liang Qi 2008-07-11 10:33:28 UTC
#11 is ok for me. 

Wait for perfect solution in 4.2.
Comment 16 Robert Knight 2008-07-13 02:52:28 UTC
Until then we'll call it fixed.
Comment 17 Yukiko Bando 2008-07-14 15:48:58 UTC
The translatable regular expression has not been added to konsole.po by Scripty today. Was the fix committed? :)
Comment 18 Robert Knight 2008-07-14 18:31:47 UTC
SVN commit 832422 by knight:

Fix accelerator removal for non-Western languages which use "(&Letter)"
to represent accelerators in menu items rather than just '&' in front of one letter.

MainWindow::removeMenuAccelerators()
    Use a regexp to match "(&Letters)" as well as '&'

CCBUG: 165949


 M  +5 -1      MainWindow.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=832422
Comment 19 Robert Knight 2008-07-14 18:31:56 UTC
SVN commit 832423 by knight:

Remove required space before parenthesies in accelerator stripping regular expression.
Chusslove informs me that there are no spaces in CJK.

CCBUG: 165949


 M  +1 -1      MainWindow.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=832423
Comment 20 Robert Knight 2008-07-14 18:31:57 UTC
SVN commit 832424 by knight:

Make pattern string for accelerator stripping pattern translateable.
CCBUG: 165949


 M  +6 -2      MainWindow.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=832424
Comment 21 Chusslove Illich 2008-07-18 14:41:20 UTC
SVN commit 834213 by ilic:

Backport stripping of accelerators from main menu items;
do not make pattern i18n for the moment, to avoid breaking message freeze
(noone needs to have it customized right away anyway; perhaps add later).
CCBUG: 165949


 M  +8 -1      MainWindow.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=834213