Bug 338380 - obtrusive missing declaration widget
Summary: obtrusive missing declaration widget
Status: RESOLVED FIXED
Alias: None
Product: kdevelop
Classification: Applications
Component: Language Support: CPP (old) (show other bugs)
Version: 4.6.90
Platform: Other macOS
: NOR major
Target Milestone: 4.7.0
Assignee: kdevelop-bugs-null
URL:
Keywords: release_blocker, usability
Depends on:
Blocks:
 
Reported: 2014-08-19 16:10 UTC by RJVB
Modified: 2014-08-24 23:18 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
patch to assistantpopup to render it less obtrusive (on OS X) (6.00 KB, patch)
2014-08-22 11:14 UTC, RJVB
Details
exported shortcut scheme I use on OS X (7.72 KB, application/zip)
2014-08-22 11:20 UTC, RJVB
Details

Note You need to log in before you can comment on or make changes to this bug.
Description RJVB 2014-08-19 16:10:07 UTC
In the KDevelop 4.7 beta (and probably in git/master and maybe even in kdev-clang), the C++ missing declaration widget has become too intrusive.
The improved visibility compared to 4.6 (stable) is fine, but not the way the keyboard is grabbed.
On OS X, I redefined the shortcuts so that Alt-Left/Right map to the system-standard word-left/right. That mapping no longer works as soon as the cursor is in the scope of a call with a missing declaration (ex. QVERIFY2 just now, but think of any OS X API calls for which KDevelop fails to find the headers). Instead, my cursor stays put, and I have to hit Alt-0 once each time before I can jump a word left or right. That's a major impairment caused by something that *could* be useful (in my experience, it so rarely is that I'd remove keyboard control from it completely).

Please correct this - or point me to where that would have to be done and I'll see if I can propose a solution.
Comment 1 Kevin Funk 2014-08-19 16:46:49 UTC
Ugh, that's bad.

@RJVB: You may have a look at shell/assistantpopup.{cpp,h} in kdevplatform.git
Comment 2 RJVB 2014-08-20 09:44:11 UTC
I was going to say that the widget in question is posted from a kdevelop source file, but then I noticed that apparently there are other cases not concerning missing declarations that block my keystrokes/shortcuts.

