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

Enaml qt6 fixes (for deprecated methods) #486

Merged
merged 6 commits into from
Jun 10, 2022

Conversation

bburan
Copy link
Contributor

@bburan bburan commented Apr 1, 2022

I discovered all of these issues when testing with the desktop example.

I have started testing against my application (psiexperiment) and may have more to report later.

Examples to check

  • aliases
  • applib
  • dynamic
  • functions
  • layout
  • stdlib
  • styling
  • templates
  • tutorial
  • widgets
  • workbench

Broken examples

  • drag_and_drop.enaml (drag and drop broken)
  • file_dialog.enaml (browse button broken)
  • flow_area.enaml (changing flow layout)
  • image_view.enaml (set to None)
  • ipython_console.enaml (possibly just need to install qtconsole),
  • vtk_canvas.enaml (most likely need to install vtk)
  • web_view.enaml

This is not backwards-compatible with Qt5, so we need to check QT_VERSION.
In Qt5, it seems to have required QPointF as well. Not sure where
the deprecation occurred.
To replicate this bug (without the commit):

* Run examples\workbench\sample.py
* Switch to persistent workspace
* Drag one of the dock items so it is floating
* Switch to workspace 1 or 2
* Switch back to the persistent workspace
@MatthieuDartiailh
Copy link
Member

Great I wished I did not release 0.15.0 some hours ago. I will review ASAP. If you can try to add tests.

@bburan
Copy link
Contributor Author

bburan commented Apr 1, 2022

@MatthieuDartiailh I discovered these only after testing by dragging the dock panes around. I see there is something called qtbot that can be used for automating tests. Pretty cool. I am not familiar with pytest-qt, so I will have to study this as I need to emulate a mouse click and drag on the dockitem titlebar. I may not be able to get to this for a few days to weeks.

@MatthieuDartiailh
Copy link
Member

Sure no rush on the tests. I would just like to improve them to be able to better trust them.

@MatthieuDartiailh
Copy link
Member

LGTM

Let me know when to merge.



def hover_event_pos(event):
if QT_VERSION[0] == "6":
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of checking the version in every event it'd be slightly better to define a different function

if QT_VERSION[0] == "6":
    def hover_event_pos(event):
        return event.position().toPoint()
else:
    def hover_event_pos(event):
        return event.pos()

Copy link
Member

Choose a reason for hiding this comment

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

I wanted to suggest the same thing but then confused myself about switching the qt version at runtime...

@MatthieuDartiailh
Copy link
Member

Looking for use of pos() it looks like there are several other candidate for similar fixes. I will try to make a PR based on this one if nobody beats me to it.

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2022

Codecov Report

Merging #486 (545713b) into main (e97409a) will decrease coverage by 0.01%.
The diff coverage is 47.05%.

@@            Coverage Diff             @@
##             main     #486      +/-   ##
==========================================
- Coverage   73.16%   73.15%   -0.02%     
==========================================
  Files         317      318       +1     
  Lines       24125    24143      +18     
  Branches       55       55              
==========================================
+ Hits        17652    17661       +9     
- Misses       6473     6482       +9     

@bburan
Copy link
Contributor Author

bburan commented Apr 6, 2022

@MatthieuDartiailh I don't think any of those additional pos() calls qualify for substitution as those are all QMouseEvent where pos is still a valid public function (https://doc-snapshots.qt.io/qt6-dev/qmouseevent.html).

@bburan
Copy link
Contributor Author

bburan commented Apr 6, 2022

@frmdstryr Made your requested change.

@MatthieuDartiailh
Copy link
Member

MatthieuDartiailh commented Apr 7, 2022

Going through the Qt6 deprecation, I think we may also want to check the way we use actions and menus (in the workbench for example).
I will do it if I find the time, I just wanted to share my research results.

@MatthieuDartiailh
Copy link
Member

I just checked and I believe we do not use the deprecated part of Action regarding handling menus.

@bburan
Copy link
Contributor Author

bburan commented Apr 8, 2022

@MatthieuDartiailh Hold off on merging this. I just ran across an error with PyQt6 when testing my application. Did not have a chance to look at it in more detail yet. Posting this here for reference.

Traceback (most recent call last):
  File "c:\users\lbhb\projects\psi2\src\enaml\enaml\qt\q_popup_view.py", line 736, in mousePressEvent
    if not path.isEmpty() and not path.contains(pos):
TypeError: arguments did not match any overloaded call:
  contains(self, QPointF): argument 1 has unexpected type 'QPoint'
  contains(self, QRectF): argument 1 has unexpected type 'QPoint'
  contains(self, QPainterPath): argument 1 has unexpected type 'QPoint'

Note that the rect sill requires QPoint and doees not appear to
accept QPointF so we need to keep both.
@bburan
Copy link
Contributor Author

bburan commented Apr 12, 2022

I have fixed the bug in the previous comment. However, there are additional bugs. Going through the Enaml widgets examples and interacting with them has revealed a few more. Right now at least drag_and_drop.enaml (drag and drop broken), file_dialog.enaml (browse button broken), flow_area.enaml (changing flow layout), image_view.enaml (set to None), ipython_console.enaml (possibly just need to install qtconsole), vtk_canvas.enaml (most likely need to install vtk), web_view.enaml.

@bburan
Copy link
Contributor Author

bburan commented Apr 12, 2022

@MatthieuDartiailh I have updated the original comment with a checklist so we can track state of testing/debugging Qt6 fixes.

@frmdstryr
Copy link
Contributor

browse button broken

This should be fixed when qtpy 2.1 is released (spyder-ide/qtpy#331).

@bburan
Copy link
Contributor Author

bburan commented Apr 12, 2022

@MatthieuDartiailh Do you know if the toPython methods on the QDate and QTime objects in PyQt6 would be fixed? I found it odd that they were dropped for QDate and QTime but not QDateTime.

@MatthieuDartiailh
Copy link
Member

Thanks @bburan that will be very helpful. Regarding QDate and QTime no I have no clue.

I am trying to finish some stuff on pegen to unblock the related PR and I will comlz back to this right after.

@MatthieuDartiailh
Copy link
Member

I started working on writing tests for the parts that require some changes under Qt6 but did not go very far due to bugs in qtest. I will let you know when I make progress.

@MatthieuDartiailh
Copy link
Member

I made some progress with testing and made a PR against @bburan branch at bburan#1

I will try to look at the other failures next.

@MatthieuDartiailh
Copy link
Member

For the image view I do not see a hard crash just PyQt complaining about an invalid size and I see the same on Qt5. Did you see the same behavior @bburan ?

For the flow layout it appears to be more of the float vs int issues that has been plaguing us for some time now. Testing for this will never be bulletproof I fear.

@MatthieuDartiailh MatthieuDartiailh mentioned this pull request May 2, 2022
19 tasks
@MatthieuDartiailh
Copy link
Member

I opened #489 to track all the remaining issues. I suggest we merge this as is and I will reopen bburan#1 as a separate PR against nucleic repo. Having small PRs will be more manageable I believe. WDYT @bburan ?

@bburan
Copy link
Contributor Author

bburan commented Jun 9, 2022

@MatthieuDartiailh Not sure how I missed this message. Yes, I think merging as-is is fine then we can use bburan#1 as the separate PR. Agree regarding small PRs.

@MatthieuDartiailh MatthieuDartiailh merged commit aa77d38 into nucleic:main Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants