Skip to content

Allow unhandled key and mouse-wheel events to bubble up.#552

Merged
rahulporuri merged 4 commits into
masterfrom
fix/key-event-bubbling
Feb 3, 2021
Merged

Allow unhandled key and mouse-wheel events to bubble up.#552
rahulporuri merged 4 commits into
masterfrom
fix/key-event-bubbling

Conversation

@rkern

@rkern rkern commented Jan 26, 2021

Copy link
Copy Markdown
Member

Fixes #550

Might have unintended consequences for existing applications, so consider it a WIP for testing.

When a Qt widget receives a QKeyEvent that it does not handle, it is supposed to call the ignore() method to allow the event to bubble up to the parent widget. KeyPress and KeyRelease events are accepted by default, so if not explicitly ignored, the in-focus Enable widget will swallow all of these QKeyEvents.

We should consider doing the same for wheelEvent(), as QWheelEvents have the same semantics. This has led to issues when Enable components are embedded in a scrolled group.

Fixes #550

Might have unintended consequences for existing applications, so consider it
a WIP.
@jwiggins

Copy link
Copy Markdown
Member

Basically LGTM. Can we try to test this and make a decision about wheelEvent in the next week or so? It would be good to get this into the 5.0.0 release.

@corranwebster

Copy link
Copy Markdown
Contributor

Getting this in for 5.0.0 would mitigate the issues around potential breakage of existing code - it is a major release so some differences in behaviour are acceptable as long as we can highlight them.

@rkern rkern changed the title WIP: Allow unhandled key events bubble up. Allow unhandled key and mouse-wheel events to bubble up. Jan 28, 2021
@rkern

rkern commented Jan 28, 2021

Copy link
Copy Markdown
Member Author

I fixed a logic error (the _on_key_*() convenience methods don't actually return the handled flag) and added the wheelEvent modification.

@rkern

rkern commented Jan 28, 2021

Copy link
Copy Markdown
Member Author

Here's a simple manual test adapted from #550

Pressing d should print out keybinding hotkey pressed whether the Chaco plot has focus or not (click on the Chaco plot to give it focus).
Pressing n while the Chaco widget has focus should print out chaco hotkey pressed.
Scrolling with the mouse wheel should Just Work even when the mouse pointer crosses the Chaco plot (it will stop on the Chaco plot on master).

import os
import numpy as np

import logging
logging.basicConfig(level=logging.DEBUG)

from traits.api import Event, HasStrictTraits, Instance, Button
from traitsui.api import UItem, ModelView, VGroup, View
from traitsui.key_bindings import KeyBinding, KeyBindings
from chaco.api import ArrayPlotData, Plot
from enable.api import BaseTool, KeySpec, ComponentEditor


class HotkeyTool(BaseTool):
    key_pressed = Event

    next_key = Instance(KeySpec, args=("n",))

    def normal_key_pressed(self, event):
        if self.next_key.match(event):
            self.key_pressed = 'next'
            event.handled = True


class CustomView(ModelView):

    model = Instance(HasStrictTraits, ())

    dummy_button = Button('Do Nothing')

    image_plot = Instance(Plot)
    hotkey_tool = Instance(HotkeyTool)

    def _kb_hotkey_pressed(self, info=None):
        print('keybinding hotkey pressed')

    def _chaco_key_pressed(self, event):
        if event.new == 'next':
            print('chaco hotkey pressed')

    def _image_plot_default(self):
        plot_data = ArrayPlotData(
            intensity=np.random.randint(0, 255, size=(100, 100)))
        plot = Plot(plot_data)
        tool = HotkeyTool(plot)
        tool.observe(self._chaco_key_pressed, 'key_pressed')
        plot.tools.append(tool)
        plot.img_plot('intensity')
        return plot

    def default_traits_view(self):
        key_bindings = KeyBindings(
            KeyBinding(binding1='D',
                       description='Hotkey',
                       method_name='_kb_hotkey_pressed')
        )
        return View(
            VGroup(
                UItem('dummy_button'),
                # Make sure the image plot takes up some space.
                UItem('image_plot', editor=ComponentEditor(), height=-400),
                # Add a bunch of dummy spaces so we get a scrollbar to test the
                # wheel event handling.
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                UItem('_'),
                scrollable=True,
            ),
            key_bindings=key_bindings,
            resizable=True,
        )


if __name__ == '__main__':
    from traits.etsconfig.api import ETSConfig
    config_dir = os.path.join(ETSConfig.application_data, 'traits')
    if os.path.exists(config_dir):
        for f in os.listdir(config_dir):
            if f.startswith('traits_ui.'):
                os.remove(os.path.join(config_dir, f))

    view = CustomView()
    view.configure_traits()

@jwiggins jwiggins left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@rkern

rkern commented Jan 28, 2021

Copy link
Copy Markdown
Member Author

I'll leave merging to the team working on the 5.0.0 release.

@jwiggins jwiggins added the ETS Backlog Good issue for ETS team members to look at label Jan 28, 2021
@jwiggins jwiggins requested a review from rahulporuri January 28, 2021 16:47

@rahulporuri rahulporuri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I was able to use the provided manual script to verify that this change works as expected on pyqt and pyqt5 on windows.

@rkern Can you make time to add a regression test in this PR? I'm not sure how easy/difficult it will be to add the test.

@rahulporuri

Copy link
Copy Markdown
Contributor

Thanks @rkern . I resolved the merge conflicts. Merging.

@rahulporuri rahulporuri merged commit 1550a3f into master Feb 3, 2021
@rahulporuri rahulporuri deleted the fix/key-event-bubbling branch February 3, 2021 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ETS Backlog Good issue for ETS team members to look at

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keybindings does not work with Chaco

4 participants