Fix #115166: Batch Rename Action Clips error #115185

Closed

Philipp Oeser wants to merge 1 commits from lichtwerk/blender:115166 into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Philipp Oeser commented

2023-11-20 14:46:36 +01:00

Member

Code from cd7ec0dac6 was assuming the selected IDs in the Outliner
were always objects, however actions can also be selected directly (in
Blender File view).

Code from cd7ec0dac6a8 was assuming the selected IDs in the Outliner were always objects, however actions can also be selected directly (in Blender File view).

Philipp Oeser added 1 commit 2023-11-20 14:46:51 +01:00

de785aa670 Fix #115166 : Batch Rename Action Clips error

Code from cd7ec0dac6 was assuming the selected IDs in the Outliner
were always objects, however actions can also be selected directly (in
Blender File view).

Philipp Oeser requested review from Campbell Barton 2023-11-20 14:47:01 +01:00

Philipp Oeser requested review from Sybren A. Stüvel 2023-11-20 14:47:28 +01:00

Philipp Oeser added this to the User Interface project 2023-11-20 14:47:35 +01:00

Philipp Oeser added the

Animation & Rigging

label 2023-11-20 14:47:41 +01:00

Sybren A. Stüvel commented

2023-12-05 11:10:55 +01:00

Member

The original code was already unnecessarily complex and hard to read. IMO before any more complexity is added, it should be refactored. I don't want to block a bugfix, but I also can't approve of the change, so I'll step down as reviewer.

The original code was already unnecessarily complex and hard to read. IMO before any more complexity is added, it should be refactored. I don't want to block a bugfix, but I also can't approve of the change, so I'll step down as reviewer.

Sybren A. Stüvel refused to review 2023-12-05 11:11:00 +01:00

Christoph Lendenfeld commented

2024-02-06 12:22:34 +01:00

Member

@dr.sybren how are you suggesting to refactor the code. I would extract the logic out of the data = (..... tuple. But I'd like to have your opinion on that before I start doing things.
Also if I do a refactor, should that include all the areas in that function that generate data?

@dr.sybren how are you suggesting to refactor the code. I would extract the logic out of the `data = (.....` tuple. But I'd like to have your opinion on that before I start doing things. Also if I do a refactor, should that include all the areas in that function that generate `data`?

Sybren A. Stüvel commented

2024-02-06 18:10:18 +01:00

Member

A few things that come to mind while reading the code of the WM_OT_batch_rename operator:

Type annotations, or at least documentation of expected types.
The if space_type == 'SEQUENCE_EDITOR': / elif / else chain should be replaced with some registry/strategy pattern, where each space type can be added by simply implementing some class/interface and adding it to the registry.
Split _data_from_context into two functions, for check_context=False and check_context=True. This might be considerably easier given the above step.
Use appropriate, ideally self-explaining data-structures. Having a tuple called data as a cheap class instance is super hard to follow.

But that's only when reading basically the _data_from_context function. So maybe these changes would be too limited. Long functions, deep nesting, mixing many different abstraction levels within the same function, all of these I would feel could use attention.

Things like assert 0 is of course unacceptable, as that should be at the bare minimum an exception with a clear explanation.

There are many more if/elif/else chains that can be converted to a strategy pattern, or at least be reduced to a dictionary lookup of some sort. For example, there are mulitple of such chains that all use action.type, which indicates that their code could be implemented as various functions on the same class, with one such class per action type.

Those are just some thoughts I get when browsing through the code.

A few things that come to mind while reading the code of the `WM_OT_batch_rename` operator: - Type annotations, or at least documentation of expected types. - The `if space_type == 'SEQUENCE_EDITOR':` / `elif` / `else` chain should be replaced with some registry/strategy pattern, where each space type can be added by simply implementing some class/interface and adding it to the registry. - Split `_data_from_context` into two functions, for `check_context=False` and `check_context=True`. This might be considerably easier given the above step. - Use appropriate, ideally self-explaining data-structures. Having a tuple called `data` as a cheap class instance is super hard to follow. But that's only when reading basically the `_data_from_context` function. So maybe these changes would be too limited. Long functions, deep nesting, mixing many different abstraction levels within the same function, all of these I would feel could use attention. Things like `assert 0` is of course unacceptable, as that should be at the bare minimum an exception with a clear explanation. There are many more `if`/`elif`/`else` chains that can be converted to a strategy pattern, or at least be reduced to a dictionary lookup of some sort. For example, there are mulitple of such chains that all use `action.type`, which indicates that their code could be implemented as various functions on the same class, with one such class per action type. Those are just some thoughts I get when browsing through the code.

Christoph Lendenfeld referenced this pull request

2024-03-01 17:23:05 +01:00

Refactor: WM_OT_batch_rename #118967

Christoph Lendenfeld referenced this pull request

2024-03-07 13:25:45 +01:00

Refactor: separate function of WM_OT_batch_rename #119160

Campbell Barton commented

2024-03-12 09:55:14 +01:00

Owner

Since this fix works, I think it'd be best to apply this for 4.1 and handle refactoring separately.

Since this fix works, I think it'd be best to apply this for 4.1 and handle refactoring separately.

Campbell Barton referenced this issue from a commit

2024-03-12 11:20:12 +01:00

Fix #115185: Batch Rename Action Clips error

Philipp Oeser commented

2024-03-12 11:32:12 +01:00

Author

Member

Closing in favor of d4610e70db

Closing in favor of d4610e70dbd1531cd736ecbd6ac871d1131396ce

Philipp Oeser closed this pull request

2024-03-12 11:32:15 +01:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

4 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#115185

No description provided.