Sequencer: Don't make undo items when an empty sequencer gets clicked
Needs ReviewPublic
Actions

Authored by Lucas Tadeu (Yup_Lucas) on Mon, Feb 6, 9:20 AM.

Details

Reviewers

Richard Antalik (ISS)

Maniphest Tasks

T94080: Clicking on empty space in editors creates undo step

Summary

Ref T94080

If the sequencer receives a click event, it looks for items to
select/deselect.

When the sequencer is empty (i.e., there are no sequences), we would have
the deselect_all variable set to true called
ED_sequencer_deselect_all to select any existing sequences.

The code just assumed that because the deselect_all flag was true and we
called ED_sequencer_deselect_all that something changed. That's not true
when the sequencer is empty.

Changes made

I've changed ED_sequencer_deselect_all to return a boolean value
indicating whether or not it has mark any sequences for deselection.

The changed variable is set to the value returned by
ED_sequencer_deselect_all.

When there are no sequences present, changed is false.

Notes

I noticed that if I click and drag (Box selection) around the editor even
when there are no sequences an empty Box selection undo item gets created.

We can either update the parent task to get that fixed or create a new
task.

I'm gonna deal with that after this diff.

Test plan:

Check-out this diff
Build blender
Open the Sequencer
Go to Edit > Undo History and confirm the history is empty
Click all over the Sequencer window
Go to Edit > Undo History and confirm the history is still empty

Diff Detail

Repository

rB Blender

Branch

Yup_Lucas/sequencer-empty-undo (branched from master)

Build Status

Buildable 25689
Build 25689: arc lint + arc unit

Event Timeline

Lucas Tadeu (Yup_Lucas) requested review of this revision.Mon, Feb 6, 9:20 AM

Lucas Tadeu (Yup_Lucas) created this revision.

Harbormaster completed remote builds in B25688: Diff 60271.Mon, Feb 6, 9:20 AM

Lucas Tadeu (Yup_Lucas) edited the summary of this revision. (Show Details)Mon, Feb 6, 9:21 AM

Lucas Tadeu (Yup_Lucas) added a task: T94080: Clicking on empty space in editors creates undo step.

Fix comment typo

Harbormaster completed remote builds in B25689: Diff 60272.Mon, Feb 6, 9:24 AM

Lucas Tadeu (Yup_Lucas) added a reviewer: Richard Antalik (ISS).Mon, Feb 6, 9:27 AM

Pratik Borhade (PratikPB2123) retitled this revision from [Sequencer] Don't make undo items when an empty sequencer gets clicked to Sequencer: Don't make undo items when an empty sequencer gets clicked.Mon, Feb 6, 10:40 AM

Pratik Borhade (PratikPB2123) removed a reviewer: Julian Eisel (Severin).

Thanks for patch, this is really good. I have one suggestion in inline, that would make this even better (IMO).

source/blender/editors/space_sequencer/sequencer_edit.c
2506	I think, that you could actually check if any strip is deselected to prevent generating undo steps by clicking in empty space when there are strips in timeline.

Richard Antalik (ISS) added inline comments.Mon, Feb 6, 2:26 PM

source/blender/editors/space_sequencer/sequencer_select.c
977–980	If you agree with suggestion above, this comment won't be even necessary.

Lucas Tadeu (Yup_Lucas) added a subscriber: • isshak (isshak).Mon, Feb 6, 9:52 PM

Lucas Tadeu (Yup_Lucas) added inline comments.

source/blender/editors/space_sequencer/sequencer_edit.c

2506

Hey @isshak (isshak), I don't totally understand your advice.

How would I check if a strip is deselected? Would I have to iterate over all of the scene's strips again and check for the flag when inside sequencer_select_exec? If we do that then we have to go over the same list of strips twice.

Something like:

bool changed = false;

  /* Deselect everything */
if (deselect_all || (seq && (extend == false && deselect == false && toggle == false))) {
  // Marks changed as true when we find that there's something that will get selected
  LISTBASE_FOREACH (Sequence *, strip, SEQ_active_seqbase_get(ed)) {
    if (strip->flag & SELECT) {
      changed = true;
      break;
    }
  }
  ED_sequencer_deselect_all(scene);
}

Lucas Tadeu (Yup_Lucas) added inline comments.Mon, Feb 6, 9:53 PM