On 10.01.2012 05:54, Eric Wasylishen wrote:
On 2012-01-09, at 2:26 PM, Fred Kiefer wrote:
Much of the overhead when using Neos is caused by the section of
-[GSTheme activate] which loads all images in the theme.
Here are some tests: startup Ink with default theme: 51k
syscalls startup Ink with Neos.theme: 183k syscalls startup Ink
with Neos.theme, with GSTheme.m lines 473 to 539 commented out:
95k syscalls.
I am not completely sure why this code is that way. What I remember
is that we need to override all the already loaded images. But
there was more to it. It could have to do with unloading an image
and restoring the previous image state. I hate the way we deal with
images in themes and have often stated this, but haven't come up
with a better idea up to now.
I didn't like the current approach either… I just committed a
rewrite of how theme images are handled which gets rid of theme
proxies entirely, and eliminates the code in -[GSTheme activate] and
-[GSTheme deactivate] which were setting/restoring images.
Now, when a name is set on an NSImage instance, it also subscribes to
the theme change notification. When an NSImage receives this
notification, it does the name-to-path lookup again that +[NSImage
imageNamed:] did, and reloads itself (discarding all reps) if the
path has changed.
It's a fairly major change but I tested Gorm, GSTest, SimpleAgenda,
and tried switching between the GNUstep default theme, Neos, and
Etoile's Nesedah theme while the apps were running, and all images
seemed to update properly.
I am completely impressed by what you did here. On the other hand, I
cannot look at code without trying to find flaws in it. Sorry, I am just
made that way.
So this is what I found:
- After your code is verified by tests we should get rid of the complete
class GSThemeProxy.
- The code in [NSImage imageNamed:] could be simpified, not that the
proxy is gone. We don't need to get the same image we just created from
the dictionary.
- I don't quite understand what happens when we switch back from having
a theme to not using one. Will the standard GNUstep images get reused?
The question here is whether we send the notification
GSThemeDidActivateNotification when there is no theme at all.
- When the old theme included an image, but the new one doesn't we keep
the old image. Is this what we want? Surely it is better than having no
image.
- Should we really reset the size in themeDidActivate: when
sizeWasExplicitlySet is YES?
- The method pathForImageNamed: should go into the private category and
have an underscore.
- Why is the method pathForImageNamed: using the imageLock? I think this
lock should only be used to protect the name and the name dictionary.
- Images that were not loaded from a file, but have a name assigned to
them would be a problem. A theme could create an image on the fly and
assign a standard name to it. I think the best solution here is just to
not allow this behaviour :-)
Looking at this small list I must say, your code is almost perfect.
_______________________________________________
Gnustep-dev mailing list
Gnustep-dev@gnu.org
https://lists.gnu.org/mailman/listinfo/gnustep-dev