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

fix: minor painterro integration fixes #338

Merged
merged 2 commits into from
Aug 30, 2022

Conversation

nderscore
Copy link
Contributor

@nderscore nderscore commented Aug 30, 2022

  • Prevent multiple script tags from being loaded if the Advanced Editor button is pressed rapidly before painterro has been loaded
  • Prevent styles from being applied more than once
  • Handle painterro script loading errors more gracefully
    • Remove the script tag after an error occurs to allow re-attempts
    • Log the error event instead of false
    • (maybe painterro should be served by the webui directly instead to avoid this scenario and improve the offline experience?)

@codedealer
Copy link
Collaborator

Removing an external call to CDN is certainly something I pondered before. Do you have an idea how to serve it locally lazily? I see no benefit just bundling painterro.js along with index.js on load.

@nderscore
Copy link
Contributor Author

Do you have an idea how to serve it locally lazily?

Not sure how feasible it is, but maybe there's a way to serve these static assets (CSS and JS, including libraries, or maybe even images or fonts in the future) from a folder, more like a webserver?

@codedealer
Copy link
Collaborator

Not to my knowledge. Serving of static assets is completely blackboxed by Gradio.

The only way I see to do this without bundling it is getting it separately on _js hook of clicking "Advanced Editor" button.

A method similar to this that will read it from a separate file painterro.js and dump it as a string for eval, followed immediately by the call to SD.Painterro.init()

@hlky hlky changed the base branch from master to pr_merge_1 August 30, 2022 21:52
@hlky hlky merged commit 0ddce37 into Sygil-Dev:pr_merge_1 Aug 30, 2022
@hlky
Copy link
Contributor

hlky commented Aug 30, 2022

These changes are okay I think, everything still works at least.

@altryne this is something we could ask gradio to change

Not to my knowledge. Serving of static assets is completely blackboxed by Gradio.

The only way I see to do this without bundling it is getting it separately on _js hook of clicking "Advanced Editor" button.

A method similar to this that will read it from a separate file painterro.js and dump it as a string for eval, followed immediately by the call to SD.Painterro.init()

@codedealer
Copy link
Collaborator

Yep, they allow to pass css as a file into Blocks constructor so it should not be out of the question to just allow serve us arbitrary css/js files. Even if they put a limit on one .css and one .js, the files can be bundled during the preprocess and then served as static content like from any other server.

Extendable gradio components is another major want. The ability to change the functionality of something like mask editor would go a long way.

@altryne
Copy link
Contributor

altryne commented Aug 30, 2022

@codedealer please consdier adding those requests in Gradio repo!
I've had an amazing chat with them today and they are really open to feedback.
Loading a JS file/library was one of my top wants!
https://github.com/gradio-app/gradio/issues

@codedealer
Copy link
Collaborator

@altryne I made a general feature request gradio-app/gradio#2137. Feel free to pitch in if you have something to expand upon it.

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.

4 participants