Fix #111169: PointerProperty ignores poll function when setting value #111189

Philipp Oeser · 2023-08-16T17:00:23+02:00

Philipp Oeser commented

2023-08-16 17:00:23 +02:00

According to docs, a pointer PointerProperty poll function should
"determine whether an item is valid for this property".
This was only used for filtering in the UI templates though, setting
(e.g. from python) ignored this entirely.

There was a comment about this in code (so I assume this was somewhat
known), however there doesnt seem to be a reason not to run the poll
when setting the value as well afaict.

Report was about python defined ID properties, but this was also true for
other properties.

For example, you could run the following (even though
rna_Brush_imagetype_poll explicitly forbids this)

image = D.images['Render Result']
brush = D.brushes['Clone']
brush.clone_image = image

So now run the poll in RNA_property_pointer_set (this should not be
performance critical since pointers cannot be animated).

Alternatively, we could have the "set" callback that would be
capable of "determine whether an item is valid for this property" for python
defined properties as well (regular RNA_def_property_pointer_funcs have this,
but then again "poll" actually seems like the right place for this already).

According to docs, a pointer PointerProperty poll function should "determine whether an item is valid for this property". This was only used for filtering in the UI templates though, setting (e.g. from python) ignored this entirely. There was a comment about this in code (so I assume this was somewhat known), however there doesnt seem to be a reason not to run the poll when setting the value as well afaict. Report was about python defined ID properties, but this was also true for other properties. For example, you could run the following (even though `rna_Brush_imagetype_poll` explicitly forbids this) ``` image = D.images['Render Result'] brush = D.brushes['Clone'] brush.clone_image = image ``` So now run the poll in `RNA_property_pointer_set` (this should not be performance critical since pointers cannot be animated). Alternatively, we could have the "set" callback that would be capable of "determine whether an item is valid for this property" for python defined properties as well (regular `RNA_def_property_pointer_funcs` have this, but then again "poll" actually seems like the right place for this already).

Philipp Oeser added 1 commit 2023-08-16 17:00:35 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

e8301b0ab8 Fix #111169 : PointerProperty ignores poll function when setting value

According to docs, a pointer PointerProperty poll function should
"determine whether an item is valid for this property".
This was only used for filtering in the UI templates though, setting
(e.g. from python) ignored this entirely.

There was a comment about this in code (so I assume this was somewhat
known), however there doesnt seem to be a reason not to run the poll
when setting the value as well afaict.

Report was about python defined ID properties, but this was also true for
other properties.

For example, you could run the following (even though
`rna_Brush_imagetype_poll` explicitly forbids this)
```
image = D.images['Render Result']
brush = D.brushes['Clone']
brush.clone_image = image
```

So now run the poll in `RNA_property_pointer_set`.

Alternatively, we could introduce a separate "set" method that would be
capable of "determine whether an item is valid for this property" (but
then again "poll" actually seems like the right place for this already).

Philipp Oeser added this to the Python API project 2023-08-16 17:00:46 +02:00

Philipp Oeser requested review from Campbell Barton 2023-08-16 17:00:55 +02:00

Philipp Oeser requested review from Bastien Montagne 2023-08-16 17:01:03 +02:00

Bastien Montagne commented

2023-08-21 16:51:05 +02:00

@blender-bot package

Blender Bot commented

2023-08-21 16:51:07 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR111189) when ready.

Bastien Montagne reviewed 2023-08-25 11:16:47 +02:00

Bastien Montagne left a comment

LGTM in principal. But I (very) vaguely seem to remember that there was a reason for the current behavior, and the comment in the rna_internal_types.h also seems to imply that the poll function only has very restricted usages.

@ideasman42 @brecht maybe you guys remember better the rational behind current behavior?

LGTM in principal. But I (very) vaguely seem to remember that there was a reason for the current behavior, and the comment in the `rna_internal_types.h` also seems to imply that the poll function only has very restricted usages. @ideasman42 @brecht maybe you guys remember better the rational behind current behavior?

Campbell Barton commented

2023-08-25 12:18:50 +02:00

I'd rather not have poll used for checking if a value can be set for a few reasons.

If setting a value would cause an error (such as creating a loop in the parent/child relationships), I think it's better this is enforced by the set function, this is already the convention and I find it works well.
Poll functions may do slower look-ups which can be OK in the context of a UI, but could slow down scripts.
Since poll may fail for existing values, using poll to reject assignment means it's not necessarily possible to temporarily override a value & restore it, because restoring the value will fail if the poll function fails.

So I rather keep poll as advice which works usefully for a UI filter, but keep the "set" callback as the function which can reject an assignment.

