feat (pattern-editor): allow for copying single row in highlighted channel when no selection is available

[henrikvilhelmberglund]

This pull request aims to add a feature to allow for coping a single row when no selection is available.

I always thought it was a bit annoying that Ctrl+c without a selection didn't do anything. Now it does something, it copies the entire row at once.

There is a crash if you copy a row without a selection, paste and then press delete, not sure what's going on there so I'm marking this as draft for now.

(If applicable) This pull request improves upon and supercedes #<PR number>.

Changes in this PR:

• (cite all changes made in the PR for change log)
  • Feature (pattern editor): allow for copying single row in highlighted when no selection is available

[henrikvilhelmberglund]

So time to get back to this I think, what I've seen so far: (debugging this causes ctrl c and ctrl v to not do anything, very cool)

• When pasting after copying without a selection ClipInfo.EndColumn is set to 15 which is not a real value
• After that GetSelectColumn() is getting 32768, it should probably not be called at all since nothing is being selected
• After that CopyNoteSection() has an end value of a few millions which then causes problems with the loop below and crashes the program.

I think I'll need to split copying with a selection and copying without a selection for things to make sense.

[henrikvilhelmberglund]

Should work now, there's an assert in debug when you paste from a copy without a selection though, not too sure what to do about that.

Usage:

• Navigate to a row
• Press Copy (no selection needed)
• Navigate to another row
• Paste, the whole row from before is pasted

[Gumball2415]

i'm hesitant to merge this feature as i can't find a good reason for automatically copying an entire row without selecting it.

wouldn't it make sense that if nothing is selected, then nothing is cut/copied?

[henrikvilhelmberglund]

i'm hesitant to merge this feature as i can't find a good reason for automatically copying an entire row without selecting it.

wouldn't it make sense that if nothing is selected, then nothing is cut/copied?

The reason is basically the inverse. If I want to copy an entire row I currently have to select it. This usually involves something like:

• Shift + right + right + right (repeat)
• Click+drag to the right

Or something. In other words it's slow. Especially with multiple different instruments in the same channel where you have to select, copy, paste, select, copy paste a bunch of times.

You're right that it makes sense that if nothing is selected, then nothing is cut/copied. But is this behavior actually useful? For me a key doing nothing (disabled) is just a waste of a key. I would be way happier if I could copy/cut without a selection (cut isn't part of this PR though).

If it's too weird it could be put behind a feature flag.

[Gumball2415]

also, as a reminder: please fill out the pull request template, it helps with changelog documentation

[Gumball2415]

personally, i'm reluctant to have it copy the entire row, but maybe it copying the row (edit: cell) highlighted by the cursor?

a few users (@zeta0134, @HeeminTV) on the NESDev Discord server also share the same sentiment, and there's also precedent of this behavior in OpenMPT.

[henrikvilhelmberglund]

also, as a reminder: please fill out the pull request template, it helps with changelog documentation

How exactly? I tried at the top.

personally, i'm reluctant to have it copy the entire row, but maybe it copying the row highlighted by the cursor?

a few users (@zeta0134, @HeeminTV) on the NESDev Discord server also share the same sentiment, and there's also precedent of this behavior in OpenMPT.

For me this isn't super useful, if you're already in the position to copy the highlighted row you might as well type it, or press shift+down+up.

It also doesn't solve my problem which is I want to copy 1-7 columns (depending on row data) with a single keystroke. This is if you're talking about the data under the highlight of the cursor though.

Maybe "entire row" was a bad description, I meant in a single channel, so it won't copy all channels at once, just the channel which the cursor is in.

[Gumball2415]

How exactly? I tried at the top.

the placeholder text is meant to be overwritten with your changes to be listed in the change log. i should probably make its intent more clear.

For me this isn't super useful, if you're already in the position to copy the highlighted row you might as well type it, or press shift+down+up.

it's more of a personal thing that i think it's not intuitive that it copies the row instead of the cell that the cursor highlights, but i'll have to interview more users about it.

[henrikvilhelmberglund]

it's more of a personal thing that i think it's not intuitive that it copies the row instead of the cell that the cursor highlights, but i'll have to interview more users about it.

Maybe it's not super intuitive but you usually don't copy without a selection anyway so I think it's fine to change the behavior here. Copying with a selection works as before. I don't this this feature would inconvenience someone really, but it's a bit hard to find out that it exists I guess.

This change was meant to make copying all columns of the highlighted channel faster because to me it was a bit annoying. This would be a workflow boost for me personally because I do this a lot and this change would make it a bit more efficient by not requiring mouse selections and so on.

Even if it doesn't make sense in a logical sense that copying nothing copies the row in the highlighted channel, if the result is that I can make songs faster I'm happy with it. But this is a subjective thing. And why I'm fine with making it a setting if needed.

[Gumball2415]

that's fair. i will review the code tomorrow afternoon.

[Gumball2415]

there's an assert fail when pasting the row data; the Column value is 0xF, beyond the tested COLUMNS = 7:

https://github.com/henrikvilhelmberglund/Dn-FamiTracker/blob/fae3a4ced958675f20997c3eb2271db79cdd47f2/Source/PatternEditorTypes.h#L111

investigating this now.

in *CPatternEditor::Copy, it seems that setting the end column to C_EFF4_PARAM2 will fail this assert. using the GetSelectColumn method will validate the column selection.

[henrikvilhelmberglund]

Yes there was some weirdness which I couldn't fix (C noob), I added some workaround to make it work like I wanted it to work but a better solution would be great. No need to rush though if you don't feel like it, I will be away for some days anyway.