Project Manager View plugin has hardcoded/fixed shortcuts
Needs RevisionPublic

Authored by rjvbb on May 2 2016, 8:16 AM.

Details

Reviewers
mwolff
Group Reviewers
KDevelop
Summary

From https://bugs.kde.org/show_bug.cgi?id=362545 :

The Project Manager View plugin hardcodes a number of shortcuts, among others for item removal, renaming, copy and paste. The remove, copy and paste shortcuts are standard and probably unlikely to be redefined, but the rename action is made available via the F2 key. I use that shortcut in KWin4 (for lowering the focus window), and it turns out that the keystroke is not eaten by KWin but passed on to the next window that receives focus, or when KDevelop itself is being lowered this way.

I've tried to make a start towards replacing these fixed shortcuts with user-modifiable ones. The QAction shortcuts work as expected, but I haven't yet been able to figure out how to make them appear in the shortcut settings dialog. Draft patch attached.

Reproducible: Always

Steps to Reproduce:

  1. Map F2 to "lower window"
  2. Select a Focus-Really-Follows-Mouse focussing model
  3. Open a project in KDevelop5
  4. Select an item in the project manager
  5. Put a window in front of KDevelop and lower it via F2 or lower KDevelop itself via F2

Actual Results:
A Rename dialog pops up; one cannot change or undefine the shortcut

Expected Results:
It should be possible to change or remove the shortcut

The undesirable described effect above happens for me on a KDE4/Plasma desktop but should occur in all situations where one of the shortcuts is mapped globally to a different action.

Diff Detail

Repository
R33 KDevPlatform
Lint
Lint Skipped
Unit
Unit Tests Skipped
rjvbb updated this revision to Diff 3596.May 2 2016, 8:16 AM
rjvbb retitled this revision from to Project Manager View plugin has hardcoded/fixed shortcuts.
rjvbb updated this object.
rjvbb edited the test plan for this revision. (Show Details)
rjvbb set the repository for this revision to R33 KDevPlatform.
rjvbb added a project: KDevelop.
rjvbb added a subscriber: KDevelop.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptMay 2 2016, 8:16 AM
mwolff added a subscriber: mwolff.May 24 2016, 9:01 PM

While configurability is always nice, do note that all shortcuts are standard shortcuts for their purpose, F2 e.g. to rename is ubiquitous - it works in dolphin and many other Qt apps with editable item views, but it also works under Window's Explorer e.g.

plugins/projectmanagerview/projectmanagerview.cpp
151

can't you connect directly to the action's triggered slot instead of doing the custom comparisons here?

plugins/projectmanagerview/projectmanagerviewplugin.cpp
86

one initializer per line:

Foo()
    : m1(...)
    , m2(...)
    ...
{}
296–307

you'll need to use KActionCollection which depends on KXMLGui to get configurable shortcuts.

plugins/projectmanagerview/projectmanagerviewplugin.h
62

all getters should be const, also move the * next to the typename please

mwolff requested changes to this revision.May 24 2016, 9:01 PM
mwolff added a reviewer: mwolff.

marking this patch to require changes

This revision now requires changes to proceed.May 24 2016, 9:01 PM
rjvbb marked 2 inline comments as done.May 25 2016, 2:10 PM
In D1523#30919, @mwolff wrote:

While configurability is always nice, do note that all shortcuts are standard shortcuts for their purpose, F2 e.g. to rename is ubiquitous

Evidently ubiquitous doesn't include OS X ;)
Regardless of that, I've been using that F2 mapping I mentioned since well before MS Windows 3 (supposing that's where the F2=Rename mapping originated). F1=Raise, F2=Lower, F5=Minimise etc. might come from DomainOS but could also be a personal choice. Too long ago.

plugins/projectmanagerview/projectmanagerview.cpp
151

I don't know, I don't even know why it isn't enough to use QAction::setShortCut to call (in this case) ProjectManagerViewPlugin::removeFromContextMenu(). It should, according to the QAction documentation, because that's what the triggered signal is connected to.
May I presume there's a reason that even the Copy and Paste actions already did a manual check (supposing KStandardAction does define shortcuts for them)?

The other thing is that ProjectManagerViewPlugin::removeFromContextMenu() calls removeItems() with ProjectManagerViewPlugin::itemsFromIndexes(), and I don't have the slightest idea how different or not that is from ProjectManagerView::selectedItems().

