Remove QtWebEngine and replace with system browser#3847
Remove QtWebEngine and replace with system browser#3847
Conversation
|
Will this get us to only needing PySide6-Essentials as a (build)dependency? (no need for PySide6-Addons)? Some quick grepping makes me think you might have already got there. That would be fantastic. |
|
There's still one remaining Addons dependency: from PySide6.QtDataVisualization import Q3DScatter, QScatter3DSeries, QScatterDataItem, QValue3DAxisWe need it for the Shape2SAS 3D scatter plot viewer. Replacing it with something from Essentials (e.g., matplotlib 3D plots or a custom implementation) to fully drop the Addons dependency. |
|
This PR can go |
QtDataVisualization is actually deprecated. iirc it caused some issues with the Flatpak, although I don't remember how it was resolved. According to the QT documentation, we're supposed to use QT Graphs instead. |
| return type('Enum', (), enums) | ||
|
|
||
|
|
||
| def _get_online_docs_url(relative_path: Path) -> str: |
There was a problem hiding this comment.
Testing locally, I deleted my docs folder after launching the app and the base fitting help url becomes https://www.sasview.org/docs/v6.1.2.dev159+g77be83657/C:/Users/jkrzywon/AppData/Local/sasview/SasView/6.1.2/doc/build/html/user/qtgui/Perspectives/Fitting/fitting_help.html, which gives a 404 error.
The extended version and HELP_SYSTEM.path passed to showHelp() will need to be accounted for.
There was a problem hiding this comment.
Having file path / URL calculations in both GuiManager.py:showHelp() and GuiUtils.py:showHelp() feels fragile - getting the separation of concerns sorted out helps avoid the double-calculation and all the associated bugs that we have struggled with in the docs for so long.
I wonder if both should end up in src/sas/system/_help.py, taking logic out of the GUI and putting it one place, with a couple of tests in test/system/utest_help.py that check the logic remains robust.
I'm also curious what we see as being the use-case for the online fallback - I think we still want to distribute the docs - that's an important principle for the user to have the documentation to hand.
There was a problem hiding this comment.
I'm also curious what we see as being the use-case for the online fallback - I think we still want to distribute the docs - that's an important principle for the user to have the documentation to hand.
When I wrote the ADR, my idea was that the online fallback would only be used if there is a bug in retrieving/packaging the offline documentation. Ideally, this should never have to be used but if there is a bug, we should probably show some documentation rather than just nothing.
| def _get_online_docs_url(relative_path: Path) -> str: | ||
| """Construct online documentation URL for the current SasView version.""" | ||
| from sas.system.version import __version__ | ||
| base_url = f"https://www.sasview.org/docs/v{__version__}" |
There was a problem hiding this comment.
Looking at that version string in Jeff's URL, perhaps __version__ isn't what we want, particularly during development/pre-release. The reduced_version from src/sas/qtgui/Utilities/WhatsNew/newer.py could move to src/sas/system/version.py, and/or expose __version_tuple__ in version.py.
| from sas.system.version import __version__ | ||
| base_url = f"https://www.sasview.org/docs/v{__version__}" | ||
| # Ensure forward slashes for URL (Windows paths use backslashes) | ||
| path_str = str(relative_path).replace("\\", "/") |
There was a problem hiding this comment.
If it's a Path coming in, then there are library methods for this already - relative_path.as_posix() https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.as_posix
4 participants
Description
This is a preliminary version of the QtWebEngine-less SasView.
All calls have been converted to consistent system browser calls.
References to QtWebEngine were removed.
Tests updated (although we don't run these tests, anyway)
Fixes #3511
How Has This Been Tested?
Locally on Win11
Review Checklist:
[if using the editor, use
[x]in place of[ ]to check a box]Documentation (check at least one)
Installers
Licensing (untick if necessary)