napari / napari-console

A plugin that adds a console to napari
BSD 3-Clause "New" or "Revised" License
3 stars 12 forks source link

Set underlying console QTextEdit widget as proxy for QtConsole focus #23

Closed aganders3 closed 1 year ago

aganders3 commented 1 year ago

This is meant to be a precursor to fixing https://github.com/napari/napari/issues/5166, allowing napari to programmatically set the keyboard focus on the console.

This is draft for now as I intended to open this in my own fork and need to fix the test failures in CI. Sorry!

Czaki commented 1 year ago

the test failure is connected with imageio 2.22.1 release You need to block this version or wait until the next imageio release (as the bugfix is already merged in imageio).

aganders3 commented 1 year ago

the test failure is connected with imageio 2.22.1 release You need to block this version or wait until the next imageio release (as the bugfix is already merged in imageio).

Thanks - this saved me some digging and worked well, but now the tests fail for a reason I'm struggling with.

It seems to me likely related to https://github.com/pytest-dev/pytest-qt/issues/206 but I'm going to close this PR until I can figure it out in my own fork.

Czaki commented 1 year ago

I'm familiar with this type of problem. Point it out, and maybe I could provide you a proper solution.

aganders3 commented 1 year ago

Thank you! I managed to get it working in my fork but of course appreciate your input on the changes. I'll reopen this so you can look here.

Here's a successful run: https://github.com/aganders3/napari-console/actions/runs/3190233617

Changes to CI to make it work:

The pytest-qt docs recommended herbstluftwm. I tried to replace with twm but it would hang on my test. My concern about removing xvfb-action and pytest-xvfb is that those packages seemed to do some cleanup and I'm not sure if it needs to be added back.

codecov[bot] commented 1 year ago

Codecov Report

Merging #23 (db99f30) into main (52eb3c4) will increase coverage by 4.98%. The diff coverage is 88.57%.

@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
+ Coverage   83.76%   88.74%   +4.98%     
==========================================
  Files           4        4              
  Lines         117      151      +34     
==========================================
+ Hits           98      134      +36     
+ Misses         19       17       -2     
Impacted Files Coverage Δ
napari_console/conftest.py 81.81% <81.81%> (ø)
napari_console/_tests/test_qt_console.py 100.00% <100.00%> (ø)
napari_console/qt_console.py 87.05% <100.00%> (+7.29%) :arrow_up:
napari_console/__init__.py

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

aganders3 commented 1 year ago

Thanks - my impression is that any minimal WM should work and I only selected herbstluftwm because it was suggested by the pytest-qt docs. I did very briefly try using twm but it was not immediately working. Let me know if there's anything else you'd like me to try. If there's a way to do this without running any WM at all that would be great!

Czaki commented 1 year ago

When I locally test, it is enough to set the environment available to QT_QPA_PLATFORM=offscreenand add QT_QPA_PLATFORM to passenv section in tox.

This should allow us to not use xvfb at all. But I'm not sure which variant is better.

aganders3 commented 1 year ago

Oh neat thanks - I was not aware of QT_QPA_PLATFORM.

psobolewskiPhD commented 1 year ago

This PR along with https://github.com/napari/napari/issues/5166 works wonderfully as advertised. I also tested +/- each of the PRs and there are no issues with one or the other is missing.

aganders3 commented 1 year ago

Thanks again for all the review effort and suggestions!