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

Status bar messages to indicate rendering and viewing progress #233

Merged
merged 6 commits into from
Feb 5, 2025

Conversation

mbway
Copy link
Contributor

@mbway mbway commented Feb 21, 2021

since the application doesn't look visually different when it is rendering/drawing and when you can interact with it, I think it would be useful if there was some way to determine if CQ-Editor is currently busy at a glance. The status bar isn't currently being used for anything so I used that to display messages:

time.sleep(1.5)  # to simulate a long running script
sphere = cq.Workplane('XY').move(6, 6).sphere(3)
loft = cq.Workplane('XY').ellipse(5, 7).workplane(5).circle(4).workplane(5).ellipse(7, 5).loft()
box = cq.Workplane('XY').move(-8, -2).box(3, 4, 2)

show_object(loft, 'my_loft')
show_object(sphere, 'my_sphere')
show_object(box, 'my_box')
statusbar.mp4

@mbway
Copy link
Contributor Author

mbway commented Feb 21, 2021

It seems to be because I'm calling QApplication.processEvents() inside set_status_message() some unit tests are failing. This is currently required for the status messages to be visible as rendering blocks the main thread.

In the test_console() unit test from master (which passes) if I insert these two lines:

    # test print_text
    pos_orig = console._prompt_pos
    console.print_text('a')
    
+    from PyQt5.QtWidgets import QApplication
+    QApplication.processEvents()
    
    assert(console._prompt_pos == pos_orig + len('a'))

then the test fails because the prompt is printed

print(repr(console._control.document().toPlainText()))
'a\n\nIn [1]: \n\nIn [2]: '

and with processEvents called inside set_status_message the content is: '\nIn [1]: a'

Any advice on how best to handle this? should I update the unit tests or find another workaround (not that I can think of any)

@jmwright
Copy link
Member

@mbway If you are still interested in pursuing this PR, could you rebase or otherwise resolve the conflicts so that it can be tested and merged?

@mbway
Copy link
Contributor Author

mbway commented Jan 31, 2025

ok. I don't use cq-editor anymore but I'm happy to help finalise this PR. Are you also interested in my other PR #220 ? I'll bring that one up to date as well.

btw it would be nice if a pyproject.toml was offered to make it easier to install into a regular virtual environment rather than a conda one. Introducing linting would really help as well.

@jmwright
Copy link
Member

Are you also interested in my other PR #220 ?

I have not had time to look closely at it yet, but if you update it, I'll spend some time testing and reviewing it.

btw it would be nice if a pyproject.toml was offered to make it easier to install into a regular virtual environment rather than a conda one

That is the plan for the 0.3 release. Conda will still be fully supported, I just want to support pip better.

@mbway
Copy link
Contributor Author

mbway commented Jan 31, 2025

turns out there were no conflicts with #220 since I last merged, but I pulled in the latest changes anyway

@jmwright
Copy link
Member

@mbway Thanks for your willingness to update these PRs.

@mbway
Copy link
Contributor Author

mbway commented Jan 31, 2025

the problem I mentioned here is still relevant #233 (comment)

I decided to fix that problem by checking against the text rather than the cursor position which seems to not get updated.

@mbway
Copy link
Contributor Author

mbway commented Feb 1, 2025

I think the remaining test failures (test_reload_import_handle_error and test_modulefinder) are intermittent? they are timing based and don't fail when I run on my machine.

@jmwright
Copy link
Member

jmwright commented Feb 1, 2025

The tests pass on my local system too. Maybe it would be worth setting the TIMEOUT on those to something like 5000 to see if it is completely stalling when in CI, or maybe just on the edge.

@jmwright
Copy link
Member

jmwright commented Feb 1, 2025

Actually, I am able to reproduce now locally with 4 tests timing out. I will investigate more.

@jmwright
Copy link
Member

jmwright commented Feb 1, 2025

I am out of time for tonight. The following are the 4 tests that fail for me locally.

FAILED tests/test_app.py::test_editor_autoreload - pytestqt.exceptions.TimeoutError: Signal triggerRerender(bool) not emitted after 500 ms
FAILED tests/test_app.py::test_autoreload_nested - pytestqt.exceptions.TimeoutError: Signal triggerRerender(bool) not emitted after 500 ms
FAILED tests/test_app.py::test_reload_import_handle_error - pytestqt.exceptions.TimeoutError: Signal triggerRerender(bool) not emitted after 500 ms
FAILED tests/test_app.py::test_modulefinder - pytestqt.exceptions.TimeoutError: Signal triggerRerender(bool) not emitted after 500 ms

All of those tests fail while waiting on the triggerRerender signal to be fired because it never is.

I also see this deprecation warning, which may or may not be related. I do see the window displayed when I run just one test by itself locally.

pytestqt/qtbot.py:267: DeprecationWarning: waitForWindowShown is deprecated, as the underlying Qt method was obsoleted in Qt 5.0 and removed in Qt 6.0. Its name is imprecise and the pytest-qt wrapper does not raise qtbot.TimeoutError if the window wasn't shown. Please use the qtbot.waitExposed context manager instead.

Copy link

codecov bot commented Feb 2, 2025

Codecov Report

Attention: Patch coverage is 98.43750% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.88%. Comparing base (d746c8e) to head (8454b5e).
Report is 17 commits behind head on master.

Files with missing lines Patch % Lines
cq_editor/widgets/viewer.py 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
+ Coverage   89.26%   89.88%   +0.62%     
==========================================
  Files          19       19              
  Lines        1583     1621      +38     
  Branches      157      161       +4     
==========================================
+ Hits         1413     1457      +44     
+ Misses        139      133       -6     
  Partials       31       31              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jmwright
Copy link
Member

jmwright commented Feb 3, 2025

@mbway I am running your branch and do not see the status bar messages like you show in your video. Is there something that has to be enabled in preferences?

@mbway
Copy link
Contributor Author

mbway commented Feb 3, 2025

have you tried with the example I posted above which takes a while to compute? It is working for me on Linux. There is no associated preferences for this feature.

@jmwright
Copy link
Member

jmwright commented Feb 3, 2025

have you tried with the example I posted above which takes a while to compute?

Yes, that was the second thing I tried. Is your branch up-to-date with master? I had to pull master into it when I checked our the PR branch.

@mbway
Copy link
Contributor Author

mbway commented Feb 3, 2025

Is your branch up-to-date with master

I merged master into the branch mbway:status_bar 3 days ago. The last commit to CadQuery:master was 5 days ago. Github also indicates 'No conflicts with base branch'

@jmwright
Copy link
Member

jmwright commented Feb 3, 2025

The last commit to CadQuery:master was 5 days ago.

You mean CQ-editor:master, correct?

@mbway
Copy link
Contributor Author

mbway commented Feb 3, 2025

I copied from the github links
image
mbway/CadQuery are the github users that the forks correspond to, but yes both are referring to the CQ-Editor repository.

I can rebase onto master instead of merging if that makes things clearer?

@jmwright
Copy link
Member

jmwright commented Feb 3, 2025

It feels like Github has subtly changed the way the merge requests are displayed, but maybe it's my imagination.

The PR branch will not update properly for me when I pull it down, so I switched to your fork and am running it that way. I have been having all sorts of weird issues with my Python and git toolchains lately.

Your fork/branch looks good to me. I'll use it as my daily driver for a bit to see if there are any issues. Thanks.

@jmwright
Copy link
Member

jmwright commented Feb 5, 2025

I have been running this feature for a couple days and have not had any issues. I'm going to go ahead and merge it for the 0.4 development cycle. Thanks @mbway !

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.

2 participants