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

Add functions to handle new code cells in thebe. #725

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

Conversation

xr4dsh
Copy link

@xr4dsh xr4dsh commented Jan 1, 2024

Add 2 functions that allow adding and removing of code cells in thebe without reconnecting every time.
I added a new example to the simple apps to showcase it.

Currently new code cells are not detected and each time the page changes the bootstrap function needs to be executed to detect new code cells. This add 2 new function to either detect new code cells and add them to the list or replace the old list with the new code cells.
Copy link

welcome bot commented Jan 1, 2024

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

@stevejpurves
Copy link
Member

stevejpurves commented Jan 4, 2024

Hi @xr4dsh and thank you for the PR!

I've run this locally and the demo works well 👏

Looking at the implementation, I see you're recreating all cells each time through a "shadow" notebook which is then discarded, while that works it requires a lot of setup and as more and more cells are added of course means more of the domis being replaced each time.

Did you consider a more targeted addCell(s), removeCell(s) approach that just adds and renders the cell that is needed?

e.g. you easily add a cell to the existing notebook by just creating a new cell (like in

const cell = new ThebeCodeCell(c.id, id, c.source, config, metadata, notebook.rendermime);
) and then assigning the session to it, as you are now.

The removeCell could by done by just removing from the notebook.cells array and removing the DOM elements, which feels like that could be done in the removeCell function.

We could also consider extending the ThebeNotebook interface to include an addCell(...) and removeCell(id: string) helper functions in thebe-core if this more targeted approach makes sense for you

@xr4dsh
Copy link
Author

xr4dsh commented Jan 5, 2024

Thank you for the answer.

The Problem that i am trying to solve is that the web page changes constantly and currently i just rerun all cells when the web page changed, what i really want is that the code cell are re-detected and only the newly added once are run, while the old cell just get their output displayed again, but the ids change when i update the web page, at the moment i just give each code cell an unique html dom id in the backend and when they updated just send back the result with that html id.

Probably thebe is not the best option for me, it would be enough if i could just execute the code in the backend, but I also want working ipywidgets, and this does not seem possible with other approaches.

I have to agree with you that a more dedicated addCell(id) removeCell(id) redetectCell(HtmlDomId) syntax would be much better for this.

@stevejpurves
Copy link
Member

The Problem that i am trying to solve is that the web page changes constantly...
in the sample you included there are add and remove cell buttons, is that the functionality you are trying to implmement or are you doing something beyond that e.g. that some other javascript / process might update the page without a page refresh?

@xr4dsh
Copy link
Author

xr4dsh commented Jan 5, 2024

Yes, i only import it and the website runs a lot of JavaScript. I am implementing an extension https://github.com/xr4dsh/CodeRunner for https://github.com/oobabooga/text-generation-webui and the biggest problem is that the webpage updates constantly without that my script can notice it, currently i just have a MutationObserver and rerun everything when the site updates.

The example is only for this repo to test if my implementation works at all.

@stevejpurves
Copy link
Member

Hi @xr4dsh I'm still hesitant to bring these changes into thebe as the way they are implemented are not ideal.

It seemed like the problem you were working around is more to do with the page lifecycle that you are dealing with and it might be more appropraite for you to use thebe-core and thebe-react directly, and manage the page integration yourself at a lower level?

It's been a while since your last comment, maybe you've made progress on this independently?

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.

None yet

2 participants