[Gui] Propage Mouse events using Qt signals

[nmellado]

• 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)

Be aware that the PR request cannot be accepted if it doesn't pass the Continuous Integration tests.

• What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
  Feature

• What is the current behavior? (You can also link to an open issue here)
  Mouse events are handled by the viewer and its inheriting classes

• What is the new behavior (if this is a feature change)?
  Mouse events are propagated through QT signals, which allows to use them in plugins

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

• Other information:

[nmellado]

This trick is already used in some plugins.
It is true it does not fit the keymapping system, but that is a quick fix that enable many interesting functionalities, to control user interaction from Radium plugins.

[codecov]

Codecov Report

Merging #848 (e7279ba) into release-candidate (24029b5) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head e7279ba differs from pull request most recent head 5ee5a11. Consider uploading reports for the commit 5ee5a11 to get more accurate results

@@                  Coverage Diff                  @@
##           release-candidate     #848      +/-   ##
=====================================================
- Coverage              26.41%   26.41%   -0.01%     
=====================================================
  Files                    321      321              
  Lines                  21410    21413       +3     
=====================================================
  Hits                    5656     5656              
- Misses                 15754    15757       +3     

Impacted Files	Coverage Δ
src/Gui/Viewer/Viewer.cpp	0.00% <0.00%> (ø)
src/Gui/Viewer/Viewer.hpp	0.00% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dedcd39...5ee5a11. Read the comment docs.

[dlyr]

I do not get the point why do we need two different kinds of behavior, one with callback for keyboard, one with signal for mouse ?
Both are fine for me, but maybe having something coherent is better ?

[nmellado]

I do agree.
But this allows to use existing code with low effort and now, while the other option will take more time to be developed (and it should be).

[MathiasPaulin]

Simple, working but questionable :

• how to prevent the needUpdate (or indirecly frameUpdate) to be called twice (or even once) if the listener is implemented such that it asks for update on some cases but do not want to update the frame in another situation.
• how to develop an application/plugin/listener object using this functionality? Please update the demo or propose a simple one.
  - this method being Qt specific, it should require to develop more code than needed to have portable component or to share the mouse behavior with keyboard behavior.

Based on this question, I propose you tag the signals as « deprecated  » and the PR as « temporary fix(hack) » so that developpers will not use this « hack  » too much in their apps

As I understand the functionallity as being important and quite urgent, i do not argue against this PR but it should definitely be replaced as soon as possible by a more robust/general approach based on Qt independant callbacks. (Similar to keyboard évent propagation)

[MathiasPaulin]

Simple, working but questionable :

• how to prevent the needUpdate (or indirectly frameUpdate) to be called twice (or even once) if the listener is implemented such that it asks for update on some cases but do not want to update the frame in another situation.
• how to develop an application/plugin/listener object using this functionality? Please update the demo or propose a simple one.
• this method being Qt specific, it should require to develop more code than needed to have portable component or to share the mouse behavior with keyboard behavior.

Based on this question, I propose you tag the signals as « deprecated » and the PR as « temporary fix(hack) » so that developers will not use this « hack » too much in their apps

As I understand the functionality as being important and quite urgent, i do not argue against this PR but it should definitely be replaced as soon as possible by a more robust/general approach based on Qt independent callbacks. (Similar to keyboard event propagation)

[nmellado]

how to prevent the needUpdate (or indirectly frameUpdate) to be called twice (or even once) if the listener is implemented such that it asks for update on some cases but do not want to update the frame in another situation.

As written is the signal documentation, needUpdate will be called after the signal is emitted anyway. So there is no need to call needUpdate explicitly in the listener.

how to develop an application/plugin/listener object using this functionality? Please update the demo or propose a simple one.
this method being Qt specific, it should require to develop more code than needed to have portable component or to share the mouse behavior with keyboard behavior.

This thing is useful only for plugins: currently, anyone writing his own application can override the handleMouseXXXEvent in his custom class inhering Ra::Gui::Viewer. So yes, it is Qt specific, but the plugin system is currently relying on Qt. Also, it is a Qt signal which can be connected to any Qt slot: I can write it in the documentation, but it is very very standard and I don't see the point. Writing a demo app is not working either, as it makes no sense for apps.

@dlyr has open an issue to remember that we need a better implementation (#850).

So, what I can do:

• mark the signals as deprecated
• add _ temporary fix(hack)_ in the commit message.

Does that work for you both ?

[dlyr]

I'm not for deprecated since there is no current alternative.

[MathiasPaulin]

This thing is useful only for plugins: currently, anyone writing his own application can override the handleMouseXXXEvent in his custom class inhering Ra::Gui::Viewer. So yes, it is Qt specific, but the plugin system is currently relying on Qt. Also, it is a Qt signal which can be connected to any Qt slot: I can write it in the documentation, but it is very very standard and I don't see the point. Writing a demo app is not working either, as it makes no sense for apps.

One can develop a specific app without inheriting from viewer or something else. All DemoApps do this. Just use the BaseApplication and its Viewer, define a specific window factory (or use the provided ones) and implement application specific logic without worrying about Qt, Viewer ... take a look on, e.g. KeyEventDemoApp the defines its own Qt window to manage events and implement actions on the event (not separated from Qt-related stuff but this can be done quite easily and is a recommandation of all CHI software development workflow).
One can also separate Qt-related event management (for mouse or keyboard actions) from application/library/plugin logic using observer/observable pattern.

But this is out of topic of this PR even if a simple demo of the usage might be helpfull.

So, what I can do:

• mark the signals as deprecated
• add _ temporary fix(hack)_ in the commit message.

Does that work for you both ?

Fine for me.

[nmellado]

Tell me what to do with the deprecated flag.

[MathiasPaulin]

Suggested change

	void onMousePress( QMouseEvent* event );
	[[deprecated]] void onMousePress( QMouseEvent* event );

Note that for deprecated method, some IDE will not automatically complement their name from any prefix and will display the available method as strikethrough. This might discourage users to abuse from ready-to-be-removed features ;)

[MathiasPaulin]

Suggested change

	void onMouseRelease( QMouseEvent* event );
	[[deprecated]] void onMouseRelease( QMouseEvent* event );

[MathiasPaulin]

Suggested change

	void onMouseMove( QMouseEvent* event );
	[[deprecated]] void onMouseMove( QMouseEvent* event );

[MathiasPaulin]

I'm not for deprecated since there is no current alternative.

Sure, but as thereis no demo nor publicly available source code that use this, we need to prevent users to do it until a better solution is implemented (what was requested is a method similar to key-event propagation) but with more questions about how to manage the priority between colliding events (if the client want to manage the same mouse event than the viewer, who takes the priority ?).

And for me, deprecated does not mean there is another way to do it right now but that this will be removed without notifications in the future, e.g. when a better alternative is available or when the functionality will become useless.