[KPhotoAlbum] Patches

Michael Witten mfwitten at MIT.EDU
Sun Sep 21 17:30:26 BST 2008


I had set out to solve a problem with windowing position (on at least  
Mac OS X),
but I ended up performing what I find to be a considerable cleanup (or  
at least
reorganization) of the SettingsData class.

All patches should result in buildable code, but I did not extensively  
test my
changes (though, it's mostly reformatting the existing code or  
recasting the
existing code in terms of macros).

I'm quite afraid that these patches will be rejected for 2 reasons  
(based on
my experience working with other people):

	(1) You can't tell what's going on
	(2) Some of the lines are too long
	(3) This alignment is just funky

(1) You can't tell what's going on
     ------------------------------
     Unfortunately, code reorganization always appears messy in  
patches, even when
     the process is broken into several pieces. My advice is just  
apply the patches
     and look at the resulting code rather than the diff, so that you  
can see it's
     not so ugly after all.

     In fact, the final version of SettingsData.cpp is some 200 lines  
shorter than
     it was before these patches; and that's AFTER moving  
implementation details
     from SettingsData.h to SettingsData.cpp.

(2) Some of the lines are too long
     ------------------------------
     Indeed, I seem to be the only programmer that doesn't like line  
wrapping; I've
     always been amused that people use line wrapping to see all parts  
of a long line
     at once, yet they constrain their lines to lengths that need no  
wrapping!

     I think that line wrapping (that is, cosntraining the lenghts of  
lines) mainly
     stems from the days when people printed code out in order to have  
a better look
     than could be had by viewing one line at time.

     These days, our virtual paper is infinitely expansive and our  
screens are pretty
     large, so it's ridiculous to be constrained even to 80 columns  
per line!

     My position: be reasonable. If there are good reasons to have a  
long line, then
     do so, and don't be worried about not being able to print it out.

     After all, these long lines can always be wrapped! However, you  
may lose formatting
     (such as alignment) that way.

(3) This alignment is just funky
     ----------------------------
     Much of the code in SettingsData.{h,cpp} follows a single pattern  
that is beautifully
     expressed by alignment. Consider the following:

         property_copy( displayLabels, setDisplayLabels, bool );
         property_copy( displayCategories, setDisplayCategories, bool );
         property_copy( autoShowThumbnailView,  
setAutoShowThumbnailView, bool );
         property_copy( showNewestThumbnailFirst, setShowNewestFirst,  
bool );
         property_copy( thumbnailDarkBackground,  
setThumbnailDarkBackground, bool );
         property_copy( thumbnailDisplayGrid, setThumbnailDisplayGrid,  
bool );
         property_copy( previewSize, setPreviewSize, int );
         property_copy( thumbnailSpace, setThumbnailSpace, int );
         property_copy( thumbnailCacheScreens,  
setThumbnailCacheScreens, int );
         property_copy( thumbSize, setThumbSize, int );
         property_copy( thumbnailAspectRatio, setThumbnailAspectRatio,  
ThumbnailAspectRatio );

     and then consider how aligning the various 'components' of these  
lines improves readability
     (let's hope your email viewer doesn't wrap these lines!):

         property_copy( displayLabels            ,  
setDisplayLabels           , bool                 );
         property_copy( displayCategories        ,  
setDisplayCategories       , bool                 );
         property_copy( autoShowThumbnailView    ,  
setAutoShowThumbnailView   , bool                 );
         property_copy( showNewestThumbnailFirst ,  
setShowNewestFirst         , bool                 );
         property_copy( thumbnailDarkBackground  ,  
setThumbnailDarkBackground , bool                 );
         property_copy( thumbnailDisplayGrid     ,  
setThumbnailDisplayGrid    , bool                 );
         property_copy( previewSize              ,  
setPreviewSize             , int                  );
         property_copy( thumbnailSpace           ,  
setThumbnailSpace          , int                  );
         property_copy( thumbnailCacheScreens    ,  
setThumbnailCacheScreens   , int                  );
         property_copy( thumbSize                ,  
setThumbSize               , int                  );
         property_copy( thumbnailAspectRatio     ,  
setThumbnailAspectRatio    , ThumbnailAspectRatio );


Give the patches a go, because I'll be basing my following patches on  
them. ;-P

Sincerely,
Michael Witten




More information about the Kphotoalbum mailing list