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: Make QtWebEngine Optional #22196

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hmaarrfk
Copy link
Contributor

Description of Changes

I've been involved with packaging on conda-forge for the last few years and unfortunately the situation for packaging QtWebEngine hasn't really improved.

I think Spyder is a REALLY important tool for those starting off with python and being able to co-install it in modern environments is also a really great asset.

However, ensuring that qt-webengine is updated is a constant challenge and we are having a hard time keeping up especially on windows and OSX.

I would really like to continue to recommend Spyder to users, but if there isn't a clear path toward compatibility with qt6 (for which we don't have the webengine plugin) it makes it a really hard sell.

The following patch would effectively silently disable the plugins that require WebEngine.

One modification I would like to propose is the addition of a flag for each plugin like:

REQUIRES_WEBENGINE

That way, plugins beyond help could declare themselves as requiring webengine and the main application would be able to disable them intelligently

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

The main visual difference is that by default the help tab no longer shows up:
image

Example showing matplotlib still working:
image

Issue(s) Resolved

Xref: conda-forge/qt-main-feedstock#49 (comment)

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: Mark Harfouche

@hmaarrfk
Copy link
Contributor Author

One thing I would like to do, is to run the tests to ensure that this continues working. I'm not sure if it would be appropriate to add an other CI job, but I can look into it if you want.

@ccordoba12
Copy link
Member

Hey @hmaarrfk, nice to see you here and thanks for all your contributions during the last days! You said:

I think Spyder is a REALLY important tool for those starting off with python and being able to co-install it in modern environments is also a really great asset.

First of all, thanks a lot for your kind words. In what feels to be a Jupyter monoculture of GUIs to do scientific programming in Python, we really value them. And second, thanks for helping us to move forward to support Qt6.

However, ensuring that qt-webengine is updated is a constant challenge and we are having a hard time keeping up especially on windows and OSX.

Is this also a problem for Qt5?

I would really like to continue to recommend Spyder to users, but if there isn't a clear path toward compatibility with qt6 (for which we don't have the webengine plugin) it makes it a really hard sell.

That's a very good point and I agree with you.

One modification I would like to propose is the addition of a flag for each plugin like:

REQUIRES_WEBENGINE

That way, plugins beyond help could declare themselves as requiring webengine and the main application would be able to disable them intelligently

I really like this idea. Would you be ok with implementing it as part of this PR? If not, this is (of course) the right first step in that direction, so we'd merge it first and implement your suggestion in a follow up PR.

@ccordoba12 ccordoba12 changed the title Make QtWebEngine Optional PR: Make QtWebEngine Optional Jun 25, 2024
@ccordoba12 ccordoba12 added this to the v6.0beta3 milestone Jun 25, 2024
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Some small suggestions for you, otherwise looks good to me.

spyder/plugins/help/widgets.py Outdated Show resolved Hide resolved
spyder/plugins/help/widgets.py Show resolved Hide resolved
spyder/plugins/help/widgets.py Show resolved Hide resolved
spyder/plugins/ipythonconsole/widgets/main_widget.py Outdated Show resolved Hide resolved
spyder/plugins/onlinehelp/widgets.py Outdated Show resolved Hide resolved
spyder/plugins/onlinehelp/widgets.py Show resolved Hide resolved
spyder/plugins/onlinehelp/widgets.py Show resolved Hide resolved
spyder/plugins/onlinehelp/widgets.py Show resolved Hide resolved
@hmaarrfk
Copy link
Contributor Author

However, ensuring that qt-webengine is updated is a constant challenge and we are having a hard time keeping up especially on windows and OSX.

Is this also a problem for Qt5?

Generally yes.... the only reason that things have been "prompt" to update on the qt-webengine side is that Qt vendors so many of the dependencies. Its also what parts of webengine engine are depended on for users of conda-forge. Building "everything" is not a feasible target.

One modification I would like to propose is the addition of a flag for each plugin like:

REQUIRES_WEBENGINE

I really like this idea. Would you be ok with implementing it as part of this PR? If not, this is (of course) the right first step in that direction, so we'd merge it first and implement your suggestion in a follow up PR.

If you can choose the name of the attribute, then I can implement it this weekend.

Thank you for the rest of the review, I'll take a look at it "soon"

@ccordoba12
Copy link
Member

If you can choose the name of the attribute, then I can implement it this weekend.

I prefer REQUIRE_WEB_WIDGETS to make it easier to understand for people not familiar with Qt.

@hmaarrfk
Copy link
Contributor Author

I feel like the plugin code should be more resilient in general to missing dependencies:

  1. pygithub not installed
  2. asyncssh
  3. yarl: /home/mark/git/spyder/spyder/plugins/remoteclient/api/jupyterhub/init.py
  4. aiohttp /home/mark/git/spyder/spyder/plugins/remoteclient/api/jupyterhub/init.py

I mostly bring this up because maybe you don't that added property?

@pep8speaks
Copy link

pep8speaks commented Jun 26, 2024

Hello @hmaarrfk! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-06-27 21:00:55 UTC

spyder/app/mainwindow.py Outdated Show resolved Hide resolved
@mrclary
Copy link
Contributor

mrclary commented Jun 27, 2024

@ccordoba12, would the help plugin require qt-webengine if it only rendered plain text? I'm wondering if there could be some fallback solution in the absence of qt-webengine that could still leave the help plugin operable.

Co-authored-by: Ryan Clary <9618975+mrclary@users.noreply.github.com>
@ccordoba12
Copy link
Member

I mostly bring this up because maybe you don't that added property?

@hmaarrfk, we did. You can check our lastet setup.py (or meta.yaml in Conda-forge) and you'll see them declared there.

I think this problem only happens in development mode because in that case Spyder is not installed properly. So, you can run it in an env that doesn't contain all dependencies required in master.

A possible fix would be to do a required (not optional) deps check at startup in development mode and inform users if some are missing. @mrclary, do you have a bit of free time to implement that?

would the help plugin require qt-webengine if it only rendered plain text? I'm wondering if there could be some fallback solution in the absence of qt-webengine that could still leave the help plugin operable.

@mrclary, no, it doesn't. But I think the Help's UI would look like a bug if we'd only display its plain text mode. However, we can talk about that in a follow-up issue and decide what to do about that.

@hmaarrfk
Copy link
Contributor Author

I think this problem only happens in development mode because in that case Spyder is not installed properly. So, you can run it in an env that doesn't contain all dependencies required in master.

yeah i get it.

I mostly was surprised to see the new features!!!! congrats!

@mrclary
Copy link
Contributor

mrclary commented Jun 28, 2024

A possible fix would be to do a required (not optional) deps check at startup in development mode and inform users if some are missing. @mrclary, do you have a bit of free time to implement that?

Shouldn't that already be reported on startup, regardless of DEV?

container.compute_dependencies()

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.

None yet

4 participants