micropython / webrepl

WebREPL client and related tools for MicroPython
MIT License
633 stars 297 forks source link

Enable re-reading and sending changed files without input dialog #51

Closed jdtsmith closed 2 years ago

jdtsmith commented 4 years ago

Enable re-reading and sending changed files without the need for additional input dialog.

A very common work-flow is to edit a file, upload it, test the changes, make further changes, re-upload etc. Currently you must select the file from the file chooser dialog each time you wish to upload it. This PR makes it possible to simply hit Send to Device and, if the file is changed, have it re-read and sent again, saving considerable time.

Status messages gives feedback on whether the file was re-read and resent ([Changed] attached to file info, or [Unchanged] in status area).

This tested fine for me with Chrome. In Safari, re-reading an already-selected file doesn't work (a file not found error is thrown). If such an error is encountered, the status message is updated to say [Error: Must Choose File Again]. Given that currently Webrepl doesn't indicate that it resends the same outdated file data on subsequent presses of Send to Device, this is an improvement even for Safari users. #

cwalther commented 4 years ago

This is a great idea – this behavior has always annoyed me, but I never bothered to try to fix it because I always assumed that it was due to a deliberate security restriction of web browsers. (And in general I actually do like Safari’s behavior better – just because I give a website permission to read a file once, I don’t necessarily want it to be able to read all future versions of that file too.)

Unfortunately I’m having mixed success: It fails as described on Safari 12. It fails, always saying “[Unchanged]“ even when the file was changed, on Firefox 66 and 71. It works as described on Chromium 69.

Also, some criticism on the implementation (but I don’t speak for the maintainers, so feel free to disregard):

jdtsmith commented 4 years ago

Happy to make changes along these lines. The callback was an attempt to capture hidden state to work around Safari’s unstated security measure. I have a version that does just as you suggest: caches files instead of capturing state in a closure. Obviously indent can be fixed and all changes can be squashed before incorporating. Just seeing what the appetite for this type of change is.

It seems Firefox does not update lastModified? Will have to look into that; maybe there’s a way to force the update.

Regarding resending if not updated, I can see both points of view. With a fresh Webrepl session, a file will always be “changed”. So your failure mode would require deleting or changing files behind the back of a single connected Webrepl session, which seems at least unlikely.

--

On Dec 27, 2019, at 10:28 AM, Christian Walther notifications@github.com wrote:

 This is a great idea – this behavior has always annoyed me, but I never bothered to try to fix it because I always assumed that it was due to a deliberate security restriction of web browsers. (And in general I actually do like Safari’s behavior better – just because I give a website permission to read a file once, I don’t necessarily want it to be able to read all future versions of that file too.)

Unfortunately I’m having mixed success: It fails as described on Safari 12. It fails, always saying “[Unchanged]“ even when the file was changed, on Firefox 66 and 71. It works as described on Chromium 69.

Also, some criticism on the implementation (but I don’t speak for the maintainers, so feel free to disregard):

I don’t understand the decision to only send the file if it has changed. What if I have modified or deleted the file on the device, or have connected to a different device, and want to send it again unchanged? If the purpose is to avoid redundant transfers, then wouldn’t you need to check the file on the device too? Your branch structure seems to be a bit of a mess. As far as I can see, commit 929b832 contains all your changes and the other two are unnecessary. Your indentation is inconsistent. I suspect the introduction of arrow functions will restrict the set of supported browsers, which I would rather not do without obtaining a consensus about it first. I had some trouble understanding this code structure with several callbacks at first, it seems overcomplicated. I haven’t tried to simplify it yet, but why not just put file into a global variable and keep all the functions constant and global rather than passing them around as arguments? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

jdtsmith commented 4 years ago

Made some simplifying fixes, and changed status messages to indicate resending a saved file (or for browser which don't allow resend without re-choosing file) re-require opening the choose dialog again.