What should I be looking for in assistantpopup? And would it help if I posted a map with my shortcuts, for more testing (if it's possible to export them to file, of course)?
Comment 3 RJVB 2014-08-20 16:13:35 UTC
OK, seems that AssistantPopup::eventFilter grabs focus indiscrimately (assistantpopup.cpp:308-310) whenever the Alt key is pressed.

I don't see many ways around that. Either one can do without the focus grab in general, or else one would have to release it as soon as the user hits a key other than 0-9 (in ::keyPress). If `QDeclarativeView::keyPressEvent(event)` returns the event to the dispatcher for handling by another handler, that would have to be called immediately after releasing the focus ... and then maybe one ought to grab focus again, for good measure?

In addition, in ::eventFilter, why is the check for Esc pressed (and executeHideAction) done *after* grabbing the focus? Seems you'd be grabbing focus for nothing in that case?
Comment 4 RJVB 2014-08-20 20:20:40 UTC
I tried this, without any noticeable effect:

    void AssistantPopup::grabFocus()
    {
        setFocus();
        m_config->setActive(true);
    }

    void AssistantPopup::releaseFocus()
    {
        m_config->setActive(false);
        if (m_view) {
            m_view->setFocus();
        }
    }

    void AssistantPopup::keyPressEvent(QKeyEvent* event)
    {
        if (event->key() >= Qt::Key_0 && event->key() <= Qt::Key_9) {
            auto actions = m_config->model();
            const int field = event->key() - Qt::Key_0;
            if (field == 0) {
                executeHideAction();
            } else {
                auto action = m_assistantActions.value(field - 1);
                if (action) {
                    action->execute();
                }
            }
        } else {
            bool restoreActive = m_config->isActive();
            if( restoreActive ){
                releaseFocus();
            }
            QDeclarativeView::keyPressEvent(event);
            if( restoreActive ){
                grabFocus();
            }
        }
    }


    void AssistantPopup::keyReleaseEvent(QKeyEvent *event)
    {
        if (event->key() == Qt::Key_Alt || event->modifiers() == Qt::AltModifier) {
            releaseFocus();
        } else {
            bool restoreActive = m_config->isActive();
            if( restoreActive ){
                releaseFocus();
            }
            QDeclarativeView::keyReleaseEvent(event);
            if( restoreActive ){
                grabFocus();
            }
        }
    }

is there another way to make sure that key strokes (even with Alt pressed) not handled by the AssistantPopup end up in the view/widget that received them?
Comment 5 RJVB 2014-08-21 18:47:09 UTC
(grrr, have to type this in a 2nd time, thx Chrome for not letting me back up and retrieve my text!)

I added some printing code dumping the received keypresses in AssistantPopup::eventFilter, on Linux. Turns out that even with the Alt key held, existing shortcuts never arrive at the widget, i.e. I don't see those key strokes on my terminal. Other combinations (except for 0-9 of course) are printed, and appear in my kdevelop edit window.
This would explain why the widget/AssistantPopup isn't as obtrusive on Linux as on OS X (I'd have to double-check that existing shortcuts DO arrive at the widget there, and that there's really no way to pass them on).

On a related not: it's a bit annoying that the popup keeps popping up esp. after you've explicitly told it to hide for a given symbol. How hard/costly would it be to add a "no, thanks" choice that keeps track of which symbols are not to be added as a class member? Or else, how would one propose/publish an option to deactivate the whole missing declaration assistant (there's no configuration UI for the C++ plugin, right)? Should I file a wish ticket for that?
Comment 6 RJVB 2014-08-22 11:14:22 UTC
Created attachment 88370 [details]
patch to assistantpopup to render it less obtrusive (on OS X)

I think I came up with a workaround that ought to be acceptable. In its current implementation, the AssistantPopup that presents the user with the suggestion to add a missing declaration to the class being edited grabs focus as soon as the user presses the Alt key (without any other modifiers). It retains focus until the user releases the Alt key or the user hits one of keys 0-9. Other keys are passed on to the corresponding upstream event handler, while the AssistantPopup::eventFilter always returns false without explicitly passing on the event.

That latter behaviour seems to be at odds with the eventFilter example from the Qt documentation, which passes on the event to the parent class in the default return statement. My patch implements that.

My patch also implements a number of changes also already made in kdevplatform git/master (parallel evolution...!): the grabFocus and releaseFocus (aka ungrabFocus) methods.

The main workaround to the issue at hand lies in treating any Alt+key combination other than Alt+0-9 as a "hide widget" command equivalent to Alt+0, coupled with an explicit focus release. This still eats the first shortcut stroke (for instance, Alt-Left to backup a word) but I find it less inconvenient to have to repeat a shortcut (because the 1st did nothing) rather than having to take another action to unblock the shortcut. With my patches, subsequent shortcuts work as expected, even if the assistant reappears (presumably because it doesn't re-grab focus, I'm not exactly sure what modification caused this).

I've also changed the logic for releasing focus somewhat: rather than checking for modifiers() == AltModifier, I check if the flag is set in the mask, I think that ought to include more cases where focus ought to be released.

Finally, I've added some tests for the assistant hotkeys Alt+0-9 in the eventFilter itself. They probably ought never be tripped. at least not as long as the popup has focus grabbed. These checks are part of an attempt to do without focus grabbing (only using a state variable instead), but for some reason eventFilter doesn't receive the expected Key_0-9 events (on OS X) when the Alt key is pressed.
Comment 7 RJVB 2014-08-22 11:20:45 UTC
Created attachment 88371 [details]
exported shortcut scheme I use on OS X

A good test-case (= the situation in which I ran into the issue) is when you import a KF5 project (say, KWallet) without having Qt5 or any other KF5 frameworks installed. The project cannot be fully configured in that case, and the C++ plugin/code parser only has access to definitions from the project itself. That leaves it with lots of missing declarations that it can propose to add to the class open in an editor ...

(Why would I have done such a thing? Simple, to create a Review Request from changes I made in KDE 4.12 and that could be ported over almost verbatim.)
Comment 8 Olivier.jg 2014-08-23 15:33:33 UTC
Should be fixed in 4.7 with 0fcaedc
Comment 9 RJVB 2014-08-23 21:04:18 UTC
(In reply to Olivier.jg from comment #8)
> Should be fixed in 4.7 with 0fcaedc

I haven't yet tried with 4.7 (I'd have to change my Portfile to refer to a git repo?), but I did try with kdevelop git/master on OS X.

Your solution indeed fixes the obtrusiveness issue, but at least on OS X 10.6.8 with KDE 4.12.5, the Alt+2 shortcut doesn't work (Alt+1 Alt+3 and Alt+0 do). Alt+2 cancels the popup by inserting the Alt+2 symbol (™ on my system) in the editor view ...

I verified, this does not seem to be because of an apparently missing call `        qt_set_sequence_auto_mnemonic(true)` (which may or may not be required on OS X to enable shortcuts).
Comment 10 Olivier.jg 2014-08-23 21:55:22 UTC
Sounds like Alt + number shortcuts are a bad idea on OSX, since at least Alt+2 is reserved at the OS level. Perhaps on OSX we could use a different shortcut. Can you recommend any other modifier + 0-9 combination? Or any other idea?
Comment 11 RJVB 2014-08-24 08:04:56 UTC
I'm not aware that Alt+2 is reserved on OS X rather than being part of the standard keymap (which admittedly boils down to something similar, but it might explain why it overrides the shortcut definition).
About shortcuts: https://developer.apple.com/library/mac/documentation/userexperience/conceptual/applehiguidelines/KeyboardShortcuts/KeyboardShortcuts.html

Your best bet might be the replace Alt with however Qt calls the Command ("Apple") key. It's the closest in function to the Alt modifier on Linux and MS Windows, and Command+0-9 don't have a standard, system-wide application.

What happens when you use QShortCut to (re)define temporary shortcuts; are the pre-existing definitions restored when the temporary definitions go out of scope?
Comment 12 Olivier.jg 2014-08-24 10:08:15 UTC
Commit 4ecc68a in master changes the modifier used on OSX to CTRL/Command, at least in theory. Could you test if that works for you?
Comment 13 Milian Wolff 2014-08-24 11:28:48 UTC
Now that we use shortcuts, can't we make these user-definable as well? That would make it possible for others to redefine these to their likings, no?
Comment 14 RJVB 2014-08-24 23:18:28 UTC
(In reply to Olivier.jg from comment #12)
> Commit 4ecc68a in master changes the modifier used on OSX to CTRL/Command,
> at least in theory. Could you test if that works for you?

Yep, seems to be fine!

> Now that we use shortcuts, can't we make these user-definable as well? That
> would make it possible for others to redefine these to their likings, no?

That would be nice, but from what I understood AssistantPopup is a sort of helper class that's used by different features, among which the missing declaration widget. I suppose that's why shortcuts 0-9 are hardwired