feat: add option to display an app in a new browser window #977

mariobuikhuizen · 2021-11-30T13:54:17Z

Description

This pull request adds button to the app which will open it in a new browser window:

popout.mp4

Popout functionality is implemented in https://github.com/mariobuikhuizen/ipypopout

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.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a change log needed? If yes, is it added to CHANGES.rst?
Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

kecnry · 2021-11-30T14:43:38Z

This looks awesome! But it doesn't seem to work for me... clicking on the button shows a new window being created but is immediately destroyed (using chrome, in both notebook and lab).

havok2063 · 2021-11-30T14:48:07Z

This is something that could potentially be useful on the MAST side of things, depending on its requirements. We should check that it works. Otherwise we will need a way to disable this button for MAST, ideally via the config file.

mariobuikhuizen · 2021-11-30T14:53:54Z

@kecnry I forgot chrome blocks popups by default. This can be changed here: chrome://settings/content/popups

mariobuikhuizen · 2021-11-30T14:57:21Z

Also, using localhost instead of 127.0.0.1 seems to allow popups by default

rosteen · 2021-11-30T14:57:27Z

Firefox seems to open and then instantly close the new window. I have popups allowed for the page.

kecnry · 2021-11-30T14:59:22Z

It doesn't seem to be blocking the popup, but rather immediately closing/crashing it (as compared to other cases where popups are blocked and never even created).

kecnry · 2021-11-30T15:01:15Z

