Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR: Add compatibility mappings between bindings for all children of QSinglePointEvent #417

Conversation

StSav012
Copy link
Contributor

@StSav012 StSav012 commented Mar 24, 2023

Analyzing #408, I've noticed that the functions I made for Qt6 are not actually of QMouseEvent, but belong to its base class QSinglePointEvent. So, that's the quick fix for that. I'm sorry I haven't done this several days earlier.

Unfortunately, I couldn't think of good tests for QNativeGestureEvent, QEnterEvent, QTabletEvent, QHoverEvent, and QWheelEvent (edit: it has always had Qt6-style position and globalPosition), which all are the child classes of QSinglePointEvent in Qt6. I'd really appreciate some help here.

@StSav012 StSav012 marked this pull request as ready for review March 24, 2023 14:44
@CAM-Gerlach CAM-Gerlach added this to the v2.3.1 milestone Mar 24, 2023
@CAM-Gerlach
Copy link
Member

Since this is a bugfix to an existing change in 2.3.1, it seems like it would be nice to get this in that release also before the previous change ships, to avoid any discruption.

Unfortunately, I couldn't think of good tests for QNativeGestureEvent, QEnterEvent, QTabletEvent, QHoverEvent, and QWheelEvent, which all are the child classes of QSinglePointEvent in Qt6. I'd really appreciate some help here.

Maybe can be omitted for now, or just check that they have the expected type and are subclasses of isinstance, and/or the correct attributes? It doesn't have to be fancy, and our tests currently aren't really that sophisticated generally—you've already gone above and beyond in a lot of cases :)

@StSav012
Copy link
Contributor Author

Added dummy checks (assert something is not None).

As a side note, would qtbot.waitExposed help the stalls of the tests on macOS CI with Python 3.7? The docs read

This is mainly useful for asynchronous systems like X11, where a window will be mapped to screen some time after being asked to show itself on the screen.

I'd like to try, but not in a PR here.

@CAM-Gerlach CAM-Gerlach changed the title Fix #394 for all children of QSinglePointEvent PR: Fix #394 for all children of QSinglePointEvent Mar 24, 2023
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

One comment on how to simplify the tests

qtpy/tests/test_qtgui.py Outdated Show resolved Hide resolved
@StSav012
Copy link
Contributor Author

One comment on how to simplify the tests

That actually is what the review process for. Thanks, @CAM-Gerlach!

One of the tests failed, but it's not me this time. I wish there was a way to stabilize the test environment and reuse it to speed the tests up.

@CAM-Gerlach
Copy link
Member

I restarted the failed job. If it keeps happening, I can look into it further or explore other options like locked dependencies or additional caching.

@dalthviz dalthviz changed the title PR: Fix #394 for all children of QSinglePointEvent PR: Add compatibility mappings between bindings for all children of QSinglePointEvent Mar 27, 2023
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @StSav012 for the work here! This LGTM 👍 but just in case, what do you think @ccordoba12 ? And also, is there any other thing that should be addressed before merging this @CAM-Gerlach ?

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @StSav012! I left some small suggestions for you to avoid very long lines in the code, otherwise looks good to me.

qtpy/QtGui.py Outdated Show resolved Hide resolved
qtpy/QtGui.py Outdated Show resolved Hide resolved
qtpy/QtGui.py Outdated Show resolved Hide resolved
qtpy/QtGui.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtgui.py Outdated Show resolved Hide resolved
qtpy/QtGui.py Outdated Show resolved Hide resolved
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM, at least as far as my expertise is concerned 👍

@StSav012
Copy link
Contributor Author

StSav012 commented Mar 28, 2023

Thanks, @ccordoba12! What line length do you prefer? I stick to 120 char limit. I'll gladly follow your code style to save you the extra work on cosmetics.

@CAM-Gerlach, please apply such formatting changes without consulting me to save time (especially when a commit delays a release). Living in Europe, I read the comments many hours after they appear.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
@CAM-Gerlach
Copy link
Member

Thanks, @ccordoba12! What line length do you prefer? I stick to 120 char limit. I'll gladly follow your code style to save you the extra work on cosmetics.

At least in most Spyder projects, nominally 79 characters per PEP 8, but the current code is somewhat inconsistent until we blacken it (and even then, we might want to go with the default 88 characters to avoid too much churn and to avoid too many line breaks on the large number of complex lines QtPy has). I've tried to avoid too many style nitpicks myself except when in the course of another suggestion.

@CAM-Gerlach, please apply such formatting changes without consulting me to save time (especially when a commit delays a release). Living in Europe, I read the comments many hours after they appear.

Sure, I can do that for you in the future if you like; we just generally like to be very respectful of the author's wishes and not do so without their explicit permission (which you've now given) to avoid any friction and misunderstandings (as has happened in the past). Thanks!

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me now, thanks @StSav012!

@ccordoba12 ccordoba12 merged commit b8291d8 into spyder-ide:master Mar 28, 2023
@StSav012 StSav012 deleted the use_QSinglePointEvent_instead_of_its_children_in_Qt6 branch April 3, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants