Bug 1701

Created attachment 1597 [details]
Patch to add accelerators (underlines) to konsole main menu bar

The patch works for me. Please test!

I tested the patch. Works great. Well done!

Thanks. Patch pushed to git in commit fd2f0b36.

I think I now understand the reason why Konsole add no accelerator keys fir the main menu.
Try to use Midnight Commander from the Konsole with the accelerator keys enable. Common commands as Alt+S and Alt+T now brings up the respective menu instead of being handled by MC.

As I guess most of TDE users use MC in a Konsole, I propose to revert the patch. What do you think?

(In reply to comment #4)
> I think I now understand the reason why Konsole add no accelerator keys fir the
> main menu.
That should have been 'why Konsole had no accelerator keys'...

Also ALt+H and Alt+Shift+H don't work any more :(

Sigh. :-)

Options/work-arounds:

* Revert the patch.

* Improve the patch.

* Change the konsole Help accelerator key (inconsistent with other TDE apps).

* Teach users to hide the menu bar before pressing Alt-H (I tested, this works).

Notes:

KDE4 konsole supports accelerators. There is a configuration option to toggle the accelerators.

KDE4 konsole has a built-in shortcut for toggling the menu bar (Ctrl+Shift+M).

Trinity inherited a clunky method for toggling the menu bar:

* From the Settings menu, select Hide Menubar (doable with the keyboard).

* "Right-click" in the konsole window to activate a popup menu and select Show Menubar. Not intuitive, not doable with the keyboard).

My desire for accelerators stems from my usage of Bookmarks. I have a nice long list and without accelerators I have to use the mouse to use the bookmarks.

My thoughts:

* Temporarily revert the patch (boo-hoo!).
* Add a configuration option to enable/disable accelerators.
* Add a keyboard shortcut to toggle the menubar (Ctrl+Shift+M).

I'm not a C++ hacker like you folks and I would need help. :-)

This bug report I had not noticed before, than I saw pushed patch in GIT. And the very first thing I thought was that this could mean conflict with applications in the Konsole... and that's the thing that I always very angry whenever I had to use GNOME and GNOME Terminal.

I personally have the menu always turned off, but I fully understand that this will annoy most other users :)

(Odpověď na komentář #7)
> Sigh. :-)
> 
> Options/work-arounds:
> 
> * Revert the patch.
> 
> * Improve the patch.
> 
> * Change the konsole Help accelerator key (inconsistent with other TDE apps).
> 

In this way, it can not be solved => other languages ​​use other accelerators.

> * Teach users to hide the menu bar before pressing Alt-H (I tested, this
> works).
> 

It would be hard to explain the user that the accelerators in Konsole gives him an advantage :)

> Notes:
> 
> KDE4 konsole supports accelerators. There is a configuration option to toggle
> the accelerators.
> 
> KDE4 konsole has a built-in shortcut for toggling the menu bar (Ctrl+Shift+M).
> 
> Trinity inherited a clunky method for toggling the menu bar:
> 
> * From the Settings menu, select Hide Menubar (doable with the keyboard).
> 
> * "Right-click" in the konsole window to activate a popup menu and select Show
> Menubar. Not intuitive, not doable with the keyboard).
> 

The default keyboard shortcut Ctrl+Alt+M is used to activate the menu - even if it was hidden.

> My desire for accelerators stems from my usage of Bookmarks. I have a nice long
> list and without accelerators I have to use the mouse to use the bookmarks.
> 
> My thoughts:
> 
> * Temporarily revert the patch (boo-hoo!).
> * Add a configuration option to enable/disable accelerators.
> * Add a keyboard shortcut to toggle the menubar (Ctrl+Shift+M).
> 
> I'm not a C++ hacker like you folks and I would need help. :-)

Proposed Ctrl+Shift+M unfortunately conflicts in MC. I do not know how difficult it is to implement a configuration option to enable/disable accelerators. Revert patch appears now to be the fastest solution.

(Odpověď na komentář #6)
> Also ALt+H and Alt+Shift+H don't work any more :(

Now you also fully understand how crucial it was for me bug 1676 :)

I reverted the patch in commit 183530f3.

With that done, I am asking for a two-point compromise.

