[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