Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy-paste bug fix #5555

Merged

Conversation

saintmatthieu
Copy link
Contributor

@saintmatthieu saintmatthieu commented Nov 9, 2023

Resolves: #5523
Resolves: #5599

  • I signed CLA
  • The title of the pull request describes an issue it addresses
  • If changes are extensive, then there is a sequence of easily reviewable commits
  • Each commit's message describes its purpose and effects
  • There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

  • Each commit compiles and runs on my machine without known undesirable changes of behavior

QA:
This PR intends to restore the exact same behaviour as that of 3.3.3 insofar as the "Always paste audio as new clips" option is disabled and no stretching is involved.

I put together this truth table:

Always paste audio as new clips Editing a clip can move other clips One clip on clipboard only Position Side effect on existing clip
OFF OFF False Before not-enough-room error
OFF OFF False Beginning not-enough-room error
OFF OFF False Middle not-enough-room error
OFF OFF True Before not-enough-room error
OFF OFF True Beginning Shifts data to right and merges
OFF OFF True Middle Shifts data to right and merges
OFF ON False Before Shifts data to right
OFF ON False Beginning Shifts data to right
OFF ON False Middle Shifts data to right
OFF ON True Before Shifts data to right
OFF ON True Beginning Shifts data to right
OFF ON True Middle Shifts data to right and merges
ON OFF False Before not-enough-room error
ON OFF False Beginning not-enough-room error
ON OFF False Middle not-enough-room error
ON OFF True Before not-enough-room error
ON OFF True Beginning not-enough-room error
ON OFF True Middle not-enough-room error
ON ON False Before Shifts data to right
ON ON False Beginning Shifts data to right
ON ON False Middle Splits and moves to right
ON ON True Before Shifts data to right
ON ON True Beginning Shifts data to right
ON ON True Middle Splits and moves to right

There's a lot of redundance I didn't bother cleaning up, sorry about that.

Explanation of column names:

  • "One clip on clipboard only": the region you selected before copying only contained one clip. It is important because pasting more than one clip should prohibit merging.
  • "Position": the position of the cursor (cursor-paste) or the end of the selection (selection-paste):
    • "Before": strictly before the beginning of a clip
    • "Beginning": exactly at beginning of a clip
    • "Middle": within a clip

Besides that:

  • Scenarios of that table relevant to selection-paste (as opposed to cursor-paste) also work
  • Applying Noise Reduction doesn't create new boundaries (if unstretched)
  • Nyquist effects that modify length (e.g. Delay) create new boundaries
  • Nyquist effects that don't modify length (e.g. Tremolo) on unstretched clip don't create new boundaries

@saintmatthieu saintmatthieu self-assigned this Nov 9, 2023
@saintmatthieu saintmatthieu added this to the Audacity 3.4.2 milestone Nov 9, 2023
@saintmatthieu saintmatthieu marked this pull request as ready for review November 9, 2023 20:52
@crsib crsib linked an issue Nov 10, 2023 that may be closed by this pull request
@@ -1942,7 +1937,7 @@ void WaveTrack::HandleClear(
}
else
{
if (split) {
if (split || clearByTrimming) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder what if both are true? I inspected calls to that function and didn't find place where it's the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think either that both happen to be true. AFAICS if split is true, whether clearByTrimming is true or not doesn't make a difference. If the former is false, though, we clear by trimming (as opposed to completely deleting) but the resulting smart clips on the right are moved back left.

Split-cut leaves clips in place,
which isn't necessarily what clear-by-trimming wants.
This rectifies a small behaviour change from 3.3.3:
With "Edit Clip Can Move" checked, pasting at the exact
beginning of a clip used to move that clip right w/o merge.
Since the boundary-checking code was modified,
`WithinPlayRegion` already includes the start sample.
@saintmatthieu saintmatthieu merged commit 23d58d5 into audacity:release-3.4.2 Nov 13, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants