DataView Image channel #683
Conversation
Codecov Report
@@ Coverage Diff @@
## master #683 +/- ##
==========================================
+ Coverage 39.23% 39.33% +0.09%
==========================================
Files 491 491
Lines 26936 26968 +32
Branches 4073 4079 +6
==========================================
+ Hits 10569 10608 +39
+ Misses 15904 15901 -3
+ Partials 463 459 -4
Continue to review full report at Codecov.
|
| @@ -182,6 +182,50 @@ def set_text(self, model, row, column, text): | |||
| """ | |||
| raise DataViewSetError("Cannot set value.") | |||
|
|
|||
| def has_image(self, model, row, column): | |||
There was a problem hiding this comment.
On naming... I notice that this is the same naming in the TableEditor / ListStrAdapter / etc. (https://github.com/enthought/traitsui/search?q=get_image&unscoped_q=get_image)
This image gets used with the DecorationRole, which is for displaying an icon or color (e.g. a color square next to the text).
Given the new data view is supposed to be more flexible, e.g. it needs to support alternative editors in the cell such as a spinbox or a combobox, we probably want to differentiate an icon that accompanies the rest of the cell from an image that fills up the entire cell. Since this is new code, could we name this has_icon and get_icon instead?
There was a problem hiding this comment.
There is a distinction in most backends between an image and an icon - an icon is a collection of related images with different styles (ie. versions for enabled/disabled, highlighted, activated, etc.). This is very much not an icon in this sense, and Pyface has no object which corresponds to an icon (IImageResource tries to be both an image and an icon and fails at both). So these method as written are very much a "image"-based.
Let's leave this as-is for now, since this is currently the only way to display image data; we might revise when it comes time to implement custom editors/renderers and provide a separate channel for icon-like image collections.
Also note that Qt decorations are not limited to icons, and can potentially display thumbnails or even full images (as well as colors).
There was a problem hiding this comment.
Okay. Just trying to say that as someone reading this method's docstring, it is not obvious that it is used as a decoration rather than an image filling up the whole cell. It was a bit of a surprise that the editable value is used alongside this image.
Qt documentation is very specific about what DecorationRole means:
The data to be rendered as a decoration in the form of an icon. (QColor, QIcon or QPixmap)
There was a problem hiding this comment.
If you have a large image and no text, the decoration will fill up the whole cell (or most of it - I think there may be a border, and you have to take into account how cell sizes are determined).
As someone writing an implementation of this method, you shouldn't care how this image is being displayed; you are providing some image data, and it is up to the view to determine what to do with it.
| ------- | ||
| image : IImageResource or None | ||
| The image associated with the underlying value, or None if there | ||
| is no image. |
There was a problem hiding this comment.
This None if there is no image is making me wonder why we have has_image at all. Can we just let has_image does the filtering so this get_image does not have to worry about no images? Otherwise it is a bit like having two ways to do the same thing.
There was a problem hiding this comment.
This is mainly for simple synchronicity with the other data channels (has_text, get_text, has_tooltip, get_tooltip etc.) consistent APIs are lower cognitive overhead. But thinking about it, None probably shouldn't be an allowed return value.
Note that I am considering an add-on PR which will allow returning a PIL/Pillow Image in addition, but that's orthogonal.
There was a problem hiding this comment.
Not allowing None as a returned value sounds good to me.
| if value_type.has_image(self.model, row, column): | ||
| image = value_type.get_image(self.model, row, column) | ||
| if isinstance(image, IImageResource): | ||
| return image.create_image() |
There was a problem hiding this comment.
Observation, not a request change: Here to more untested code, but this entire module is untested and supposed to be rewritten.
I don't see an issue opened for rewriting this and getting it covered with tests, did I miss it or should I open it?
There was a problem hiding this comment.
There's a lot of things that don't have issues yet on the dataview roadmap. This is one of them.
|
Note: partially resolves #533 |
This adds an image channel to the DataView.
This currently uses
IImageResourceobjects andImagetraits, which is good enough for many use cases, but not completely generic. Eventually we would like some sort of dynamic image object to be able to be used, but that is orthogonal to the PR.Currently only supported in Qt. Wx support is possible, but not attempting for now.