On 17 July 2010 00:32, Vincent Berthoux wrote:
>
> Here is my patch to allow graphical sign loading and displaying in MacVim.
> I've tested it under Mac OS 10.6.3 with the CoreText Renderer. I'm totally new
> to Objective-C memory management, so you may want to check if there is no
> memory leak left.
>
> The implementation work as follow :
> For each loaded sign in vim, the vim backend convert it as a NSString and
> store it in the vim runtime, and send a message to a gui to load the image.
> The message is rerouted to the textview helper where it is loaded and
> stored in a associative table. The file path is used as the key. Each
> time the sign need to be drawn, the path is sent to the backend with the
> screen coordinates.
>
> Let me know if you have any suggestions/enhancements.
Hi Vincent,
Thanks for the patch! I have taken a glance at the patch but not
tried it out yet. Overall it looks good, I will take a proper look at
it once you've addressed a few points (some serious, many not so
much). Eventually I will merge this.
Here are my first impressions (in no specific order):
- You have to read up on Cocoa memory management ([1] in particular):
e.g. init... methods give you objects that need to be released,
container classes retain objects that you add to them. I've noticed a
few spurious retain messages in the patch that should not be there.
- Whitespace issues: There are a few blank lines delete here and
there, some blank lines added. Please get rid of these. Also, please
format your code more like the MacVim code ("if ( blah )" should be
"if (blah)" etc.).
- The method [MMBackend sendUTF8String] is confusing, just add to the
draw queue directly
- In [MMTextViewHelper drawImage:cellSize:insetSize:frame] you should
not pass all those extra parameters. Use the
rectForRow:column:numRows:numColumns (I think) instead. Some text
views are flipped, others are not so it is not safe to convert
(row,column) to coordinates in the helper "manually". Also, although
this means repeated code, deserialize incoming data in each respective
text view instead of doing it in the helper (like the other draw
messages). Also, call the variables "row, col" instead of "x,y" so it
is obvious what they are.
- Please test that the code works in both the default and core text
renderer (the ATSUI renderer you don't have to worry about).
- "signsImage" should probably be "signImages"
- The proper return type for "init" is "id" (in the helper class).
Also, please change the code to follow the same code as the other
"init" methods.
- Don't leave things like this in the code:
// why not calling the one defined as class method
// on NSString? I don't know, for some reason (maybe
// some initialisation timing...) calling it was
// crashing vim when some signs where loaded in a
// plugin.
It gives a very bad impression. Figure out what your bug is instead
and call the method on NSString (probably related to improper
retain/release use).
- Don't declare variables at the beginning of a function -- declare
and initialize variables at the same time (I'm looking at gui_macvim.m
now). Why are you using the CG functions instead of NSImage by the
way (should be less code)? What happens if the image is not PNG?
- Have you checked that all APIs you use are available on 10.4? (I
haven't checked myself)
- Don't leave printf statements in the patch
Thanks once again for the patch,
Björn
[1]
http://developer.apple.com/mac/library/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmObjectOwnership.html#//apple_ref/doc/uid/20000043-BEHDEDDB
--
You received this message from the "vim_mac" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
