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.

Sign up for GitHub

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

Jump to bottom

Fix problems with the built in help system #1084

Merged
merged 7 commits into from
Feb 16, 2024
Merged

Fix problems with the built in help system #1084

merged 7 commits into from
Feb 16, 2024

Conversation

ejeschke
Copy link
Owner

@ejeschke ejeschke commented Jan 6, 2024 •
edited
Loading

This fixes several problems with the built in help system:

  • The WBrowser plugin is often unable to intialize correctly because it requires a QWebEngineView widget which is often not installed (for example it is offered separately in conda)
  • Even when the widget is installed, it often can fail to start if OpenGL libraries are not configured correctly (often the case for conda on Linux, for example)
  • The RTD web site often fails to access certain items, giving a HTTP 403 (Forbidden) error, particularly with trying to fetch the list if versions (ginga.doc.download_doc._find_rtd_version())
  • The HTML ZIP download (ginga.doc.download_doc. _download_rtd_zip()) sometimes fails as well

Solutions in this PR:

  • WBrowser plugin is deprecated
  • For plugin help, the reference viewer will pop up a dialog to ask the user whether they want to view the documentation in an external browser from the RTD link, or view the (local) plugin docstring in a text widget

@ejeschke ejeschke added this to the 5.0 milestone Jan 6, 2024
@ejeschke ejeschke self-assigned this Jan 6, 2024
@ejeschke
Copy link
Owner Author

ejeschke commented Jan 6, 2024

@pllim, I've had a number of problems with the built in help system. This is meant to address them.

When you have time, can you test this with stginga in your environment?

@pllim
Copy link
Collaborator

pllim commented Jan 6, 2024

@ejeschke , is there a deadline? I am not sure if I can get to this next week.

@ejeschke
Copy link
Owner Author

ejeschke commented Jan 6, 2024

No deadline. Release 5.0 is way overdue, and now it is looking like late Jan or early Feb.

@ejeschke
Copy link
Owner Author

ejeschke commented Jan 8, 2024