I'd rather not have poll used for checking if a value can be set for a few reasons. - If setting a value would cause an error (such as creating a loop in the parent/child relationships), I think it's better this is enforced by the set function, this is already the convention and I find it works well. - Poll functions may do slower look-ups which can be OK in the context of a UI, but could slow down scripts. - Since poll may fail for existing values, using poll to reject assignment means it's not necessarily possible to temporarily override a value & restore it, because restoring the value will fail if the poll function fails. So I rather keep poll as advice which works usefully for a UI filter, but keep the "set" callback as the function which can reject an assignment.

Philipp Oeser commented

2023-08-25 12:34:19 +02:00

I'd rather not have poll used for checking if a value can be set for a few reasons.

If setting a value would cause an error (such as creating a loop in the parent/child relationships), I think it's better this is enforced by the set function, this is already the convention and I find it works well.

Poll functions may do slower look-ups which can be OK in the context of a UI, but could slow down scripts.

Since poll may fail for existing values, using poll to reject assignment means it's not necessarily possible to temporarily override a value & restore it, because restoring the value will fail if the poll function fails.

So I rather keep poll as advice which works usefully for a UI filter, but keep the "set" callback as the function which can reject an assignment.

That would mean exposing "set" for PointerProperty, right? (atm. this is not available)

> I'd rather not have poll used for checking if a value can be set for a few reasons. > > - If setting a value would cause an error (such as creating a loop in the parent/child relationships), I think it's better this is enforced by the set function, this is already the convention and I find it works well. > - Poll functions may do slower look-ups which can be OK in the context of a UI, but could slow down scripts. > - Since poll may fail for existing values, using poll to reject assignment means it's not necessarily possible to temporarily override a value & restore it, because restoring the value will fail if the poll function fails. > > So I rather keep poll as advice which works usefully for a UI filter, but keep the "set" callback as the function which can reject an assignment. That would mean exposing "set" for `PointerProperty`, right? (atm. this is not available)

Brecht Van Lommel commented

2023-08-26 04:39:13 +02:00

I agree with the points Campbell made.

I would not do anything here at all and consider this not a bug. Any conditions that a poll/set function checks can be violated afterwards by modifying the datablock that is being pointed to.

I agree with the points Campbell made. I would not do anything here at all and consider this not a bug. Any conditions that a poll/set function checks can be violated afterwards by modifying the datablock that is being pointed to.

Philipp Oeser commented

2023-08-29 13:13:39 +02:00

I would not do anything here at all and consider this not a bug. Any conditions that a poll/set function checks can be violated afterwards by modifying the datablock that is being pointed to.

Fine with that, I can close #111169 referring to the discussion here. However:

I think it's better this is enforced by the set function, this is already the convention and I find it works well.

I am confused, set is not available for PointerProperty?

> I would not do anything here at all and consider this not a bug. Any conditions that a poll/set function checks can be violated afterwards by modifying the datablock that is being pointed to. Fine with that, I can close #111169 referring to the discussion here. However: >I think it's better this is enforced by the set function, this is already the convention and I find it works well. I am confused, `set` is not available for `PointerProperty`?

Philipp Oeser commented

2023-12-29 14:56:11 +01:00

OK, coming back to this I think we can close this try at a solution.

I have been asking the following before, and it looks to me that when you want this kind of control over python-defined properties, you would need the set callback [which at the moment of writing is not available for PointerProperty]:

Alternatively, we could have the "set" callback that would be
capable of "determine whether an item is valid for this property" for python
defined properties as well

That would mean exposing "set" for PointerProperty, right? (atm. this is not available)

I am confused, set is not available for PointerProperty?

OK, coming back to this I think we can close this try at a solution. I have been asking the following before, and it looks to me that when you want this kind of control over python-defined properties, you would need the `set` callback [which at the moment of writing is not available for PointerProperty]: > Alternatively, we could have the "set" callback that would be capable of "determine whether an item is valid for this property" for python defined properties as well > That would mean exposing "set" for PointerProperty, right? (atm. this is not available) > I am confused, set is not available for PointerProperty?

Philipp Oeser closed this pull request

2023-12-29 14:56:24 +01:00

Philipp Oeser referenced this pull request

2023-12-29 14:58:18 +01:00

PointerProperty ignores "poll" function [except fo UI dropdowns], "set" function is not exposed (so no real way to validate when setting) #111169

buildbot/vexp-code-patch-coordinator Build done.

Details

Pull request closed

Please reopen this pull request to perform a merge.

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

Fix #111169: PointerProperty ignores poll function when setting value #111189

Pull request closed