Replace usage of Instance(ImageResource) with Image() #693
Conversation
|
Just to confirm - these changes are strictly backwards compatible. Right? |
|
Looks like CI is not at all happy and i'm not sure if it's actually because of changes you introduced in this PR |
|
I was too vigorous about removing imports - hopefully fixed. The error was weird, though - for whatever reason we weren't seeing the actual cause reported at all and only the failure of the task window to clean itself up. |
Codecov Report
@@ Coverage Diff @@
## master #693 +/- ##
==========================================
+ Coverage 39.72% 39.83% +0.10%
==========================================
Files 492 492
Lines 27222 27230 +8
Branches 4134 4134
==========================================
+ Hits 10815 10847 +32
+ Misses 15935 15920 -15
+ Partials 472 463 -9
Continue to review full report at Codecov.
|
| @@ -259,10 +260,10 @@ class DockImages(HasPrivateTraits): | |||
| # --------------------------------------------------------------------------- | |||
|
|
|||
| # Image for closing a tab: | |||
| close_tab = Instance(ImageResource, ImageResource("close_tab")) | |||
| close_tab = Image(ImageResource("close_tab")) | |||
There was a problem hiding this comment.
Is there a specific reason why we can't just use Image("close_tab") which IIUC is the same as Image(ImageResource("close_tab"))?
There was a problem hiding this comment.
The lookup rules for Image and ImageResource are subtly different (they go up different numbers of containing directories when looking for image directories, IIRC) and Image also does caching.
I left them as ImageResource instances since then I know the behaviour is the same, to be conservative.
There was a problem hiding this comment.
So on the one hand we want people to use Image() instead of Instance(ImageResource) but then on the other hand we want to discourage setting a non-ImageResource value to it because Image is too magical?
There was a problem hiding this comment.
No. It's no more magical than ImageResource, it's just different.
I would encourage people to use strings directly in new code if it makes sense for them, or to replace old ImageResource instances with strings when they migrate, as long as they check that it works correctly for them.
In this PR, I didn't want to change two behaviours at once.
There was a problem hiding this comment.
Orthogonal but related in spirit: Yesterday we discussed how unintuitive to read code like this:
from library import BaseCalss
class Foo(BaseClass):
name = "NAME"
where the BaseClass is defined elsewhere (maybe from a separate library) like this:
class BaseClass(HasTraits):
name = Str()
It is not obvious to the developer that name there is a trait rather than just a plain class attribute. This increases cognitive load: developers have to spend more time to understand the code and be surprised by the magic. (Speaking from my own experience, I have been surprised by exactly this behaviour, and that had costed me time trying to understand code that uses this pattern.)
The example above with Str is less bad, because Str does not transform the given "NAME" value.
But for Image that does transform the value, the code is going to be even less straightforward to read and understand.
I'd personally in favour of typing a bit more such that the code is easier to read and to understand. Code is written not just for the machine but for the human as well.
There was a problem hiding this comment.
To put it differently: The developer who sets the trait to a string knows it gets transformed to something else because of the trait type. But another developer who reads the code may not know that.
There was a problem hiding this comment.
This also came up from offline discussion, for the record:
Often when I want to change library code, I want to gather and discover usages of that library feature to understand how it has been used. It is particularly important to decide if a change is backward compatible, and to decide whether a change is worth it. So for example, I can search the enthought GitHub organization for all occurrences of ImageResource to see how it has been used. That's very quick task. But if project codes are not instantiating an instance of ImageResource but simply set a string, such usage will escape my search, i.e. I get less information. If I want to gather the same information, I will need to spend a (hell) lot more time to find what objects have declared the Image trait, where these objects are used (and subclassed) and where the corresponding trait is changed.
|
@corranwebster do you want to resurrect this PR? |
The
Imagetrait provides a strict superset of the functionality ofInstance(ImageResource)and related constructs likeInstance(IImageResource)- in addition to acceptingNoneandImageResourceinstances it accepts strings that define the location of an image.This will potentially make it easier to configure application splash screens and similar.
In terms of changes, this PR:
Instance(ImageResource)withImageImageResource()values as-is even though they could likely be replaced with the string name of the resource directly.Fixes #691.