VectorDataWidget single value drop fix #6242
Conversation
| @@ -131,6 +131,17 @@ def __dataChanged( self, widget ) : | |||
| for plug, value in zip( self.__dataPlugs(), self.__dataWidget.getData() ) : | |||
| plug.setValue( value ) | |||
|
|
|||
| def _convertValue( self, value ) : | |||
There was a problem hiding this comment.
Am I right in thinking that we are overriding this for the case where the value is dropped onto the plug's label, and PlugValueWidget handles the drop rather than VectorDataWidget?
There was a problem hiding this comment.
Could we move it up to live with the other protected overrides for PlugValueWidget methods? I find things easier to understand if they broadly follow a public/protected/private structure.
There was a problem hiding this comment.
Yes, this is for when the value is dropped onto the plug's label. PlugValueWidget is handling the drop, but it wasn't getting quite the right data type to drop, so this override course-corrects it.
There was a problem hiding this comment.
Right, makes sense to place it up with the other protecteds. Amended into the original, now 72c4bd1
| @@ -664,7 +676,8 @@ def __drop( self, widget, event ) : | |||
| else : | |||
There was a problem hiding this comment.
This isn't a new thing, but in testing I realised that dragging onto the plug label acts differently to dragging onto the cells, although both use the same highlighting style. The label sets the entire value, and the cells act in the same was as a drop on the + button. Do you think there would be any merit in making them both act the same (setting the whole value)? Or maybe we should highlight the + button when the drag goes onto the cells?
Feel free to call this out of scope for this PR though.
There was a problem hiding this comment.
I found that a bit odd too. I'm not entirely sure what I'd want the behavior to be. I'm undecided between "I dragged this value onto the list, so that value should replace the list" and "I didn't mean to replace the whole list, just to add to it".
If we highlight the + button when dragging onto the list, does that entice users to want to use Shift / Ctrl + drop to add / remove from the list?
I'm inclined to leave it out of scope for this PR. At the least, would it be better done on the 1.5 branch?
ericmehl
force-pushed
the
vectorDataWidgetIntDropFix
branch
from
3b191b3 to
72c4bd1
Compare
Previously, we were letting `PlugValueWidget` attempt to convert dropped data into something the target plug can support. When dropping a single value such as `IntData` onto a `VectorDataPlugValueWidget`, this meant converting `IntData` to `IntVectorData`. `*VectorData` supports construction from an integer value, in which case it creates n elements in the vector. This commit instead converts a non-sequence value into a single item sequence holding that value to use as drop data.
ericmehl
force-pushed
the
vectorDataWidgetIntDropFix
branch
from
72c4bd1 to
5ee1b12
Compare
This fixes dropping a single value onto a
VectorDataWidget/VectorDataPlugValueWidget. Previously, if you dropped a single value, the+and-buttons would not activate. This meant it reverted to thePlugValueWidgetbehavior, which attempts to convert the dropped data into the data type expected by the widget.*VectorDataobjects can take a single value in their constructor. ForIntVectorData, this creates a vector of length equal to the value being dropped.StringVectorDatasimilarly turns a string constructor argument into a vector where each letter is an element.So the data would convert and the drop would succeed but with unexpected values.
This PR handles those cases by converting to single-element vectors instead, for correct dropping onto
VectorDataWidget, including the+,-and overall plug (effectively the plug name)`.Checklist