-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat: use embedded qt browser for jdaviz standalone #3188
base: main
Are you sure you want to change the base?
feat: use embedded qt browser for jdaviz standalone #3188
Conversation
Nope... |
Starting server on http://localhost:60717 :( |
b3f6983
to
565107e
Compare
jdaviz/cli.py
Outdated
|
||
app = QApplication([""]) | ||
web = QWebEngineView() | ||
web.resize(1000, 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what we should do here.
I included optional dependencies now @camipacifici , so running
Should get you the correct dependencies. |
Thank you! I had to install also |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3188 +/- ##
==========================================
- Coverage 88.12% 87.75% -0.37%
==========================================
Files 127 128 +1
Lines 19574 19656 +82
==========================================
Hits 17249 17249
- Misses 2325 2407 +82 ☔ View full report in Codecov by Sentry. |
Strange to see this error come back from #2944 |
9bf74d6
to
901b68c
Compare
ee19839
to
7e48dfe
Compare
jdaviz/cli.py
Outdated
from qtpy.QtWidgets import QApplication | ||
from qtpy.QtWebEngineWidgets import QWebEngineView | ||
from qtpy import QtCore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if these fail to import, should we throw an error with instructions to install the optional dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put this in qt.py
f4bbdda
to
db025e6
Compare
what do we need to do to get the tests passing, do we just make sure CI is installing qt or will it still fail on headless setups? It doesn't look like there are conflicts, but also might be worth rebasing since this sat for a while (sorry about that!) |
This also restore the --browser feature that was previously broken.
db025e6
to
40c338e
Compare
This mostly looks good, but I noticed one thing in testing: if you call e.g. |
This is now fixed, this is ready for review. |
Description
This allows running jdaviz in its own browser (using qt) for the command line and the standalone version
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.