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 mappings for QMouseEvent methods #408

Merged
merged 17 commits into from
Feb 23, 2023

Conversation

StSav012
Copy link
Contributor

@StSav012 StSav012 commented Feb 13, 2023

Added

    QMouseEvent.pos = lambda self: self.position().toPoint()
    QMouseEvent.x = lambda self: self.position().toPoint().x()
    QMouseEvent.y = lambda self: self.position().toPoint().y()
    QMouseEvent.globalPos = lambda self: self.globalPosition().toPoint()
    QMouseEvent.globalX = lambda self: self.globalPosition().toPoint().x()
    QMouseEvent.globalY = lambda self: self.globalPosition().toPoint().y()

for PyQt6 and PySide6.

And I had to add

    QMouseEvent.position = lambda self: QPointF(float(self.x()), float(self.y()))
    QMouseEvent.globalPosition = lambda self: QPointF(float(self.globalX()), float(self.globalY()))

to PyQt5 and PySide2 to make the event test universal and (consequently, but this was not the required enhancement) bidirectional. This way, I check that the coordinates returned by the back-ported functions are equal to what the Qt6 functions return (except for the type, but I use integer coordinates for that to work).

@CAM-Gerlach CAM-Gerlach changed the title Might close https://github.com/spyder-ide/qtpy/issues/394 PR: Add mappings for QMouseEvent methods Feb 14, 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.

Seems good so far, thanks, but would it be possible to actually test that the functions can be called correctly and consistently? That seems to be the user's concern on that issue. Thanks!

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

StSav012 commented Feb 17, 2023

It looks like the test crashes the process on Ubuntu with conda=No. The test stalls on Python3.7 on macOS. There are several tests which express this very behavior. Any thoughts on that?

For the issue with Ubuntu, the most related thread I found on SO was https://stackoverflow.com/questions/56281631/qapplication-instance-qtbot-fixture-causes-travis-ci-to-abort-and-core-dump. Maybe, it'll help.

From the logs, I see that the crash occurs when pytest creates an instance of QApplication with an empty list being the only argument. At home, I get warnings (I don't remember what they read) when I create an app like this: QApplication([]), but not a crash. For now, I can't reproduce the crash locally to prevent it before pushing here.

Oh, wow, I get the same crash in GitHub tests when I create an instance of QApplication manually. That is, there is nothing wrong with pytest. It can't influence QApplication that much, can it?

I had to skip the test for the config for now.

And I can't actually get the coordinates I move the mouse to from the event methods. I can't predict the window border dimensions. Possibly, I missed something. Anyway, the mapping I use should prevent any default values (like QPoint(-1, -1)) from passing the test.

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.

Looks generally good to me, thanks, aside from another minor nit; for more substantive expert feedback I would defer to @dalthviz

qtpy/tests/test_qtgui.py Outdated Show resolved Hide resolved
qtpy/tests/test_qtgui.py Outdated Show resolved Hide resolved
@CAM-Gerlach
Copy link
Member

(Sidenote: @dalthviz , if/when this is merged, it should be presumably done via the Squash strategy (or with rebase cleanup first) to avoid all the repeated fixup commits for a relatively small change. I enabled a setting that will still preserve a list of the commit messages in the commit description, and the full commit history of this branch can still be referred to on the PR in perpetuity, while the history on the main branch is kept clean.)

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 your work here! Besides that seems like there is a change from a PR #407 also here, this LGTM 👍

Do you know what could have happened there @CAM-Gerlach? Also, feel free to squash merge this when you think is ready (seems like there a unresolved conversation)

qtpy/QtCore.py Show resolved Hide resolved
@dalthviz dalthviz added this to the v2.3.1 milestone Feb 22, 2023
@CAM-Gerlach CAM-Gerlach merged commit a112bcf into spyder-ide:master Feb 23, 2023
StSav012 added a commit to StSav012/qtpy that referenced this pull request Feb 23, 2023
See spyder-ide#408 (comment)

Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Differences in QEvent subclass APIs in PyQt6 cause attribute and/or type errors
3 participants