Another possibility, @pllim, is to deprecate the WBrowser plugin completely. We could skip trying to download a zip of HTML (which doesn't seem to work for me anymore) and either show the online documentation in an external browser, or docstrings in a text widget if offline, or the user prefers.

@pllim
Copy link
Collaborator

pllim commented Jan 8, 2024

I never used WBrowser myself except to play YouTube in a live demo so I am okay with replacing it with something else.

@ejeschke
Copy link
Owner Author

ejeschke commented Jan 8, 2024

I never used WBrowser myself except to play YouTube in a live demo so I am okay with replacing it with something else.

Ok, sounds good. I feel like the browser widget in Qt (and Gtk) is getting more and more complicated and difficult to support. I read that the Qt browser widget is built on Chrome (!) so it is a pretty complex piece of software to embed.

@ejeschke
Copy link
Owner Author

ejeschke commented Jan 8, 2024

I'll rework this to just deprecate the WBrowser, so hold off reviewing for now...

@pllim
Copy link
Collaborator

pllim commented Jan 8, 2024

I am a bit sad because I used to taunt Tom Robitaille saying that glue cannot play YouTube like Ginga. 😆

@ejeschke
Copy link
Owner Author

ejeschke commented Jan 8, 2024

We could replace it with Tetris or Pong...

@pllim
Copy link
Collaborator

pllim commented Jan 8, 2024

A kid already broke Tetris. What about Doom? 😸

@ejeschke
Copy link
Owner Author

ejeschke commented Jan 8, 2024

I dunno, maybe a space-based game might be best. There are lots to choose from. Asteroid?

@pllim
Copy link
Collaborator

pllim commented Jan 8, 2024

Oh, that is good idea. Or Space Invaders. 🤣

@ejeschke
Copy link
Owner Author

Ok, @pllim, when you have a few minutes to try with stginga, please give it a whirl.

@ejeschke ejeschke mentioned this pull request Jan 12, 2024
Closed
8 tasks
@ejeschke
Copy link
Owner Author

Rebased

@pllim pllim mentioned this pull request Jan 24, 2024
Closed
pllim
pllim reviewed Jan 24, 2024
View reviewed changes
Copy link
Collaborator

@pllim pllim left a comment

Choose a reason for hiding this comment

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

These changes are not backward compatible with existing plugins developed outside Ginga while WBrowser was still a thing.

For example, the Help button for https://github.com/spacetelescope/stginga/blob/master/stginga/plugins/BackgroundSub.py now does nothing.

Maybe you need to also write a transition guide or make the existing Help button from the old way of things somehow invoke the new stuff.

@ejeschke
Copy link
Owner Author

@pllim, ok, good point. Let me rework this slightly to make it backward compatible for the help button API.

@ejeschke
Copy link
Owner Author

@pllim, I have updated the PR so that the behavior for stginga is the same as if the WBrowser was not installed (because it is actually removed in this PR). I tested with BackgroundSub and it now pops up a text window with the docstring in RST. Can you give a quick test to confirm?

To get the behavior of the PR, where it asks the user whether they want the online help rendered in an external browser or the docstring in a text window, I will submit a PR for stginga.

@ejeschke ejeschke mentioned this pull request Jan 25, 2024
Merged
@ejeschke
Copy link
Owner Author

ejeschke commented Feb 7, 2024

Rebased

pllim
pllim approved these changes Feb 15, 2024
View reviewed changes
Copy link
Collaborator

@pllim pllim left a comment

Choose a reason for hiding this comment

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

With spacetelescope/stginga#234 merged, I have no problem with this. Thanks!

@ejeschke
Fix problems with the built in help system
54e6d6a 
This fixes several problems with the built in help system:
- The WBrowser plugin is often unable to intialize correctly because
  it requires a QWebEngineView widget which is often not installed
  (for example it is offered separately in conda)
- Even when the widget is installed, it often can fail to start if
  OpenGL libraries are not configured correctly (often the case for
  conda on Linux, for example)
- The RTD web site often fails to access certain items, giving a
  HTTP 403 (Forbidden) error, particularly with trying to fetch the
  list if versions (ginga.doc.download_doc._find_rtd_version())
- The HTML ZIP download (ginga.doc.download_doc. _download_rtd_zip())
  sometimes fails as well

Solutions in this PR:
- WBrowser is made more robust, so that if it cannot create the
  browser widget the plugin will still start in a degraded mode
- In the degraded mode, WBrowser will pop up a dialog to ask the
  user whether they want to view the documentation in an external
  browser from the RTD link, or view the (local) plugin docstring
  in a text widget
- If the browser widget is successfully created, but the RTD version
  check fails, or the download of the zipped documentation fails,
  it will fall back to displaying the online documentation; if that
  fails it will fall back to showing the local plugin docstring in
  a text widget.
@ejeschke
Remove WBrowser plugin
5ed277e 
- now handles URLs by calling Python's "webbrowser" module to open them
- option to view local docstring in a plain text widget
@ejeschke
Remove WebView widget from various Widgets.py
58e30cc
@ejeschke
Fix Help menu item
7864b33
@ejeschke
Remove requirements for beautifulsoup and docutils
68da715
@ejeschke
Fix backward compatibility with _help_docstring()
63708c1
@ejeschke
Fix to allow other ginga implementations to refer to their online help
e47e088
@ejeschke ejeschke merged commit 0f976ac into ejeschke:main Feb 16, 2024
10 checks passed
@ejeschke ejeschke deleted the fix-help-system branch February 16, 2024 03:27
@ejeschke
Copy link
Owner Author

Thanks, @pllim!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pllim pllim pllim approved these changes

Assignees

@ejeschke ejeschke

Labels
bug documentation plugin reference viewer widget
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
2 participants
@ejeschke @pllim