Pose library: fix asset creation operator poll when no object active #104466

Damien Picard · 2023-03-03T15:07:54+01:00

Damien Picard commented

2023-03-03 15:07:54 +01:00

When the POSELIB_OT_create_pose_asset operator was displayed in the
UI, for instance pressing F3 after deleting an object, the poll method
tried to access the mode of a None object, leading to the following
error message being displayed:

Traceback (most recent call last):
  File ".../3.5/scripts/addons/pose_library/operators.py", line 63, in poll
    if context.object.mode != "POSE":
AttributeError: 'NoneType' object has no attribute 'mode'

This commit adds a check for whether an object is currently active.

When the POSELIB_OT_create_pose_asset operator was displayed in the UI, for instance pressing F3 after deleting an object, the poll method tried to access the mode of a None object, leading to the following error message being displayed: ``` Traceback (most recent call last): File ".../3.5/scripts/addons/pose_library/operators.py", line 63, in poll if context.object.mode != "POSE": AttributeError: 'NoneType' object has no attribute 'mode' ``` This commit adds a check for whether an object is currently active.

Damien Picard requested review from Sybren A. Stüvel 2023-03-03 15:08:03 +01:00

Sybren A. Stüvel requested changes 2023-03-07 14:19:10 +01:00

Sybren A. Stüvel left a comment

Thanks for the patch!

It can't land as-is, though. The PR description should be a proper commit message, and thus be slightly more specific than "the operator" or "an error message".

Thanks for the patch! It can't land as-is, though. The PR description should be a proper commit message, and thus be slightly more specific than "the operator" or "an error message".

pose_library/operators.py

						
				@ -63,3 +63,3 @@

				        if context.object.mode != "POSE":

				        if context.object is None or context.object.mode != "POSE":

				            # The operator assumes pose mode, so that bone selection is visible.

				            cls.poll_message_set("The object must be in Pose mode")

Sybren A. Stüvel commented

2023-03-07 14:18:58 +01:00

I think the poll message is now a little vague, as it references 'the object' while there may not even be one.

How about "An active armature object in pose mode is needed"?

I think the poll message is now a little vague, as it references 'the object' while there may not even be one. How about "An active armature object in pose mode is needed"?

👍 1

dr.sybren marked this conversation as resolved

Damien Picard force-pushed dp_fix_poselib_operator_poll from e51b6023e8 to 39b130bc66

2023-03-07 17:29:36 +01:00

Compare

Damien Picard commented

2023-03-07 17:33:34 +01:00

It can't land as-is, though. The PR description should be a proper commit message, and thus be slightly more specific than "the operator" or "an error message".

Thank you for the feedback, I admit it’s sometimes a bit hard to know what granularity needs to go into commit messages. Here I thought the operator mentioned in the commit title was precise enough, and the full error message too much info.

> It can't land as-is, though. The PR description should be a proper commit message, and thus be slightly more specific than "the operator" or "an error message". Thank you for the feedback, I admit it’s sometimes a bit hard to know what granularity needs to go into commit messages. Here I thought the operator mentioned in the commit title was precise enough, and the full error message too much info.

Damien Picard requested review from Sybren A. Stüvel 2023-03-07 17:33:55 +01:00

Sybren A. Stüvel commented

2023-03-07 17:40:37 +01:00

That's fine, and understandable.

My personal approach for such descriptions is to make sure they can be understood without having to read the code. And if I can make it understandable when someone lazily skips over details in the title, even better :)

As for the error message, for me as reviewer it's nice to know explicitly which error is solved. In this case it's also clear from the code, but for more complex situations I want to know what I can expect before looking at the code. If I have to check the code to understand the change, I can never see if the code is doing the right thing or not, making my job as a reviewer that much harder.

This is perfect, thanks!

That's fine, and understandable. My personal approach for such descriptions is to make sure they can be understood without having to read the code. And if I can make it understandable when someone lazily skips over details in the title, even better :) As for the error message, for me as reviewer it's nice to know explicitly which error is solved. In this case it's also clear from the code, but for more complex situations I want to know what I can expect *before* looking at the code. If I have to check the code to understand the change, I can never see if the code is doing the right thing or not, making my job as a reviewer that much harder. This is perfect, thanks!

Sybren A. Stüvel approved these changes 2023-03-07 17:40:57 +01:00

Sybren A. Stüvel merged commit 48f52ddfc3 into blender-v3.5-release

2023-03-07 17:41:59 +01:00

Sybren A. Stüvel referenced this issue from a commit

2023-03-07 17:41:59 +01:00

Pose library: fix asset creation operator poll when no object active

Sybren A. Stüvel deleted branch dp_fix_poselib_operator_poll

2023-03-07 17:42:00 +01:00

Damien Picard commented

2023-03-07 17:42:25 +01:00

Interesting, thanks! I’ll try to do better next time ;)

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Pose library: fix asset creation operator poll when no object active #104466