Closed bburan closed 2 years ago
Great I wished I did not release 0.15.0 some hours ago. I will review ASAP. If you can try to add tests.
@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.
Sure no rush on the tests. I would just like to improve them to be able to better trust them.
LGTM
Let me know when to merge.
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.
Merging #486 (545713b) into main (e97409a) will decrease coverage by
0.01%
. The diff coverage is47.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
@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).
@frmdstryr Made your requested change.
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.
I just checked and I believe we do not use the deprecated part of Action regarding handling menus.
@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'
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
.
@MatthieuDartiailh I have updated the original comment with a checklist so we can track state of testing/debugging Qt6 fixes.
browse button broken
This should be fixed when qtpy 2.1 is released (https://github.com/spyder-ide/qtpy/pull/331).
@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.
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.
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.
I made some progress with testing and made a PR against @bburan branch at https://github.com/bburan/enaml/pull/1
I will try to look at the other failures next.
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.
I opened #489 to track all the remaining issues. I suggest we merge this as is and I will reopen https://github.com/bburan/enaml/pull/1 as a separate PR against nucleic repo. Having small PRs will be more manageable I believe. WDYT @bburan ?
@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.
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
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