webui-dev / nim-webui

Use any web browser as GUI, with Nim in the backend and HTML5 in the frontend.
https://webui.me
MIT License
130 stars 9 forks source link

tooltips change show() #31

Closed siriuslee69 closed 4 months ago

siriuslee69 commented 4 months ago

I changed the tooltips for the Nim bindings, making it clearer that a script and js file should be included in the html for proper window response. As per issue #29. ~~Additionally (as mentioned by another commenter in issue #29), the setTimeout() function controls the general wait times for the backend. I find it important to note that a timeout change will also change the backends response time to the close of the app in case of a browser crash. Specifically, setting setTimeout(0) should keep the backend running indefinitely in case of a browser crash, which could lead to very unsafe behaviour/multiple backend instances upon restart. (If I understood its behaviour correctly) Additonally, providing setTimeout with a number greater than 86400 would also lead to the backend waiting forever (see resp. C code). So I added that to the tooltip as well.~~

neroist commented 4 months ago

Additonally, providing setTimeout with a number greater than 86400 would also lead to the backend waiting forever

Though in the webui_set_timeout function, the time provided is capped to WEBUI_MAX_TIMEOUT.

...
#define WEBUI_MAX_TIMEOUT    (60)    // Maximum startup timeout in seconds the user can set
...

void webui_set_timeout(size_t second) {
    ...
    if (second > WEBUI_MAX_TIMEOUT)
        second = WEBUI_MAX_TIMEOUT;

    _webui_core.startup_timeout = second;
}

That is true for webui_script, however.

AlbertShown commented 4 months ago

We don't need to mention 86400 as it's already too long, no one will put this number, commune timeout use are up to 30 seconds. While mentioning the importance of adding webui.js is good to be added to source code comments, and also in the online documentation.

siriuslee69 commented 4 months ago

Thanks for the clarifications, I reversed some of the changes accordingly.

AlbertShown commented 4 months ago

I suggest:

  ## Please include <script src="webui.js"></script> in the HTML
  ## for proper window communication.
siriuslee69 commented 4 months ago

I think the hint "Essential for creating multiple windows." should stay as part of the tooltip in addition to the one mentioned above. Unless there is a specific reason not to include it. This was also the main reason for my opening of issue #29. While "proper window communication" does surely include the proper opening of multiple windows, it is not as obvious to the beginner programmer (me, lol).

AlbertShown commented 4 months ago

That's okay, but webui.js is essential for everything, single window, multiple windows. The hint Essential for creating multiple windows sounds like we don't need webui.js in a single window.

siriuslee69 commented 4 months ago

That's okay, but webui.js is essential for everything, single window, multiple windows. The hint Essential for creating multiple windows sounds like we don't need webui.js in a single window.

Haha, yes, I get your point. However, a single window will correctly spawn even without webui.js, while multiple Windows won't without it. That makes it a necessity for multi-window sessions and just an "option" for single window sessions. (Whether the single window will work properly is a different matter.)

Edit: I guess what I mean is that webui.js is actually not essential for creating a single window, while it is essential for creating multiple.

AlbertShown commented 4 months ago

a single window will correctly spawn even without webui.js, while multiple Windows won't without it

I got it, now it's clear, so, actually, when you spawn the first window, webui waits for it to connect before spawning the second window, so when webui.js is missing from first window, then the window will never connect, which will make webui waits.

I understand now your point, but still, we should avoid saying single and multiple windows... webui.js is necessary for making communication between UI and back-end.

Let's keep the text in the correct meaning:

  ## Please include <script src="webui.js"></script> in the HTML
  ## for proper window communication.
siriuslee69 commented 4 months ago

I got it, now it's clear, so, actually, when you spawn the first window, webui waits for it to connect before spawning the second window, so when webui.js is missing from first window, then the window will never connect, which will make webui waits.

I think you're right, yes. That would make a lot of sense.

Let's keep the text in the correct meaning:

## Please include <script src="webui.js"></script> in the HTML ## for proper window communication.

I understand now your point, but still, we should avoid saying single and multiple windows...

Why do you think that we should avoid also mentioning multiple windows (in addition to the tooltip described above)? Would it cause confusion or similar? Alternatively I may ask: What would be the benefit of omitting the additonal hint "Essential for creating multiple windows." ?

AlbertShown commented 4 months ago

Because as I said, it's not the normal behavior. webui.js is necessary for communication, while spawning issue can be changed in future, for example a new APIs/settings to make webui does not wait, which will make single and multiple windows spawns all in one shot, even without webui.js.

This is the code for waiting, removing it make multiple windows spawns without needs of webui.js: https://github.com/webui-dev/webui/blob/a994629f90454b78f83916a9911505e0d38d851e/src/webui.c#L5998

siriuslee69 commented 4 months ago

Because as I said, it's not the normal behavior. webui.js is necessary for communication, while spawning issue can be changed in future, for example a new APIs/settings to make webui does not wait, which will make single and multiple windows spawns all in one shot, even without webui.js.

The change you mentioned has not been implemented yet, idk if there is even a pull request for it lol. The hint I added to the tooltip would have correctly stated how the current implementation works. The additional hint would not have been a nuisance, since it was quite short, too. You argue that a future version could fix this issue. Well, since this future version will take approximately several months to ship, I feel like in the meantime, an additional hint wouldn't have been bad. However, I don't feel the need to argue about this anymore and quite frankly, considering this entire conversation was about a tooltip and the fact that your profile is private, I am almost tempted to think you are trolling me. At this point I'll just leave it up to the maintainer and consider this discussion done on my part.