[KPhotoAlbum] Patches

Henner Zeller h.zeller at acm.org
Sun Sep 21 19:05:46 BST 2008


Hi Michael,
Thanks for the patches - good to know that someone tries kphotoalbum
on Mac OS X. I only had a quick glance at your patches this morning
and they look good so far. I am not at my development machine right
now, but as soon as I or anyone from the team is, we're probably going
to submit them.

Don't be concerned about the formatting and such, I guess people
mostly agree, that reasonable formatting is good. Jesper likes lines >
80 characters and has probably nothing against nicely aligned code.

<no-start-of-debate>
I personally avoid lines >80 characters and wrap them properly (emacs
can do this well). Reason for this is that I _do_ want to see all the
code but as well want to have three editor windows next to each other.
So even though there is infinite virtual paper, there is only a
limited physical screen-estate. Two dimensional scrolling is not nice
and auto-wrapping of lines is horrible; after all, code is a result of
art and should look esthetic. The best is to avoid wrapping in the
first place: if control structures get too nested and the lines get
too long then something is wrong with the code anyway; however,
sometimes this is not easy to avoid with certain idioms like
iterators. Anyway, I am used that I can only have one editor and still
ugly long lines in kphotoalbum so it is ok if you add more :-/
</no-start-of-debate>

On Sun, Sep 21, 2008 at 6:30 PM, Michael Witten <mfwitten at mit.edu> wrote:
> 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
>
> _______________________________________________
> KPhotoAlbum mailing list
> KPhotoAlbum at kdab.net
> http://mail.kdab.net/mailman/listinfo/kphotoalbum
>



More information about the Kphotoalbum mailing list