@havok2063 - I can run those tests with your standalone repo before we merge this and if not make sure you have a hook to disable the button. There are also some styling issues, especially in imviz (which has a taller toolbar and isn't vertically centering the button).

rosteen · 2021-11-30T15:03:20Z

It doesn't seem to be blocking the popup, but rather immediately closing/crashing it (as compared to other cases where popups are blocked and never even created).

Exactly what I'm seeing.

mariobuikhuizen · 2021-11-30T15:04:29Z

Can you try right clicking? This should open a tab instead of a window, maybe this gives some more info.

rosteen · 2021-11-30T15:07:37Z

That results in behavior I've never seen before in a browser - the tab opens and then immediately crashes/closes.

kecnry · 2021-11-30T15:08:15Z

@mariobuikhuizen - similar behavior. It very briefly seems to start opening a new tab (just start the see the x button of a new tab expanding, blue background of an empty tab, and immediately disappears)

kecnry · 2021-11-30T15:10:20Z

Ah, seems to be my adblocker's fault (I use ublock origin) - disabling that and it works. @rosteen - can you confirm on your end?

pllim · 2021-11-30T15:10:41Z

Did you try to turn your antivirus off? 😅 (Oh, nvm, didn't see Kyle's post above.)

mariobuikhuizen · 2021-11-30T15:12:25Z

Well, at least that's consistent with opening a window 😅

I can't reproduce it. I'm setting up a fresh environment to see if that helps.

rosteen · 2021-11-30T15:13:40Z

Ah, seems to be my adblocker's fault (I use ublock origin) - disabling that and it works. @rosteen - can you confirm on your end?

Ahhh good call, uBlock Origin was killing it. After turning off my extensions it works. Looks awesome, I'll have to do a little more testing with it.

mariobuikhuizen · 2021-11-30T15:14:44Z

Ok, great!

pllim · 2021-11-30T15:15:25Z

Probably should add a warning on user facing doc about this. Maybe under known issues?

Also please add change log.

kecnry · 2021-11-30T15:20:05Z

I'm not sure if there is any way around that or to detect when it happens and raise a message? Or if we should just handle it in the docs/tooltip text?

It looks like this also breaks all tests as ipypopout is looking for a non-existing Kernel.

I'll do more testing including embedded in MAST in the meantime!

mariobuikhuizen · 2021-11-30T15:28:35Z

For MAST we might have to add the right base-url to the body tag.

rosteen · 2021-11-30T15:37:19Z

@kecnry I had the same issue here that I showed you a week or two ago - in the popup window, if I open the model fitting plugin (which has a lot of content), the viewer/app resizes to the entire length of the plugin, rather than restricting the plugin height and adding a scrollbar. I think I showed you this in Voila, can't remember if you have a fix open for it already.

kecnry · 2021-11-30T16:45:01Z

@rosteen - I can reproduce that here (although it wasn't introduced here, I'm guessing it just occurs whenever the wrapping div doesn't set a fixed height as Jupyter does) and created #979.

havok2063 · 2021-12-01T03:55:52Z

Even if it works in the standalone repo, we may run into other issues when it's deployed in a MAST domain. It might be a good idea to include with this PR the option for hiding the button. Another question I have is how is this mode different than running Jdaviz in desktop app mode?

pllim · 2022-06-01T19:51:03Z

This is from Notebook. The text is supposed to be white and it is white in the original notebook, but black in the popout.

pllim · 2022-06-01T19:55:31Z

The viewer stretches weirdly under some circumstances. Try this workflow:

Run Cubeviz example notebook and load the data.
Pop it out.
In the pop out, open up Metadata Viewer.
See the image viewers all stretches out very long vertically until Metadata Viewer is closed. This does not happen in the original notebook.

jdaviz/components/tooltip.vue

rosteen · 2022-06-01T20:00:29Z

I also saw the viewers stretching to match the length of the plugin tray, rather than the plugin tray being limited to the height of the app.

- Button is hidden when data-base-url is not set - Enable button in MAST by setting: <body data-base-url="/" data-voila-host="http://mydomain:8888"

- fix: button is disappearing after clicking it - feat: expose windowName and windowFeatures to python

Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>

The doctype in ipypopout's HTML file was missing, putting the page in quirks-mode, which causes tables not to inherit the text color (and can cause many more rendering differences). This was fixed in v0.0.7

pllim · 2022-06-02T16:19:12Z

I installed this branch on Windows 10 (editable install). The standalone app does not load properly (i.e., localhost:<port> page popped up, I see "running cell 1" for a sec, then everything is blank).

On the terminal, I see this:

WARNING:tornado.general:403 GET /voila/static/voila.js.map (::1): voila.js.map is not in root static directory

On the browser dev console:

For comparison, when I run the same CLI from current main, there is no warning on the terminal and everything loaded fine. And this is what I see on the browser dev console:

maartenbreddels · 2022-06-07T14:45:32Z

WARNING:tornado.general:403 GET /voila/static/voila.js.map (::1): voila.js.map is not in root static directory

FYI: this is the browser looking for source mapping (if you care about debugging js), it can be ignored.

pllim

The error in Windows is fixed but the "standalone app" mode still gives me weird stuff, like this extra "add" text in subset selection tool. I do not see it in notebook mode nor in main.

mariobuikhuizen · 2022-06-27T12:45:38Z

@pllim I can't reproduce this issue in the standalone app on mac or windows.

pllim · 2022-06-27T14:48:08Z

I will try again. It was on editable install, maybe I'll try a non-editable and see. 🤷

pllim

I updated my dependencies on Windows and the weird wording is gone in both editable and non-editable installs. Works fine in "standalone" and "notebook" modes for me now.

The box zoom is a little weird but I don't think it is related as I remember @kecnry mentioned it separately before.

Since my approval is the second one, I will merge. Thanks!

mariobuikhuizen requested review from duytnguyendtn, ibusko, javerbukh, kecnry, ojustino, pllim and rosteen as code owners November 30, 2021 13:54

github-actions bot added the embed Regarding issues with front-end embedding label Nov 30, 2021

mariobuikhuizen marked this pull request as ready for review June 1, 2022 19:16

pllim removed the Upstream fix required label Jun 1, 2022

This comment was marked as outdated.

Sign in to view

rosteen reviewed Jun 1, 2022

View reviewed changes

jdaviz/components/tooltip.vue Outdated Show resolved Hide resolved

mariobuikhuizen and others added 10 commits June 2, 2022 15:26

feat: add option to display an app in a new browser window

c5c543a

fix: use ipypopout v0.0.2 to support MAST

a7c8224

- Button is hidden when data-base-url is not set - Enable button in MAST by setting: <body data-base-url="/" data-voila-host="http://mydomain:8888"

fix: errors in test because ipython is not available

4cc3f2f

docs: add popout feature to the change log

8038136

feat: add message to tooltip about reasons popout might not work

22e3815

chore: update to ipypopout v0.0.4

0ed4620

- fix: button is disappearing after clicking it - feat: expose windowName and windowFeatures to python

feat: use the echo messages backport in ipywidgets 7.7 in ipypopout

45c7b90

fix: stretched out interface when opening long content in sidebar

8f15a38

fix: typo

b85b0dc

Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>

fix: hover text in the header is black instead of white

58078dc

The doctype in ipypopout's HTML file was missing, putting the page in quirks-mode, which causes tables not to inherit the text color (and can cause many more rendering differences). This was fixed in v0.0.7

mariobuikhuizen force-pushed the feat_popout branch from ddd07ed to 58078dc Compare June 2, 2022 13:28

fix: acquiring kernel id on windows fails

de7a4cd

pllim reviewed Jun 13, 2022

View reviewed changes

pllim approved these changes Jun 27, 2022

View reviewed changes

pllim merged commit bb76ac6 into spacetelescope:main Jun 27, 2022

Jdaviz-Triage-Bot mentioned this pull request Jun 27, 2022

New popout button eats into coordinate info bar #1422

Open

kecnry mentioned this pull request Jun 28, 2022

popout plugins #1423

Merged

9 tasks

kecnry mentioned this pull request Jul 6, 2022

imviz tabs: hide popout button #1461

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add option to display an app in a new browser window #977

feat: add option to display an app in a new browser window #977

mariobuikhuizen commented Nov 30, 2021 •

edited by pllim

Loading

kecnry commented Nov 30, 2021

havok2063 commented Nov 30, 2021

mariobuikhuizen commented Nov 30, 2021

mariobuikhuizen commented Nov 30, 2021

rosteen commented Nov 30, 2021

kecnry commented Nov 30, 2021

kecnry commented Nov 30, 2021

rosteen commented Nov 30, 2021

mariobuikhuizen commented Nov 30, 2021

rosteen commented Nov 30, 2021

kecnry commented Nov 30, 2021

kecnry commented Nov 30, 2021 •

edited

Loading

pllim commented Nov 30, 2021 •

edited

Loading

mariobuikhuizen commented Nov 30, 2021

rosteen commented Nov 30, 2021

mariobuikhuizen commented Nov 30, 2021

pllim commented Nov 30, 2021

kecnry commented Nov 30, 2021

mariobuikhuizen commented Nov 30, 2021

rosteen commented Nov 30, 2021

kecnry commented Nov 30, 2021

havok2063 commented Dec 1, 2021 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

pllim commented Jun 1, 2022

pllim commented Jun 1, 2022

rosteen commented Jun 1, 2022

pllim commented Jun 2, 2022

maartenbreddels commented Jun 7, 2022

pllim left a comment

mariobuikhuizen commented Jun 27, 2022

pllim commented Jun 27, 2022

pllim left a comment

feat: add option to display an app in a new browser window #977

feat: add option to display an app in a new browser window #977

Conversation

mariobuikhuizen commented Nov 30, 2021 • edited by pllim Loading

Description

Checklist for package maintainer(s)

kecnry commented Nov 30, 2021

havok2063 commented Nov 30, 2021

mariobuikhuizen commented Nov 30, 2021

mariobuikhuizen commented Nov 30, 2021

rosteen commented Nov 30, 2021

kecnry commented Nov 30, 2021

kecnry commented Nov 30, 2021

rosteen commented Nov 30, 2021

mariobuikhuizen commented Nov 30, 2021

rosteen commented Nov 30, 2021

kecnry commented Nov 30, 2021

kecnry commented Nov 30, 2021 • edited Loading

pllim commented Nov 30, 2021 • edited Loading

mariobuikhuizen commented Nov 30, 2021

rosteen commented Nov 30, 2021

mariobuikhuizen commented Nov 30, 2021

pllim commented Nov 30, 2021

kecnry commented Nov 30, 2021

mariobuikhuizen commented Nov 30, 2021

rosteen commented Nov 30, 2021

kecnry commented Nov 30, 2021

havok2063 commented Dec 1, 2021 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

pllim commented Jun 1, 2022

pllim commented Jun 1, 2022

rosteen commented Jun 1, 2022

pllim commented Jun 2, 2022

maartenbreddels commented Jun 7, 2022

pllim left a comment

Choose a reason for hiding this comment

mariobuikhuizen commented Jun 27, 2022

pllim commented Jun 27, 2022

pllim left a comment

Choose a reason for hiding this comment

mariobuikhuizen commented Nov 30, 2021 •

edited by pllim

Loading

kecnry commented Nov 30, 2021 •

edited

Loading

pllim commented Nov 30, 2021 •

edited

Loading

havok2063 commented Dec 1, 2021 •

edited

Loading