First, add a General configuration option to 'Enable menu accelerators'. The default would be disabled/off. Having that option should satisfy all users.

Enabling the accelerators introduces certain conflicts discussed here, which leads to the second point.

I notice no conflict with Midnight Commander when the Konsole menubar is hidden. If we add a keyboard shortcut to toggle the konsole menubar that would provide relief for those who want menu accelerators enabled.

The What's This help tooltip for 'Enable menu accelerators' would explain the potential conflicts and that the only work-around is to hide the menu bar when using console based apps with conflicting shortcuts. For example:

General tab

| | Enable menu accelerators

What's This Tooltip

Most Trinity apps use menu accelerators to use menus from the keyboard. Konsole is a unique app that has the menu accelerators disabled as the default. The reason is many console based apps use keyboard shortcuts and the Konsole menu accelerators conflict with those console based app shortcuts. Midnight Commander is a prime example. To those users who want Konsole menu accelerators and want to avoid conflicts with console based app shortcuts, the recommended work-around is to hide the Konsole menu bar when using console based apps with conflicting shortcuts.

> First, add a General configuration option to 'Enable menu accelerators'. The
> default would be disabled/off. Having that option should satisfy all users.
This sound like a good idea

> I notice no conflict with Midnight Commander when the Konsole menubar is
> hidden. If we add a keyboard shortcut to toggle the konsole menubar that would
> provide relief for those who want menu accelerators enabled.
This is also a good idea, regardless of point 1. I wonder why such shortcut hasn't been made available

I'm going to have to agree here...menu accelerators are a bad idea in a terminal window that will likely run applications that use their own "accelerators" (i.e. control key sequences).  Please revert the patch.

>Please revert the patch.
Already done: refer to comment 10.

>menu accelerators are a bad idea
Please read my proposal in comment 10. :-)

Note: The KDE4 Konsole and Xfce Terminal support toggling menu accelerators. KDE4 Konsole supports a shortcut to toggle the menubar while the Xfce Terminal toggles the menubar the same way as Trinity --- with the mouse.

(Odpověď na komentář #10)
> I notice no conflict with Midnight Commander when the Konsole menubar is
> hidden. If we add a keyboard shortcut to toggle the konsole menubar that would
> provide relief for those who want menu accelerators enabled.
> 

Hiding / displaying menu has one small drawback - resizing the window for the applications. Frequent toggle menu visibility also does not seem as user-friendly solution.

Configuration option to enable / disable accelerators for me seems as only way to solve this.

>Hiding / displaying menu has one small drawback - resizing the window for the
>applications. Frequent toggle menu visibility also does not seem as
>user-friendly solution.
Hiding the menubar already exists --- we just don't have a nice keyboard shortcut. KDE4 uses Ctrl+Shift+M. If that shortcut conflicts with MC then we could try Ctrl+Alt+M. Trinity-Amarok uses Ctrl+M. I'm sure whatever shortcut is used will conflict with some kind of console app, even if not MC. In my testing with toggling the menubar, the nominal resizing did not bother me. I had not even noticed until reminded.

The only people who think about any of this are those who want accelerators. They have to take two explicit steps to use them seamlessly: 1) manually enable them in the Konsole settings and 2) manually hide the menubar as necessary to avoid shortcut conflicts. Every other user just ignores the whole deal. :-)

I will work on this bug this week, when I have enough free time.
I am looking to add both options suggested by Darrell. With them, each user would be able to select their preferred setup, while the default configuration would remain "accelerators off" and "menu bar visible".

While looking at this bug, I noticed that a shortcut to show/hide the menu bar already exists, even though it doesn't have any default key associated to it.
In Konsole -> Settings -> Configure shortcut... -> Show Menu bar entry.
After associating a shortcut to this action (for example Ctrl+Shift+Alt+M), it works both way (show and hide).

So the only thing left to do is to add an option to enable/disable the main menu accelerator keys.
Also if there is nothing against it, I will make 'Ctrl+Shift+Alt+M' the default shortcut for 'Show Menu bar'. Hopefully this sequence of keys minimizes the risk of conflicts with application run inside Konsole. 
If you are aware of any application that uses 'Ctrl+Shift+Alt+M', please let me know and I will look at a difference sequence (suggestions are welcome as well)

