blender
/
blender
136
406
Fork 589
Code Issues 6.3k Pull Requests 766 Projects 19 Wiki Activity

Sequencer: Don't create undo step when click-select does nothing #104453

Closed
Lucas Tadeu wants to merge 10 commits from Yup_Lucas/blender:T94080/sequencer-empty-undo into main
pull from: Yup_Lucas/blender:T94080/sequencer-empty-undo
merge into: blender:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 4 Commits 10 Files Changed 6 +20 -10
6 changed files with 20 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
release/datafiles/locale

@ -1 +1 @@
Subproject commit 4331c8e76c2f42b9fd903716c333d6cdeaa5cebd
Subproject commit 547a54b294f32ee11bce73273c6f183d8b235f92

2
release/scripts/addons

@ -1 +1 @@
Subproject commit b3f0ffc587d197b37eac9a1566d1d24b7bee7d9a
Subproject commit 7d80f2f97161fc8e353a657b179b9aa1f8e5280b
ideasman42 marked this conversation as resolved
Campbell Barton commented 2023-02-08 10:27:25 +01:00
Review
Copy Link

This needs change should not be included.

This needs change should not be included.
Lucas Tadeu commented 2023-02-08 20:32:56 +01:00
Review
Copy Link

any idea how to do that?

tried git submodule foreach --recursive git submodule update --init but that doesn't seem to do anything

any idea how to do that? tried `git submodule foreach --recursive git submodule update --init` but that doesn't seem to do anything
Richard Antalik commented 2023-02-08 22:18:59 +01:00
Review
Copy Link

Not super familiar with new workflow, git submodule foreach --recursive git checkout origin/main worked with synthetic test. I think it would be good to run make update before. But here you change commit to newer one, which I am not sure how it can happen. Also not sure if when submitting PR it shows changes in submodules hash to your branch or to origin/main.

For future, it's best to avoid including submodules in commit by specifying changed files manually.

Not super familiar with new workflow, `git submodule foreach --recursive git checkout origin/main` worked with synthetic test. I think it would be good to run `make update` before. But here you change commit to newer one, which I am not sure how it can happen. Also not sure if when submitting PR it shows changes in submodules hash to your branch or to origin/main. For future, it's best to avoid including submodules in commit by specifying changed files manually.
Campbell Barton commented 2023-02-08 23:30:37 +01:00
Review
Copy Link

To avoid this use git commit -a instead of git commit ..

To avoid this use `git commit -a` instead of `git commit .`.
Lucas Tadeu commented 2023-02-09 06:23:23 +01:00
Review
Copy Link

ack, thanks @iss and @ideasman42

ack, thanks @iss and @ideasman42
Lucas Tadeu commented 2023-02-09 06:31:29 +01:00
Review
Copy Link

Yeah I tried @iss 's command and it didn't make any change for me here. Same for make update.

I don't think it would make a different for trunk/master since the hashes in the .gitmodules are unchanged (all of the point to master)

Yeah I tried @iss 's command and it didn't make any change for me here. Same for make update. I don't think it would make a different for trunk/master since the hashes in the `.gitmodules` are unchanged (all of the point to master)

8
source/blender/editors/include/ED_sequencer.h
View File

@ -19,7 +19,13 @@ struct bContext;
void ED_sequencer_select_sequence_single(struct Scene *scene,
struct Sequence *seq,
bool deselect_all);
void ED_sequencer_deselect_all(struct Scene *scene);
/**
* Iterates over a scene's sequences and deselects all of them.
*
* \param scene: scene containing sequences to be deselected.
* \return true if any sequences were deselected; false otherwise.
*/
bool ED_sequencer_deselect_all(struct Scene *scene);
bool ED_space_sequencer_maskedit_mask_poll(struct bContext *C);
bool ED_space_sequencer_check_show_maskedit(struct SpaceSeq *sseq, struct Scene *scene);

