ImageViewer: Fix and improve setting default attributes #245
Conversation
janezd
force-pushed
the
imageviewer-context-attr
branch
from
a1ccc45 to
f88b838
Compare
janezd
force-pushed
the
imageviewer-context-attr
branch
2 times, most recently
from
b8dc637 to
07ba906
Compare
| @@ -231,8 +223,13 @@ class Error(OWWidget.Error): | |||
| "Unable to display images! Please ensure that the chosen " | |||
| "Image Filename Attribute store the correct paths to the images." | |||
| ) | |||
| no_image_attr = Msg( | |||
| "Data does not contain any variables with image names.\n" | |||
There was a problem hiding this comment.
This message is a bit confusing for me.
You are looking for data with image paths or image type. Data with image names does not necessarily provide any images.
For example. To see the images, only image column is needed:
| self.title_model.set_domain(data.domain) | ||
| if not self.image_model: | ||
| self.Error.no_image_attr() | ||
| self.commit.now() |
There was a problem hiding this comment.
self.data is not set at this point, so None is sent to the outputs.
If this is intentional, ignore my comment.
However, if there are any StringVariables (even though not images), some data is sent to the outputs.
There was a problem hiding this comment.
By "even though not images" you mean that an attribute is chosen (it cannot not be chosen), but it does not refer to images? For instance, one loads zoo and chooses animal name as image name?
I changed no_images_shown from Error to Warning because the widget still "works", it just shows red icons instead of images. In this sense, it makes sense that it outputs the data.
There was a problem hiding this comment.
If, on the other hand, there are not string variables, the widget errors and outputs no data. I guess this is appropriate.
| # image_model is empty and widget reports an error, | ||
| # but avoid those marked as "image" and in particular the one used | ||
| # for image_attr | ||
| self.title_attr = self.data.domain.class_var or max( |
There was a problem hiding this comment.
Why class var with the highest priority? Is it really more relevant than the image title?
There was a problem hiding this comment.
I'm just keeping the existing behavior. Also because of all existing guides and possibly videos.
It seemed weird to me, too, but it makes sense in one very common setup: if you load images from subdirectories, subdirectory names become classes, and you may want those see class names (e.g. types of handbags, or names of trees, or types of buildings...) as titles.
|
@janezd, what is the state on this one? |
janezd
force-pushed
the
imageviewer-context-attr
branch
from
7b9b99d to
f2b77d6
Compare
|
@markotoplak, I think I addressed @VesnaT 's comments. Could you take a look at tests? If we allow them to fail, please merge and release. |
|
I see we do, and I merged. Would you make a release? |
Issue
Fixes #243.
Description of changes
Also change the models so that image attr must be a string, and title prefers strings (unless there is a class var, which has priority).
Includes