plugins/projectmanagerview/projectmanagerviewplugin.cpp
296–307

adding the actions to actionCollection() indeed makes them show up in the shortcut settings, but I must still be doing something wrong because changes I make don't "stick".

plugins/projectmanagerview/projectmanagerviewplugin.h
62

"official" KDE guidelines have the "*" and "&" attached to the variable, no (like QObject *parent in the ctor)?

rjvbb updated this revision to Diff 4001.May 25 2016, 2:12 PM
rjvbb edited edge metadata.
rjvbb marked an inline comment as done.

updated patch

rjvbb marked 2 inline comments as done.May 25 2016, 2:13 PM
rjvbb added a comment.Jun 3 2016, 12:08 PM

I'm not getting the shortcut changes applied through an actionCollection() to work. They show up, and I do get conflict warnings about Key_Delete until I change the shortcut on the Project Manager's "Remove..." action. But for some reason the old shortcuts remain active.

I've also realised that these shortcuts are apparently enabled only when the project manager has focus, so not when you're editing a file for instance. That's logical, but IMHO it also means that there's a lot less point in having shortcuts. Shortcuts are extremely useful if they can prevent you from having to go for the mouse, but if you're already using the mouse instead of the keyboard you can almost apply the opposite argument. IOW, I'm now running a local patch which removes the F2 shortcut (just use a right-click to select a file to be renamed and use the context menu). It also remaps the Remove shortcut to the one used in the Finder; that could of course be a Mac-specific change.

diff --git plugins/projectmanagerview/projectmanagerview.cpp plugins/projectmanagerview/projectmanagerview.cpp
index f08fe23..e8eeca1 100644
--- plugins/projectmanagerview/projectmanagerview.cpp
+++ plugins/projectmanagerview/projectmanagerview.cpp
@@ -146,12 +146,12 @@ bool ProjectManagerView::eventFilter(QObject* obj, QEvent* event)
     if (obj == m_ui->projectTreeView) {
         if (event->type() == QEvent::KeyRelease) {
             QKeyEvent* keyEvent = static_cast<QKeyEvent*>(event);
-            if (keyEvent->key() == Qt::Key_Delete && keyEvent->modifiers() == Qt::NoModifier) {
+            if (keyEvent->key() == Qt:: Key_Backspace && keyEvent->modifiers() == Qt::ControlModifier) {
                 m_plugin->removeItems(selectedItems());
                 return true;
-            } else if (keyEvent->key() == Qt::Key_F2 && keyEvent->modifiers() == Qt::NoModifier) {
-                m_plugin->renameItems(selectedItems());
-                return true;
+//             } else if (keyEvent->key() == Qt::Key_F2 && keyEvent->modifiers() == Qt::NoModifier) {
+//                 m_plugin->renameItems(selectedItems());
+//                 return true;
             } else if (keyEvent->key() == Qt::Key_C && keyEvent->modifiers() == Qt::ControlModifier) {
                 m_plugin->copyFromContextMenu();
                 return true;
mwolff requested changes to this revision.Feb 18 2017, 10:50 PM

The same shortcut must be usable in different contexts, and the context is the focus. Just like now F2 allows to rename an item in every editable list view, this should continue to work. I don't want to have N different shortcuts for "rename item" in every list view.

In general, I think solving this in KDevelop is the wrong place. Rather, this may need to be done globally in KF5 (Qt integratation?). E.g. have a central KStandardAction::rename action with a globally defined shortcut.

This revision now requires changes to proceed.Feb 18 2017, 10:50 PM
rjvbb added a comment.Feb 19 2017, 9:22 AM

Agreed.

I presume you're running a Plasma5 desktop; could you check if KWin5 and/or kglobalacceld still propagate keystrokes (like F2) that are custom shortcuts for KWin actions as described in the summary for KWin4? I guess that's actually the underlying issue.

In D1523#87467, @mwolff wrote:

In general, I think solving this in KDevelop is the wrong place.

Did you see the thread I started on the frameworks ML? KDevelop *might* be the right place after all, unless there's a specific reason why ProjectManagerView::eventFilter() uses KeyRelease instead of KeyPress?

I could imagine using KeyRelease for dangerous actions like Delete that may not post a confirmation dialog, so that users can back out by pressing an additional key before releasing, say, the Backspace key. But actions like rename that will always post a dialog can just as well be triggered by KeyPress events.

This would be a much simpler mod that should be almost transparent from the ("normal") user's point of view: