Add Output Language Handling to Sequence Editor

[duranb]

Summary

This PR improves the sequence editor's handling of input vs. output files and adds missing TypeScript type declarations for reactive variables in Svelte components.

Changes

1. Refactor Output Format Controls (816c2ec)

• Moved output buttons from top editor down into the output preview panel

This refactor was spurned from the discovery of the bug that the output copy/download dropdown button currently ignores the language selected in terms of the content. What is happening is, rather than running the input sequence through the adaptation's converter function, we're actually using the content of the output preview editor. This means that the content being copied/downloaded is not controlled by the dropdown button, but rather it is indirectly controlled by the dropdown of the output preview panel. This was never noticed until now because, so far, all the use cases for adaptations involve a single output language. This refactor is to both improve UX for copying/downloading output sequences and to fix the aforementioned bug.

2. Conditional Output Panel Display (91e6a5d)

• Output panel now only displays when editing files that match the adaptation's input file extension within SequenceEditor
• Prevents showing irrelevant output formats when viewing already-converted output files

3. Initial Differentiation between Input and Output Files (2115e8c)

• Output files: Now finds and applies the matching adaptation output format's extensions for linting and highlighting

Testing

1. Upload an adaptation with custom linting in the output language (you can add a temporary console log to the seqJsonLinter() in the test sequence-adaption.js file)
2. Create a workspace that uses the adaptation file uploaded in step 1
3. Create a valid input sequence file
4. Verify that output copy/download buttons are now present in the output panel header
5. Download the output file
6. Upload the output file into the workspace
7. Select the new output file
8. Verify that the output panel is not present
9. Modify the output file
10. Verify that you are seeing the relevant linting console logs added in step 1

[AaronPlave]

Just a few small comments but otherwise tested and didn't see any issues.

[dandelany]

Can we add some more context to the description of this? Specifically, there is not a very clear distinction between what parts are refactoring vs. what user-facing behavior actually changed. For the parts where behavior changed I want to know - was this a bug? If so, what was the specific buggy behavior that we saw and the expected behavior in that case? A lot of what is described in parts 2 and 3 above sound like what I expected the behavior to be all along, so what was it doing if not that? Behavior changes should be the "headline" rather than mingled with the implementation details

[duranb]

Behavior changes should be the "headline" rather than mingled with the implementation details

You mean instead of:

2. Conditional Output Panel Display (91e6a5d)

   • Output panel now only displays when editing files that match the adaptation's input file extension within SequenceEditor
   • Prevents showing irrelevant output formats when viewing already-converted output files

You just want

2. Output panel now only displays when editing files that match the adaptation's input file extension within SequenceEditor
3. Prevents showing irrelevant output formats when viewing already-converted output files

[duranb]

A lot of what is described in parts 2 and 3 above sound like what I expected the behavior to be all along, so what was it doing if not that?

There is currently no distinction between input and output files within the UI explicitly. There is only a distinction of if the file is a sequence file or not. That means that if the database is mapping .seq.json as a Sequence, we currently show the *.seq.json file in SequenceEditor. Additionally, we currently don't try to match the extension to the language within the adaptation. We just assume that it is the input language being loaded in SequenceEditor. So if SeqJson is loaded, the wrong linter is being applied.

We realized this a while ago, and our suggestion at the time was to disassociate the .seq.json extension from the Sequence and just treat it as JSON within the database.

[dandelany]

Thanks, the additional context helps.

You mean instead of:

For PRs with issues, we link to the issue at the top. It's fine to have ad-hoc PRs with no issue, but if they're making actual functional changes, we should think of what the issue(s) would contain and start with a short version of that.

Think: how would a smart user report this problem[s] and propose this behavioral change (with knowledge of their own adaptation & our UI, but not our code). That's the perspective I'm generally reviewing from first. All of the implementation & architecture details you have are good to include, but lower. Readers shouldn't need a dev-level understanding of our internals to understand what the problems are, under what conditions, & how the changes affect/fix it

[duranb]

Think: how would a smart user report this problem[s] and propose this behavioral change (with knowledge of their own adaptation & our UI, but not our code). That's the perspective I'm generally reviewing from first. All of the implementation & architecture details you have are good to include, but lower. Readers shouldn't need a dev-level understanding of our internals to understand what the problems are, under what conditions, & how the changes affect/fix it

I think I understand. So you mean that PRs should be for users first and devs second?

[dandelany]

PRs should be for users first and devs second

They're mainly for us - for reviewers, and recording our work for future devs to understand what we did. Reviews are just a lot harder, and records are less useful, without a description of the existing vs. expected behavior. My suggestion above is just a way to take your writing out of the perspective of someone who already knows all the subtleties and details & into the shoes of someone without context who is reading the description fresh.

[duranb]

@dandelany Took another stab at updating the PR description. Let me know if it needs more tweaks!