What combinations of 'Ctrl'+'Shift'+'Alt'+M conflict with MC?

There is an 'Activate Menu' shortcut with an assignment of Alt+Ctrl+M. I am unable to get that shortcut to do anything.

(Odpověď na komentář #19)
> What combinations of 'Ctrl'+'Shift'+'Alt'+M conflict with MC?
> 
> There is an 'Activate Menu' shortcut with an assignment of Alt+Ctrl+M. I am
> unable to get that shortcut to do anything.

For me Ctrl+Alt+M works correctly. After pressing the keyboard shortcut the menu is opened. If menu is not displayed, then first appear and then is opened.

Ctrl+Alt+Shift+M seems to be not conflict.

>For me Ctrl+Alt+M works correctly.
I found the problem: a stale assignment in khotkeysrc.

>Ctrl+Alt+Shift+M seems to be not conflict.
'Ctrl+Alt+Shift' is a finger twister on some keyboards. What combinations of *+M are used in MC? I presume the new shortcut will be reassignable?

(Odpověď na komentář #21)
> >For me Ctrl+Alt+M works correctly.
> I found the problem: a stale assignment in khotkeysrc.
> 
> >Ctrl+Alt+Shift+M seems to be not conflict.
> 'Ctrl+Alt+Shift' is a finger twister on some keyboards. What combinations of
> *+M are used in MC? I presume the new shortcut will be reassignable?

For me, the MC responds to Ctrl+M and Ctrl+Shift+M.
And of course i Shift+M ;)

No reaction I observe on the Alt+M (it did not seem to be a suitable combination) and Alt+Shift+M.

> >Ctrl+Alt+Shift+M seems to be not conflict.
> 'Ctrl+Alt+Shift' is a finger twister on some keyboards. What combinations of
> *+M are used in MC? I presume the new shortcut will be reassignable?

'Ctrl+Alt+Shift' is a little awkward indeed, but I think it is wiser to avoid key combinations that only use Ctrl, Shift or Alt because such combinations may be used by programs run inside Konsole. Such weird combination should minimize the risk of conflicts with other programs' shortcuts.
The shortcut is reassignable anyway.

The alternative is to leave the shortcut without a default combination as it has been so far.

Created attachment 1628 [details]
tdebase Konsole menu accelerator patch

This patch provides the following.

1) The keyboard shortcut for 'Show/Hide menu bar' is defaulted to 'Ctrl+Shift+Alt+M'.
If you prefer another combination, you just need to change one line before committing to GIT

2) Provide a check box 'Enable/disable main menu accelerator keys' in Konsole -> Configure Konsole... to enable/disable the main menu accelerator keys.

Michele the patch works fine for me. Good job! :-)

One comment:

AS IS:

Enable/disable main menu accelerator keys

CHANGE TO:

Enable main menu accelerator keys

As the patch is controversial I won't push to git until at least Slavek has tested, preferably Tim too.

> AS IS:
> Enable/disable main menu accelerator keys
> CHANGE TO:
> Enable main menu accelerator keys
Attached the new patch. I am not on a TDE machine, but being only an inline change I just manually edited the patch file

> As the patch is controversial I won't push to git until at least Slavek has
> tested, preferably Tim too.
Fair enough. Anyhow the default behavior is to leave everything as is (menu accelerator disabled), so it shouldn't cause any major problem. It just provides more options to the final user.

Created attachment 1636 [details]
new patch

The latest patch works as expected.

The patch looks good.
Just a few notes:

1) instead of QString should be TQString

2) instead of QString(m_session_string).remove('&')
   (and similar) I would suggest
   TQString(m_session_string).replace(TQRegExp("&([^&])"), "\\1")

Created attachment 1639 [details]
updated patch

> 1) instead of QString should be TQString
Ah, yes! That was a silly mistake of mine

>2) instead of QString(m_session_string).remove('&')
>   (and similar) I would suggest
>   TQString(m_session_string).replace(TQRegExp("&([^&])"), "\\1")
Good suggestion, thanks!

I have attached the updated patch with Slavek's corrections.
Again, I am not on a TDE machine so I edited the file manually, but that should do.

No surprises here with the latest patch. All is working here and no noticeable conflicts with mc.

Pushed to GIT in hash c244bac1.
Thank you both for your good work.