-
Notifications
You must be signed in to change notification settings - Fork 74
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 :( |
This also restore the --browser feature that was previously broken.
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.45% 88.36% -0.10%
==========================================
Files 124 124
Lines 18389 18409 +20
==========================================
+ Hits 16266 16267 +1
- Misses 2123 2142 +19 ☔ View full report in Codecov by Sentry. |
Strange to see this error come back from #2944 |
9bf74d6
to
901b68c
Compare
901b68c
to
ee19839
Compare
ee19839
to
7e48dfe
Compare
if "--browser" not in args: | ||
args.append("--browser") | ||
args.append("qt") |
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.
since qt is an optional dependency (and we want to keep it that way), can this default to qt only if it is installed and otherwise use the system browser?
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.
This is only for the standalone, which is built in CI with qt installed. Or do you mean if people build in locally?
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 anyone installs jdaviz (from pip for example) but not using the standalone installer, they still have access to the CLI, but may not have the qt 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.
But they would not use this file right, maybe you are confusing it with cli.py?
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!) |
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.