trzsz / trzsz.js

trzsz.js is the js version of trzsz, makes terminal built with electron and webshell to support trzsz ( trz / tsz ).
https://trzsz.github.io/js
MIT License
189 stars 12 forks source link

Make timeout configurable #13

Closed jkroepke closed 11 months ago

jkroepke commented 11 months ago

Hi,

it would be nice, if the timeout could be configured

https://github.com/trzsz/trzsz.js/blob/0c55bdc47c22afbde71adaf9cdbdd0870e0715b5/src/filter.ts#L243

lonnywong commented 11 months ago

Or just increase it a little bit? What value do you want to set it to?

jkroepke commented 11 months ago

Due local limitations, calling the trzsz binary may takes some time to execute.

Currently, If I drag n drop a file into terminal, the timeout can exceeded. If this happens, the trz binary is executed anyways and trzsz.js opens the file browser instead uploading the file.

In the same environment, if trz is executed fast enough (within 1 second), everything works fine.

While 1000ms is a good default, our enterprise environment may needs more.

Is it possible to use a global scope variable from JS point of view? It would be great if the timeout is adjustable without rebuilding trzsz.js. At the end, I'm trying to integrate ttyd which already embedded trzsz.js

lonnywong commented 11 months ago

I'm ok to add an option in options.ts.

Should we change the default value to 2000, or 3000 ?

It shouldn't be multiple uploads within 3 seconds.

jkroepke commented 11 months ago

Does an option in options.ts requires an rebuild of trzsz?

lonnywong commented 11 months ago

Any change requires rebuilding.

jkroepke commented 11 months ago

An using something like window['__trzsz_timeout'] is an option?

Thinking something like xterm.js which expose some options without rebuilding the software.

lonnywong commented 11 months ago

Add window['__trzsz_timeout'], or change the any other source code, we need to publish a new version of trzsz.js.

jkroepke commented 11 months ago

Is that a problem?

lonnywong commented 11 months ago

Since we need to publish trzsz.js anyway. Isn't adding an option in options.ts better then window['__trzsz_timeout']?

jkroepke commented 11 months ago

If the maintainers of https://github.com/tsl0922/ttyd doesn't agree with an higher default timeout, I may have to rebuild ttyd entirely from scratch which would be a big too much for my scope.

I agree, that using the window variable is silly in context of mondern web application. But what about a interface for runtime parameters instead build-time parameters?

Something like trzsz.setOption('timeout', 4000)

lonnywong commented 11 months ago

Is it more convenient to set it in the console ( every time ) than to build ttyd yourself ( once )?

If I set the default timeout to 3000, Is the API no longer needed?

jkroepke commented 11 months ago

Is it more convenient to set it in the console ( every time ) than to build ttyd yourself ( once )?

From downstream end-user perspective yes. It may not need every time, but I guess the trzsz library will constructed somewhere in the code and the option is define once.

Rebuild the entire software stack to change single properties feels too much.

For xterm.js. If the option about the background color would be a build time, no one would really benefit from it.

If I set the default timeout to 3000, Is the API no longer needed?

Maybe, but I guess it would relax the problem on my side without known the side-effects in general here.

lonnywong commented 11 months ago

In fact, there are no side effects. I just felt that 1 second was enough when writing it.

I think run trz -r command is much convenient then change the timeout settings in console.

jkroepke commented 11 months ago

I think run trz -r command is much convenient then change the timeout settings in console.

Not sure, if that helps. From my limited view, in this case it's more a client side timeout (trzsz.js) than the timeout on the console.

The time between request for upload (client) and accepting (on console) can take more then 1 seconds.

lonnywong commented 11 months ago

Oh, I mean the F12 console in the browser. Not the server console.

lonnywong commented 11 months ago

How do you set the window['__trzsz_timeout'] on client? Do you need to press F12 to open the console in the browser? I don't think it's more convenience then type trz command, and choose files from dialog. which isn't drag and drop files to upload, and there is no timeout for trz command.

jkroepke commented 11 months ago

How do you set the window['__trzsz_timeout'] on client?

I will never do this.

ttys has a concept of client-options. It's a json file that get downloaded on initial session and the client javascript of ttyd should define this on session start, get getting the value from json and set the value on the given library. That how ttyd is able the define options for xterm.js. I hope to integrate a similar way for trzsz.

lonnywong commented 11 months ago

I see. Which option is defined by xterm but not by ttyd?

lonnywong commented 11 months ago

https://github.com/tsl0922/ttyd/blob/bf06c1449eae6e0d8da4dff12d7a91e4adc68fcb/html/src/components/terminal/xterm/index.ts#L384-L392

ttyd passes options to xterm. we can't do that without ttyd's help.

jkroepke commented 11 months ago

we can't do that without ttyd's help.

Sure, I expect this. Thats why I'm asking for an option lnterface like trzsz.setOption('timeout', 4000) and after release of the new version of trzsz.js I have create an issue on ttyd to update and integrate the new capabilities.

jkroepke commented 11 months ago

I if got it correctly. the client-option has to integrate here

https://github.com/tsl0922/ttyd/blob/bf06c1449eae6e0d8da4dff12d7a91e4adc68fcb/html/src/components/terminal/xterm/addons/zmodem.ts#L67C32-L78

Maybe as option/parameter for TrzszFilter

lonnywong commented 11 months ago

The argument of new TrzszFilter is options.ts.

https://github.com/trzsz/trzsz.js/issues/13#issuecomment-1782576758

jkroepke commented 11 months ago

I guess we have an miscommunication here. Sorry.

The context of the questions

Does an option in options.ts requires an rebuild of trzsz?

had the background: After build and release trzsz, would be the timeout value adjustable without rebuilding trzsz.js?

The answer seems like "no" now.

lonnywong commented 11 months ago

We have to build and release trzsz.js first ( and only one time ), then the timeout is adjustable.

jkroepke commented 11 months ago

Thanks for the clarification! Add the timeout option to options.ts looks good to me, and after a release I will create an issue at ttyd.

lonnywong commented 11 months ago

I may name the timeout option to dragInitTimeout. And the ttyd may name it trzszDragInitTimeout.

It might be two weeks before I release the next version of trzsz.js.

lonnywong commented 11 months ago

trzsz.js v1.1.4 released.