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.

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

Fixes #64 #92

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fixes #64 #92

wants to merge 7 commits into from

Conversation

niharsalunke
Copy link

Made some changes to fix bug #64.

Hi @astrojuanlu, I was able to solve the missing earth problem. We need to generate a Cesium ion token before can proceed to display plots. To generate a free token, I followed this link . And after that, I also made a few modifications to the widgets.py file. Everything seems to be working now.

However, with the fullscreen issue, I have worked up to a point where if you perform "cesium_widget_object.fullscreen()" in a python code, you will be able to trigger the fullscreen mode of the canvas.

I was not satisfied here so I tried to write a bit of javascript event listener code in the CESIUM_TPL string of widgets.py and tried to bind the event listeners on the fullscreen button. Now the functions are being triggered correctly. Meaning, if you click on the "fullscreen" button of the CZMLWidget, a JavaScript code responsible to "FULLSCREEN-ize" the canvas gets triggered. But after all this, I got this error: "Failed to execute 'requestFullscreen' on 'Element': API can only be initiated by a user gesture."

This is not easily solvable since it will require external permissions. More here.

In the example file, I have included a basic example demonstrating how a simple widget-plot can be generated and how it can be switched to full-screen mode and back.

Example:
image

Fullscreen mode after executing feature example_widget.request_full_screen():
image

class CZMLWidget:
document = attr.ib(default=Document([Preamble()]))
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove all these properties?

Copy link
Author

Choose a reason for hiding this comment

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

The properties are in place. I have just made the Cesium Widget object creation task flexible.
Each property Change Elaborated below:

  1. Suppose I want to use the Cesium distribution files (CSS and JS scripts) of version 1.72.1, I could specify those while declaring the CZML Widget object If I do not specify, then the default is 1.79.1 (the current latest).
    In the previous approach, we were enforcing a specific version every time (1.68).
  2. ion_token - allowing users to specify an ion_token in case if they want to. Otherwise, creating an exception. ion_token is essential to overcome the 'invisible earth' problem. ion_token is used by the cesium API to fetch cesium assets needed for the plot. (in this case, the earth image). Cesium Ion API is free as of now.
  3. The terrain and imagery parameters give us the flexibility to specify the terrains and imageries of our choice (could be one of the various default terrains or could be one of our custom terrains). Earlier, with any plot, they were set to TERRAIN["Cesium"] and IMAGERY["Bing_Aerial"] by default.
    Based on my observations on the Cesium Ion website, we can upload our custom Tiles, Imageries, and 3D files on the Cesium cloud and then generate our custom plots. A very exciting feature that will unlock tremendous possibilities... I am yet to explore more on this.
  4. _container_id parameter: This is a completely optional parameter. If specified, the CesiumWidget plot will be displayed in a div with id="CesiumContainer-custom_specified_name" otherwise, "CesiumContainer-name_specified_by_uuid4". No specific purpose or advantage in doing this.

To summarize, the properties are all the same. But, instead of using defaults, I have given the user extra flexibility in case if he/she wants to customize the plot.
With the new approach, only the ion_token becomes a compulsory field, which eliminates the 'invisible earth' issue.

Copy link
Member

Choose a reason for hiding this comment

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

Even if "the properties are all the same", the fact that they are not declared in the body will confuse attrs. Please have a look at attrs documentation, especially https://www.attrs.org/en/stable/init.html?highlight=__attrs_post_init__#post-init-hook if you need it.

Also, the quality checks have failed, please do tox -e reformat.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I will have a look at it. It seems I am misunderstanding attrs. I will work on the quality checks too. Thanks a lot.

],
"source": [
"# triggers full-screen of the above widget\n",
"example_widget.request_full_screen()"
Copy link
Member

Choose a reason for hiding this comment

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

The need to call an extra method is very troublesome to me.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed. It's just a temporary fix. I am actively exploring the root cause of this problem and ways to eliminate it. The fullscreen itself is working but the JavaScript that comes with Cesium Assets is not able to detect the right container to expand into to fullscreen.

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Hi @niharsalunke, thanks for this pull request! Unfortunately I am not very happy with the solutions. To begin with, you seemed to remove lots of properties from the class which I don't understand. And secondly, I want to be sure that this JavaScript is the right thing to do. Therefore, I would like you to ask in https://discourse.jupyter.org/ and https://discourse.jupyter.org/ if this is the right approach, to get some input.

@niharsalunke
Copy link
Author

Hi @astrojuanlu , I have removed the JS code which was being bound at runtime. It wasn't helping and I believe that it's not the standard practice. I went through the complete documentation of attrs as you had suggested and found it to be very helpful. Now, I have implemented the earlier procedure but with the help of attrs.

However, I am struggling to pass any test beyond refactor and quality. It is because I am unable to import the 'IPython' module while running the test. The test always throws an error saying "IPython not found". I had mentioned it in the tox.ini and mypy.ini files but that won't help either.

Could you please help me here?
Many Thanks.

_container_id = attr.ib(factory=uuid4)

@ion_token.validator
def _check_ion_token(self, attribute, value):
if value == "no_token" or value == "" or value is None:
warnings.warn("No ion_token Provided. Some features may not work. Please get your free token at https://cesium.com/ion/tokens", FutureWarning)
warnings.warn(
Copy link
Author

Choose a reason for hiding this comment

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

Instead of completely halting the CZML Widget, I have just raised warnings if he does not specify ion_token. The CZML Widget displays just fine but the earth remains invisible.

@@ -115,10 +122,12 @@ def _repr_html_(self):
return self.to_html()

def request_full_screen(self):
import IPython
return IPython.core.display.HTML(f'''

Copy link
Author

Choose a reason for hiding this comment

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

The temporary fix for fullscreen.

@astrojuanlu
Copy link
Member

Hi @astrojuanlu, I was able to solve the missing earth problem. We need to generate a Cesium ion token before can proceed to display plots. To generate a free token, I followed this link .

Just a clarification: it's not mandatory to have an Ion token to display the Earth. The user either needs to:

  • Specify the most up to date Cesium.js version
  • Use a different tile source

I don't think we should change any of that. If anything, we should document it (feel free to open an issue about it)

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Sorry but I don't like this temporary solution. I know it might be a challenging problem, but I think adding a method just for this is not the right thing to do.

I have no idea about front-end, so I am really not in a position to give a clear path. But I think we should make the fullscreen button just work.

If the button works on Cesium Sandcastle but it doesn't work on Jupyter notebook, then either:

  • We are not embedding Cesium correctly (which is very likely - we should be making an extension instead of spitting a blob of HTML, see Support JupyterLab #61)
  • Jupyter notebook has some special layout or behavior that blocks this event
  • ?

We should build a minimum reproducible example and ask in https://community.cesium.com/ about this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants