Bug 128984 - "Jump to folder" kbd shortcut hardcoded (can't be changed/turned off)
Summary: "Jump to folder" kbd shortcut hardcoded (can't be changed/turned off)
Status: RESOLVED FIXED
Alias: None
Product: kmail
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Gentoo Packages Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: kdepim bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-06-11 10:12 UTC by Boris Dušek
Modified: 2007-09-14 12:17 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
Patch adding configurability of the shortcuts (1.49 KB, patch)
2006-06-20 18:31 UTC, Boris Dušek
Details
Fix a default shortcut to SHIFT+Key_P (589 bytes, patch)
2006-06-20 23:39 UTC, Boris Dušek
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Boris Dušek 2006-06-11 10:12:18 UTC
Version:            (using KDE KDE 3.5.3)
Installed from:    Gentoo Packages

Hi,

running KMail shipped with 3.5.3 (in Kontact), whenever I press the key "j", a "Jump to folder" dialog pops up. This annoys me since I heavily use the vim-like behavior that uses "k", "j" to move down and up. I found the "Jump to folder" action in "Configure shortcuts" dialog and unset the keyboard shortcut for that action (set it to "None"), but kmail did not respect the change. Note that scrolling up with "k" key works.
Comment 1 hads 2006-06-17 04:52:19 UTC
I agree, I use j, k scrolling heavily too and it bugs me that I can't turn this off (using Kubuntu packages). Cheers.
Comment 2 Boris Dušek 2006-06-20 18:30:10 UTC
Ok, so I looked into it and added three lines for 3 shortcuts, one of which was the "j" shortcut. Now it works as expected. Patch follows.

Please test it/review it and report back any problems. Thanks.
Comment 3 Boris Dušek 2006-06-20 18:31:42 UTC
Created attachment 16719 [details]
Patch adding configurability of the shortcuts
Comment 4 Magnus Holmgren 2006-06-20 19:05:43 UTC
I don't think the setShortcutConfigurable() calls are needed - KAction shortcuts are by default configurable. You just need to delete the mAccel->connectItem() calls.

This bug overlaps bug 124501.
Comment 5 Boris Dušek 2006-06-20 20:05:33 UTC
Yes, Magnus, the setShortcutConfigurable() calls are not needed. I updated your patch from bug 124501 with the "j" shortcut (which was probably added after 3.5.2) and sent it there. Thanks for the info.

Please someone mark this as a duplicate of 124501 (I don't know how). Thanks.
Comment 6 Ingo Klöcker 2006-06-20 21:55:42 UTC

*** This bug has been marked as a duplicate of 124501 ***
Comment 7 Ingo Klöcker 2006-06-20 23:03:00 UTC
SVN commit 553370 by kloecker:

Fix bug 124501 (Some keyboard shortcuts bound twice) and 128984 ("Jump to folder" kbd shortcut hardcoded).
Don't use a QAccel. For one we already have an appropriate KAccel (actioncollection()->kaccel()). Moreover, using QAccel breaks the configurability of kbd shortcuts. KAction->plugAccel is deprecated but I don't see what else to use. I guess the additional plugAccel shouldn't be necessary, but at least for Return the accel didn't work out of the box (probably because the corresponding KAction doesn't appear in any menu or toolbar).

BUGS: 124501
CCBUGS: 128984


 M  +37 -43    kmmainwidget.cpp  
 M  +0 -3      kmmainwidget.h  


--- branches/KDE/3.5/kdepim/kmail/kmmainwidget.cpp #553369:553370
@@ -38,6 +38,7 @@
 #include <kstandarddirs.h>
 #include <dcopclient.h>
 #include <kaddrbook.h>
+#include <kaccel.h>
 
 #include "globalsettings.h"
 #include "kcursorsaver.h"
@@ -126,8 +127,7 @@
     QWidget(parent, name),
     mQuickSearchLine( 0 ),
     mShowBusySplashTimer( 0 ),
-    mShowingOfflineScreen( false ),
-    mAccel( 0 )
+    mShowingOfflineScreen( false )
 {
   // must be the first line of the constructor:
   mStartupDone = FALSE;
@@ -473,8 +473,6 @@
 //-----------------------------------------------------------------------------
 void KMMainWidget::createWidgets(void)
 {
-  mAccel = new QAccel(this, "createWidgets()");
-
   // Create the splitters according to the layout settings
   QWidget *headerParent = 0, *folderParent = 0,
             *mimeParent = 0, *messageParent = 0;
@@ -544,9 +542,10 @@
           this, SLOT(slotMsgActivated(KMMessage*)));
   connect( mHeaders, SIGNAL( selectionChanged() ),
            SLOT( startUpdateMessageActionsTimer() ) );
-  mAccel->connectItem(mAccel->insertItem(SHIFT+Key_Left),
+  QAccel *accel = actionCollection()->kaccel();
+  accel->connectItem(accel->insertItem(SHIFT+Key_Left),
                      mHeaders, SLOT(selectPrevMessage()));
-  mAccel->connectItem(mAccel->insertItem(SHIFT+Key_Right),
+  accel->connectItem(accel->insertItem(SHIFT+Key_Right),
                      mHeaders, SLOT(selectNextMessage()));
 
   if (mReaderWindowActive) {
@@ -562,33 +561,34 @@
         mMsgView, SLOT(clearCache()));
     connect(mMsgView, SIGNAL(noDrag()),
         mHeaders, SLOT(slotNoDrag()));
-    mAccel->connectItem(mAccel->insertItem(Key_Up),
+    accel->connectItem(accel->insertItem(Key_Up),
         mMsgView, SLOT(slotScrollUp()));
-    mAccel->connectItem(mAccel->insertItem(Key_Down),
+    accel->connectItem(accel->insertItem(Key_Down),
         mMsgView, SLOT(slotScrollDown()));
-    mAccel->connectItem(mAccel->insertItem(Key_Prior),
+    accel->connectItem(accel->insertItem(Key_Prior),
         mMsgView, SLOT(slotScrollPrior()));
-    mAccel->connectItem(mAccel->insertItem(Key_Next),
+    accel->connectItem(accel->insertItem(Key_Next),
         mMsgView, SLOT(slotScrollNext()));
   } else {
     mMsgView = NULL;
   }
 
-  new KAction( i18n("Move Message to Folder"), Key_M, this,
+  KAction *action;
+
+  action = new KAction( i18n("Move Message to Folder"), Key_M, this,
                SLOT(slotMoveMsg()), actionCollection(),
                "move_message_to_folder" );
-  new KAction( i18n("Copy Message to Folder"), Key_C, this,
+  action->plugAccel( actionCollection()->kaccel() );
+
+  action = new KAction( i18n("Copy Message to Folder"), Key_C, this,
                SLOT(slotCopyMsg()), actionCollection(),
                "copy_message_to_folder" );
-  new KAction( i18n("Jump to Folder"), Key_J, this,
+  action->plugAccel( actionCollection()->kaccel() );
+
+  action = new KAction( i18n("Jump to Folder"), Key_J, this,
                SLOT(slotJumpToFolder()), actionCollection(),
                "jump_to_folder" );
-  mAccel->connectItem(mAccel->insertItem(Key_M),
-		     this, SLOT(slotMoveMsg()) );
-  mAccel->connectItem(mAccel->insertItem(Key_C),
-		     this, SLOT(slotCopyMsg()) );
-  mAccel->connectItem(mAccel->insertItem(Key_J),
-		     this, SLOT(slotJumpToFolder()) );
+  action->plugAccel( actionCollection()->kaccel() );
 
   // create list of folders
   mFolderTree = new KMFolderTree(this, folderParent, "folderTree");
@@ -607,50 +607,45 @@
           this, SLOT(slotFolderTreeColumnsChanged()));
 
   //Commands not worthy of menu items, but that deserve configurable keybindings
-  new KAction(
+  action = new KAction(
     i18n("Remove Duplicate Messages"), CTRL+Key_Asterisk, this,
     SLOT(removeDuplicates()), actionCollection(), "remove_duplicate_messages");
+  action->plugAccel( actionCollection()->kaccel() );
 
-  new KAction(
+  action = new KAction(
     i18n("Abort Current Operation"), Key_Escape, ProgressManager::instance(),
     SLOT(slotAbortAll()), actionCollection(), "cancel" );
-  mAccel->connectItem(mAccel->insertItem(Key_Escape),
-                     ProgressManager::instance(), SLOT(slotAbortAll()));
+  action->plugAccel( actionCollection()->kaccel() );
 
-  new KAction(
+  action = new KAction(
    i18n("Focus on Next Folder"), CTRL+Key_Right, mFolderTree,
    SLOT(incCurrentFolder()), actionCollection(), "inc_current_folder");
-  mAccel->connectItem(mAccel->insertItem(CTRL+Key_Right),
-                     mFolderTree, SLOT(incCurrentFolder()));
+  action->plugAccel( actionCollection()->kaccel() );
 
-  new KAction(
+  action = new KAction(
    i18n("Focus on Previous Folder"), CTRL+Key_Left, mFolderTree,
    SLOT(decCurrentFolder()), actionCollection(), "dec_current_folder");
-  mAccel->connectItem(mAccel->insertItem(CTRL+Key_Left),
-                     mFolderTree, SLOT(decCurrentFolder()));
+  action->plugAccel( actionCollection()->kaccel() );
 
-  new KAction(
+  action = new KAction(
    i18n("Select Folder with Focus"), CTRL+Key_Space, mFolderTree,
    SLOT(selectCurrentFolder()), actionCollection(), "select_current_folder");
-  mAccel->connectItem(mAccel->insertItem(CTRL+Key_Space),
-                     mFolderTree, SLOT(selectCurrentFolder()));
-  new KAction(
+  action->plugAccel( actionCollection()->kaccel() );
+
+  action = new KAction(
     i18n("Focus on Next Message"), ALT+Key_Right, mHeaders,
     SLOT(incCurrentMessage()), actionCollection(), "inc_current_message");
-    mAccel->connectItem( mAccel->insertItem( ALT+Key_Right ),
-                        mHeaders, SLOT( incCurrentMessage() ) );
+  action->plugAccel( actionCollection()->kaccel() );
 
-  new KAction(
+  action = new KAction(
     i18n("Focus on Previous Message"), ALT+Key_Left, mHeaders,
     SLOT(decCurrentMessage()), actionCollection(), "dec_current_message");
-    mAccel->connectItem( mAccel->insertItem( ALT+Key_Left ),
-                        mHeaders, SLOT( decCurrentMessage() ) );
+  action->plugAccel( actionCollection()->kaccel() );
 
-  new KAction(
+  action = new KAction(
     i18n("Select Message with Focus"), ALT+Key_Space, mHeaders,
     SLOT( selectCurrentMessage() ), actionCollection(), "select_current_message");
-    mAccel->connectItem( mAccel->insertItem( ALT+Key_Space ),
-                        mHeaders, SLOT( selectCurrentMessage() ) );
+  action->plugAccel( actionCollection()->kaccel() );
 
   connect( kmkernel->outboxFolder(), SIGNAL( msgRemoved(int, QString) ),
            SLOT( startUpdateMessageActionsTimer() ) );
@@ -3516,8 +3511,7 @@
 //-----------------------------------------------------------------------------
 void KMMainWidget::setAccelsEnabled( bool enabled )
 {
-  if ( mAccel )
-    mAccel->setEnabled( enabled );
+  actionCollection()->kaccel()->setEnabled( enabled );
 }
 
 
--- branches/KDE/3.5/kdepim/kmail/kmmainwidget.h #553369:553370
@@ -30,7 +30,6 @@
 #include "kmkernel.h" // for access to config
 #include <kaction.h>
 
-class QAccel;
 class QVBoxLayout;
 class QSplitter;
 
@@ -501,8 +500,6 @@
   KXMLGUIClient *mGUIClient;
 
   static QValueList<KMMainWidget*>* s_mainWidgetList;
-
-  QAccel *mAccel;
 };
 
 #endif
Comment 8 Boris Dušek 2006-06-20 23:38:42 UTC
Ingo, thanks for taking care of this. I suggest that the default shortcut for 
"Jump to folder" actions be changed to "SHIFT + Key_P", since "Key_J" collides 
with scrolling down. Attaching a patch that does just this.
Comment 9 Boris Dušek 2006-06-20 23:39:29 UTC
Created attachment 16730 [details]
Fix a default shortcut to SHIFT+Key_P
Comment 10 Till Adam 2006-06-21 00:13:35 UTC
On Tuesday 20 June 2006 23:02, Ingo Klöcker wrote:
> SVN commit 553370 by kloecker:


> I guess the additional plugAccel
> shouldn't be necessary, but at least for Return the accel didn't work
> out of the box (probably because the corresponding KAction doesn't
> appear in any menu or toolbar).


Yes.

Till
Comment 11 Ingo Klöcker 2006-06-21 00:41:46 UTC
I'm sorry, but we won't change the default shortcut from j to something else. We love simple and easy to remember shortcuts (like c for Copy, m for Move and j for Jump).
Comment 12 Magnus Holmgren 2006-06-21 00:52:38 UTC
I agree. I have no problem with it as long as it's configurable. I have changed *many* shortcuts. :-)

Will you make the keys for scrolling configurable next? (As suggested in the infamous(?) bug 96301.) :-)

BTW, this bug was marked duplicate before. Well, it doesn't really matter.
Comment 13 Boris Dušek 2006-06-21 08:28:36 UTC
While I agree that shortcuts should be simple, I do not think that shipping kmail with conflicting default shortcuts is a good thing. Especially when the shortcut that introduces the conflict is a new one and takes precendence over another shortcut that people might have been using for years and *got used* to it. Even more used to it since scrolling with "j" and "k" is available in konqueror too.

But if after reading the previous paragraph you still think that the shortcuts should conflict than I can't do much with it :).
Comment 14 Ingo Klöcker 2006-06-21 09:55:12 UTC
I expect normal users to be far more confused by a Konqueror that scrolls with "j" and "k" than with a KMail which invokes the _J_ump to Folder functionality with "j". Scrolling with "j"/"k" is a relict of the stone age when computer keyboards didn't have arrow keys. If a usability study shows that choosing "j" as shortcut for Jump to Folder was a bad idea, then we'll change it. Until then we'll keep it.

In any case, the shortcut is now configurable and no longer hardcoded. So this bug is resolved.

BTW, thanks for your patches. It's always great to see users who don't just file bug reports and wait for us to fix them, but who also dive into the source code to resolve the problem themselves.