13
source/blender/editors/space_sequencer/sequencer_edit.c
View File

@ -2492,17 +2492,22 @@ void SEQUENCER_OT_copy(wmOperatorType *ot)
/** \name Paste Operator
* \{ */
void ED_sequencer_deselect_all(Scene *scene)
bool ED_sequencer_deselect_all(Scene *scene)
{
Editing *ed = SEQ_editing_get(scene);
bool changed = false;
Yup_Lucas marked this conversation as resolved
Campbell Barton commented 2023-02-08 10:24:11 +01:00
Review
Copy Link

Many operators track changes, the convention is to name this changed.

Many operators track changes, the convention is to name this `changed`.
if (ed == NULL) {
return;
return changed;
}
LISTBASE_FOREACH (Sequence *, seq, SEQ_active_seqbase_get(ed)) {
seq->flag &= ~SEQ_ALLSEL;
if (seq->flag & SEQ_ALLSEL) {
seq->flag &= ~SEQ_ALLSEL;
ideasman42 marked this conversation as resolved
Campbell Barton commented 2023-02-08 10:26:23 +01:00
Review
Copy Link

Since this is intended to track change, this should only be set if the flags changed.

if (seq->flag & SEQ_ALLSEL) {
  seq->flag &= ~SEQ_ALLSEL;
  changed = true;
}
Since this is intended to track change, this should only be set if the flags changed. ``` if (seq->flag & SEQ_ALLSEL) { seq->flag &= ~SEQ_ALLSEL; changed = true; } ```
Lucas Tadeu commented 2023-02-08 20:25:48 +01:00
Review
Copy Link

done

done
changed = true;
}
}
return changed;
}
static void sequencer_paste_animation(bContext *C)

3
source/blender/editors/space_sequencer/sequencer_select.c
View File

@ -974,8 +974,7 @@ static int sequencer_select_exec(bContext *C, wmOperator *op)
/* Deselect everything */
if (deselect_all || (seq && (extend == false && deselect == false && toggle == false))) {
ED_sequencer_deselect_all(scene);
changed = true;
changed |= ED_sequencer_deselect_all(scene);
ideasman42 marked this conversation as resolved
Campbell Barton commented 2023-02-08 10:24:47 +01:00
Review
Copy Link

Use C-style comments for comment blocks.

Also, 'changed' will be true if any sequences were deselected. Otherwise, it'll be false. isn't necessary, this is already part of the functions doc-string and quite clear as well.

Use C-style comments for comment blocks. Also, `'changed' will be true if any sequences were deselected. Otherwise, it'll be false.` isn't necessary, this is already part of the functions doc-string and quite clear as well.
Lucas Tadeu commented 2023-02-08 20:25:37 +01:00
Review
Copy Link

done

done
Richard Antalik commented 2023-02-08 21:43:32 +01:00
Review
Copy Link

I think whole comment is redundant, variable changed is in general used to decide whether operator has been finished or cancelled and therefore generates undo step.

I think whole comment is redundant, variable `changed` is in general used to decide whether operator has been finished or cancelled and therefore generates undo step.
👍 1
Lucas Tadeu commented 2023-02-09 06:21:52 +01:00
Review
Copy Link

removed

removed
Campbell Barton commented 2023-02-09 07:15:15 +01:00
Review
Copy Link

picky prefer changed |= ED_sequencer_deselect_all(scene); ensures in the future, this assignment never clears the changed state.

*picky* prefer `changed |= ED_sequencer_deselect_all(scene);` ensures in the future, this assignment never clears the changed state.
Lucas Tadeu commented 2023-02-09 08:27:30 +01:00
Review
Copy Link

done

done
}
/* Nothing to select, but strips could be deselected. */

2
source/tools

@ -1 +1 @@
Subproject commit e133fc08cd3254bb3d3bd1345028c8486700bca4
Subproject commit baa9ba0405081bde968e4d340971b62e1464e816