Review Board

1.0

Project Manager core rewrite

Updated 5 months, 3 weeks ago

Christopher Armstrong Reviewers
TheRaven, carmstrong
None Etoile trunk (etoile/trunk/Etoile) 
Proposed ProjectManager update to map/unmap windows.
Can map unmap windows, but root screen drawing is broken and it seems to suffer from some errors from time to time (maybe we need to flush now and then?)
David Chisnall
Posted 5 months, 4 weeks ago (March 15th, 2010, 7:09 a.m.)
I like the overall structure - most of my comments are syntactic in nature:

Please don't use the GNUstep macros for retaining, releasing, and autoreleasing.  They only exist to remove these messages when you compile for the Boehm GC, and none of our code supports that.  Clang will remove these messages automatically when you compile with GC - we may choose to support that in future, but it requires writing -finalize methods everywhere.

I don't understand why you have -destroy methods everywhere.  Please remove these and put their code in -dealloc.  Objects that encapsulate resources should destroy the resources when the object itself is destroyed.  An explicit -destroy is going to cause all sorts of problems when these objects are aliased.

Finally, please try to match the existing style.  Declare methods like this:

- (void)initWithXCBWindow: (XCBWindow*)aWindow;

Not like this:

- (void) initWithXCBWindow:(XCBWindow*)aWindow;

When sending a message, align all of the selector components by their colon, not their start.  Make sure that you indent with tabs and align with spaces, so two lines in the same scope have the same number of tabs at the start and the remainder of the padding is spaces.  :set list in vim will make this easier to spot.  Alternatively, just change your tab size and see what suddenly looks wrong.
This change is incorrect.  The etoile.make file is in the Étoilé tree, not installed with GNUstep Make.
I really, really, don't like that idea.  If it's autoreleased, the object will be destroyed automatically at the end of the run loop iteration.  Why not automatically destroy the X11 resources then?
Please note the spacing here - the original spacing is correct for Étoilé, where there is no gap in the middle of cast expressions and there is a space after colons.  You seem to have changed things to use the Apple conventions in quite a few places.
Please don't use the RELEASE()/RETAIN()/AUTORELEASE() macros from GNUstep.  The needlessly obfuscate the code and are only necessary when using the Boehm GC (which we never intend to support).
I also don't really like the idea that we are destroying the resources encapsulated by an object independently of the object.  It seems cleaner to be destroying them in -dealloc.

This is especially true for things like the picture, where the PM may decide that it wants to keep picture for unmapped windows around for drawing a miniwindow, or something like Exposé showing hidden windows.  
More pedantry with alignment:

Please make sure that colons (not the start of selector components) are aligned.  Indent with tabs, align with spaces, and put a space after each colon.
Please don't use multiple declarations on one line.  Since you're initialising each of these immediately below, please add int16_t at the start of each of the next 4 lines and delete this one.  
Did you delete the -description method?  If so, please can you restore it.  If not, ignore this comment.
Please use SUPERINIT.
Please use FOREACH()
Omit the delegate && part of this.  The runtime also checks whether the receiver is nil, so here we're just checking twice, which adds overhead in the most-common case and reduces it in the less-common case.

Also remove the == YES.  It returns a boolean value, so == YES is semantically null.
Review request changed
Updated 5 months, 3 weeks ago (March 16th, 2010, 5:46 a.m.)
Applied most of the formatting changes.

Removed destroy methods.

Made some fixes to the root window painting (now alot better, but still not perfect)

Fixed some retain bugs.

Removed ASSIGN/RETAIN/RELEASE/AUTORELEASE macros.

Changed extents handling slightly.

Added a way to remove reply handlers from connection (needed to prevent reply handlers to destroyed windows).
David Chisnall
Posted 5 months, 3 weeks ago (March 16th, 2010, 6:16 a.m.)
I can't tell if the review board isn't letting me see changes in the new files, but they all seem to have the old alignment.  A few other general points, but apart from that it looks good.
Sorry - the ASSIGN macro is okay, this one actually saves some effort.  It's just the RETAIN/RELEASE/AUTORELEASE ones that I don't like.  DESTROY() is also okay and would be applicable here.
This is now bad style.  You should do the retain before you do the release, in case the objects are the same (they won't be in this case, but it's a bad habit to get into).
Can you not just use [nc removeObserver: self] here?  Or are there any notifications that it should keep listening for?
Same with this one; please remove the spaces between (id) and initWithScreen: and so on.  

For comments in headers, ideally please use autogsdoc comments:

/** comments like this go in the auto-generated documentation for the declaration immediately after them. */
I can't tell if it's a problem with the review board, or if this file still uses ASSIGN() and the bad indenting.
Please don't use @throw.  Send the exception object a -raise message.  Or, better, just use -raise:format:
I missed this in a few places.  Please use the rvalue on the left of comparisons when comparing an lvalue and rvalue.  It helps catch typos at compile time.
It's better to use an NSIndexSet for this.
Alignment.
SUPERINIT;
I wonder if it would be better to make the functions here into methods on XCBConnection.  Probably combining the check and query version into on method, so you'd have something like:

- (BOOL)initializeExtensionNamed: (NSString*)name withVersionMajor: (int)major minor: (int)minor;
SUPERINIT again.

Alignment in this file.
Review request changed
Updated 5 months, 3 weeks ago (March 18th, 2010, 3:19 a.m.)
Okay, I think I've addressed all the outstanding issues including the formatting problems and bugs. The background should paint a purple colour (I had a bug in the background painting code) instead of the random colour it was using before.
Review request changed
Updated 5 months, 3 weeks ago (March 18th, 2010, 3:32 a.m.)
Formatting issues
ASSIGN() fixes
@throw removal
Some fixes to root window painting (now purple)