luci-app-tinyproxy: Translate to JS

[Ramon00]

luci-app-tinyproxy: Translate to JS

Signed-off-by: Ramon Van Gorkom [email protected]

[Ramon00]

So I think its fully working, except if you apply the settings then the settings only get saved, not reloaded. I tried with the ucitrack file, but somehow that does not work. Can somebody have a look?
Easy alternative is adding a button to do a manual reload, but that is not so nice...

[systemcrash]

Take a look at what @dannil just did in squid.

[Ramon00]

Take a look at what @dannil just did in squid.

I tried something like that already, it sort of works, but somehow not always, maybe a timing thing?

[Ramon00]

i have to hit the save and apply button twice then it always works

[Ramon00]

Ah seems to work now. If somebody can test the new app that would be great

[Ramon00]

That was a bit premature. It still sometimes does not work. Seems the ui.changes.apply returns before it is actually saved, then the /etc/init.d/tinyproxy restart still reads the old config.

[dannil]

That was a bit premature. It still sometimes does not work. Seems the ui.changes.apply returns before it is actually saved, then the /etc/init.d/tinyproxy restart still reads the old config.

Yeah, it's a timing issue, it doesn't work in my PR as well (I didn't see it at first since squid takes a while to startup since it checks for existing PID during startup which takes about 2 seconds sometimes), I had naively assumed that the apply function returned it's promise so it could be awaited on (when there's actual changes to persist through UCI) but looking at the code it doesn't so we need to think of another solution.

[Ramon00]

Yeah, it's a timing issue, it doesn't work in my PR as well (I didn't see it at first since squid takes a while to startup since it checks for existing PID during startup which takes about 2 seconds sometimes), I had naively assumed that the apply function returned it's promise so it could be awaited on but looking at the code it doesn't so we need to think of another solution.

Just doing uci.apply() works at least here, but that leaves the "unchange changes" in the header until you refresh the page.
So that is not ideal either

[dannil]

Yeah, it's a timing issue, it doesn't work in my PR as well (I didn't see it at first since squid takes a while to startup since it checks for existing PID during startup which takes about 2 seconds sometimes), I had naively assumed that the apply function returned it's promise so it could be awaited on but looking at the code it doesn't so we need to think of another solution.

Just doing uci.apply() works at least here, but that leaves the "unchange changes" in the header until you refresh the page. So that is not ideal either

Yeah, the reason why I used ui.changes.apply() is because that's how handleSaveApply does it if you don't implement a custom handler for it, then you get functionality like checking that persisting actually succeeded and rolling back if it didn't as a bonus which we really shouldn't implement on the app level. IMO not using facilities like that would be a big regression in the conversion.

[Ramon00]

Yeah, the reason why I used ui.changes.apply() is because that's how handleSaveApply does it if you don't implement a custom handler for it, then you get functionality like checking that persisting actually succeeded and rolling back if it didn't as a bonus which we really shouldn't implement on the app level. IMO not using facilities like that would be a big regression in the conversion.

We can call a ui.changes.renderChangeIndicator() afterwards to clear it, but then it is a sort of unchecked apply, which will also apply other changes unchecked. So not a real solution.

Ideally ui.fs should be extended with a check function if the apply is finished.

[Ramon00]

@dannil I may have found a solution:
this.super('handleSaveApply', [ev]) .then(() => fs.exec_direct('/etc/init.d/tinyproxy', ['restart']))

That seems to work, not exactly sure why, maybe due to a file lock from ubus? Or maybe it is just "luck" (i.e. slower). Would be great if we can make sure

[dannil]

@dannil I may have found a solution: this.super('handleSaveApply', [ev]) .then(() => fs.exec_direct('/etc/init.d/tinyproxy', ['restart']))

That seems to work, not exactly sure why, maybe due to a file lock from ubus? Or maybe it is just "luck" (i.e. slower). Would be great if we can make sure

Interesting... I assume tinyproxy doesn't have a procd reload trigger as I wrote about in #7320 (comment)? So I was wrong about it being a timing issue in Squid at least; yes, the boot takes a while but it also reloads itself properly. That would be a way to solve it but would require changes in the principal package, but it's a nice solution since when there's UCI changes, irregardless of it's done via the CLI or LuCI, it's handled the same.

[Ramon00]

I think that was premature again... sigh.
But new idea, reproduce the "unsaved changes" value. Seems to be in uci.changes() https://github.com/openwrt/luci/blob/84e24c08699ab28e077259756cdd5a5b40cfd2a6/modules/luci-base/htdocs/luci-static/resources/ui.js#L4430
going to experiment with that

[Ramon00]

OK test please!

[systemcrash]

Just curious - does reload work better than restart? Sometimes restart is a more substantial operation. And reload completes in a much shorter time.

[Ramon00]

both seem to work equally. Want me to make into reload?

[systemcrash]

Could you try using reload? I think that usage should be encouraged unless there's a "yeah, unplug and restart is necessary". Maybe a separate reload convenience button available also. There are a few examples of those in the apps. @stangri tends to put those in :)

[Ramon00]

Yes reload works I already tested that. I can add a reload and a restart button. No problem. Will do that tomorrow though.

[Ramon00]

How about this:

[Ramon00]

or this maybe:

Not sure how to get rid of the large vertical space in the imported html... I stripped all styles and attributes from the loaded html

[systemcrash]

Those look great.

[systemcrash]

Merged. Thanks @Ramon00 !