[KPhotoAlbum] [PATCH] Patches (maybe this one won't be formatted by Big Brother)

Michael Witten mfwitten at MIT.EDU
Sun Sep 21 18:15:40 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