Expose binding

[dlyr]

• Please check if the PR fulfills these requirements

• The commit message follows our guidelines
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

• What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
  Expose binding in KeyMappingManager, so client code can inspect and manage binding.

• What is the current behavior? (You can also link to an open issue here)
  No access to binding.

• What is the new behavior (if this is a feature change)?
  Nothing changes, this will allow to have better event management, like the one done for custom event in viewer, for all kind of event.

Todo in an upcomming PR: update viewer event handling with something like (example code adapted from WIP ...)

void Viewer::setupActionCallbacks() {
    setupActionCallback("ACTION_NAME", [this]( QEvent* e ) {
        if ( e->eventType() == QEvent::QPress ) { /* key press action */ }
    } );
}

void Viewer::setupActionCallback( std::string actionName,
                                    std::function<void( QKeyEvent* )> callback ) {
    auto keyMappingManager = KeyMappingManager::getInstance();
    auto context           = thisKeyMapping::getContext();
    auto actionIndex       = keyMappingManager->getActionIndex( context, actionName );
    auto binding           = keyMappingManager->getBinding( context, actionIndex );
    if ( binding )
    {
        if ( binding->isKeyEvent() )
        {
            m_customActions[KeyEventType::KeyPressed].insert( { actionIndex, callback } );
            m_customActions[KeyEventType::KeyReleased].insert( { actionIndex, callback } );
        }
        if ( binding->isMouseEvent() )
        {
            m_customActions[KeyEventType::MousePressed].insert( { actionIndex, callback } );
            m_customActions[KeyEventType::MouseMoved].insert( { actionIndex, callback } );
            m_customActions[KeyEventType::MouseReleased].insert( { actionIndex, callback } );
        }
        if ( binding->isWheelEvent() )
        {
            m_customActions[KeyEventType::WheelActivated].insert( { actionIndex, callback } );
        }
    }
    else
    { LOG( logERROR ) << "setupActionCallback: binding not found for  " << actionName; }
}

• Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
  No

[codecov]

Codecov Report

Merging #906 (3185b50) into release-candidate (af38ec3) will decrease coverage by 0.47%.
The diff coverage is 48.98%.

❗ Current head 3185b50 differs from pull request most recent head 6497844. Consider uploading reports for the commit 6497844 to get more accurate results

@@                  Coverage Diff                  @@
##           release-candidate     #906      +/-   ##
=====================================================
- Coverage              44.51%   44.04%   -0.48%     
=====================================================
  Files                    337      336       -1     
  Lines                  22477    22430      -47     
=====================================================
- Hits                   10006     9879     -127     
- Misses                 12471    12551      +80     

Impacted Files	Coverage Δ
src/Gui/Viewer/CameraManipulator.hpp	0.00% <ø> (ø)
src/Gui/Viewer/FlightCameraManipulator.cpp	0.00% <0.00%> (ø)
src/Gui/Viewer/Gizmo/GizmoManager.cpp	0.00% <ø> (ø)
src/Gui/Viewer/RotateAroundCameraManipulator.cpp	0.00% <ø> (ø)
src/Gui/Viewer/TrackballCameraManipulator.cpp	0.00% <0.00%> (ø)
src/Gui/Viewer/Viewer.cpp	0.00% <0.00%> (ø)
src/Gui/Viewer/Viewer.hpp	0.00% <ø> (ø)
src/Gui/Utils/KeyMappingManager.inl	26.92% <26.92%> (ø)
src/Gui/Utils/KeyMappingManager.cpp	82.02% <84.21%> (+5.28%)	⬆️
src/Gui/Utils/KeyMappingManager.hpp	100.00% <100.00%> (ø)
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[dlyr]

On second thought, the callback collection could also be on the KeyMappingManager side. With current proposed code, handle event is

    auto actionPainty = keyMap->getAction( thisKeyMapping::getContext(), buttons, modifiers, key );
    auto itr          = m_customActions[KeyEventType::KeyPressed].find( actionPainty );

    if ( itr != m_customKeyActions[KeyEventType::KeyPressed].end() )
    {
        itr->second( event );
        return true;
    }

I can try to have setup like

void Viewer::setupActionCallbacks() {
     auto keyMappingManager = KeyMappingManager::getInstance(); 
     keyMappingManager->setupActionCallback(thisKeyMapping::getContext(), "ACTION_NAME", [this]( QEvent* e ) {
        if ( e->eventType() == QEvent::QPress ) { /* key press action */ }
    } );
}

And handle event could be

    auto actionTriggered = keyMap->triggerAction( thisKeyMapping::getContext(), buttons, modifiers, key );
    if( !actrionTriggered) {
        // try another context
    }

[MathiasPaulin]

On second thought, ...

I find this second option, with the callback collection on the KeyMappingManager side, quite better.
It simplifies the Viewer code (no need to manage callbacks collection) and is rather compact for configuration and handling.
Do you add it in this PR or do you open an issue to keep track of the discussion ?

[dlyr]

In this PR, later

[dlyr]

This PR will closes #793

[dlyr]

I've done some test ... but it's not as versatile as before.
For instance, for TrackballCameraManipulator, the manipulator register its callback to the keymappingmanager.
But this means only instance of TrackballCameraManipulator should have registered its callbacks at some point ... and I'm not sure how to unregister the callback.
See current code version (last commits) to discuss.

[dlyr]

Question here, currently keymapping configuration file is stored as a QDomDocument during loading, and updated when one adds new binding, so that when saved, the dom document contains loaded + added actions.
I'm working on a writer that use actual binding data (from the keymappingmanager's maps) to save the configuration file.
The main difference is that loaded file is lost, with it's optionnal comments, and anything not related to keymapping, and the saved file node order might change.
I think it's cleaner, but I'm open to feedback.

[nmellado]

I guess the question is: do we want to have persistent comments in this file ?
I would be tempted to say that we don't, and I would go even further: in most cases, users will never open this file (and if they do, they know what they are doing ;) ).
So we can do whatever with this file, even changing the action order.

In the long term, it would be beneficial to have an UI to edit this file.

[MathiasPaulin]

I do not understand how everything work nor if the proposal is the right way to do that but it seems to allow finer customization of the gui event processing system. So, if others approve, I'll do not argue against their decision. I've just a concern on what seems to me to be a strong limitation as custom binding (at least as done in the EntityAnimationDemo, which is the only demo to rely on numeric-key events ) seems to be dependent of the langage (or configuration) of the keyboard.

[nmellado]

Ready to merge, please update wrt to release_candidate.
Thanks !