Redump: clear form before parsing?

JohnVeness commented 2 years ago

A little quality of life suggestion:

Just before parsing the JSON, can you clear anything that is already in the form?

At the moment, if there is something in, for example, the disc number/letter field but the new JSON has that blank, when you press Parse, the disc number/letter field will still be populated and you might not notice when you submit.

These are examples of use cases:

You open a JSON and parse it, before realising it was the wrong JSON. So you open the correct JSON and parse that. However, you don't notice that the first JSON had, for example, a disc number/letter that the second JSON did not have.
You open a JSON and parse it and submit it. You then scroll to the top and load another JSON to parse and submit, and again, similar potential problem as above.

In the second case, there is an "Add another disc" link, which calls javascript:resetForm('newdisc'), but the user might be lazy!

In general, I think the user would expect parsing a JSON to override everything currently in the form, including "leftovers".

I wondered about just adding a resetForm('newdisc') into function parse(event), but I notice that the big JSON text box is actually part of the newdisc form and so gets blanked!

nightson commented 2 years ago

I think the better way is to call the resetForm function when you use the file picker to select the JSON file. Easy fix and you don't have to worry about the JSON textarea getting reset since you're already selecting a new JSON. What do you think?

JohnVeness commented 2 years ago

That is a great idea and I'm shocked I didn't think of it! I was thinking you would have to put the json textarea in a separate form. But then I'm still thinking of early 1990s web design where one form can only have one submit button!

nightson commented 2 years ago

https://github.com/nightson/UserScripts/commit/6792515b9e17a9949b09d6060f0dc1e047612832

JohnVeness commented 2 years ago

Hmm, I've just tested this, and it doesn't seem to work right! I press Browse, pick a file, but after picking it, the form is empty (nothing in the large textarea, and all the fields below as default).

I tried moving the resetForm() call to the top of readJSON(), i.e.:

  async function readJSON(event) {
    resetForm('newdisc'); //Clear all fields of new disc form
    let file = await fileToJSON(event.target.files[0]);
    function filter(key, value){
      if (key == 'artifacts') {
        return undefined;
      } else {
        return value;
      }
    }
    document.getElementById('d_subinfo').value = JSON.stringify(file, filter, 2).replace(/\[T:ISBN\] \(OPTIONAL\)|\(REQUIRED\)|\(OPTIONAL\)|\(REQUIRED, IF EXISTS\)|\(CHECK WITH PROTECTIONID\)/g, '');//Remove Placeholders
  }

but that behaves exactly the same :(

Also, thinking about this, I'm not sure that resetting the form when loading the JSON file is correct after all. What if a user pasted text directly into the large textarea, rather than browsed for a JSON file, then clicked Parse, while there were still leftovers in the newdisc form below?

Maybe we should revisit my initial suggestion of resetting the newdisc form when clicking the Parse button (rather than at the Browse stage), if you can somehow make sure the JSON textarea doesn't get wiped?

Also, having seen the bit where you remove placeholders when loading a JSON from a file, again that makes me think what if the user pastes into the textarea instead of loading a file - the placeholders won't be removed then. So maybe, it would be best to remove the placeholders as part of the Parse process rather than the Browse one, for consistency?

JohnVeness commented 2 years ago

Sorry for the braindump, but I've thought of one more thing: maybe the JSON textarea could be made readonly, i.e. so that the only way to input JSON is by browsing for a file, and not allow the user to paste in text or edit the text before parsing. That would eliminate some of my worries in the above comment! In fact, you could argue that the user doesn't actually need to be able to see the JSON text at all, so maybe you just need a Browse button and a Parse button.

Sorry if I'm spamming too many ideas at you at once!

I notice that http://wiki.redump.org/index.php?title=Disc_Dumping_Guide_(MPF) says to copy and paste into the large textarea, so that would have to be looked at too, if there was a change.

nightson commented 2 years ago

Weird. It's working all right here.

I didn't think of the user case where one might paste JSON text directly into the textarea. This script is not supposed to work like that. But I guess it's useful sometimes.

nightson commented 2 years ago

Also, having seen the bit where you remove placeholders when loading a JSON from a file, again that makes me think what if the user pastes into the textarea instead of loading a file - the placeholders won't be removed then. So maybe, it would be best to remove the placeholders as part of the Parse process rather than the Browse one, for consistency?

Yeah you have a good point here. This is definitely something I should do. Should be an easy fix, too.

JohnVeness commented 2 years ago

As per your comment in #6 (I'm sorry we keep jumping around issues!), I looked in the browser console. I see the following after I browse for a file: ReferenceError: resetForm is not defined[sandbox eval code:157:5] (where clicking on the [sandbox eval code:157:5] bit shows me line 157 in the RedumpReimagined script).

JohnVeness commented 2 years ago

I'm way out of my depth here, but I see that resetForm() is defined in redump.org/javascript/form.js. I don't know why my browser would be stopping the userscript from finding the code in there though. I'm using up-to-date Firefox and GreaseMonkey.

Maybe as a workaround you could just take the code from resetForm() and put it in your script? It doesn't seem long.

nightson commented 2 years ago

resetForm

Hmm. I was able to reproduce the bug with Greasemonkey. It seems that Greasemonkey handles // @grant none differently than ViolentMonkey or there is a bug in the current version. With // @grant none you should be able to access to page functions like resetForm() but I was only able to access it with unsafeWindow.resetForm(). unsafeWindow.resetForm() works in ViolentMonkey as well but I guess I should just take the code from resetForm() like you suggested.

nightson commented 2 years ago

Hmm it seems that taking the code from resetForm() directly doesn't work in GreaseMonkey either. I got a similar error TypeError: form.submit.removeAttribute is not a function.

I found some info here but I won't pretend that my little non-techy brain understands any of it.

It seems that unsafeWindow.resetForm() is the way to go since it works in both script managers.

nightson commented 2 years ago

I believe it's a bug of Greasemonkey since resetForm() works in TamperMonkey as well. I think I will go with unsafeWindow.resetForm() as it appears to work with all 3 script managers.

nightson commented 2 years ago

Just updated the script once again. It should be working now.

https://github.com/nightson/UserScripts/commit/4116db44c5445ab3e38b88194d9c695acfaab1f7

JohnVeness commented 2 years ago

Lovely, thank you for all your work on this. It seems to work as expected now :heart:

nightson / UserScripts

Redump: clear form before parsing? #5