[KPhotoAlbum] [PATCH 6/8] Cleanup: Moved implementation from SettingsData.h to SettingsData.cpp

[Michael Witten]

On 23 Sep 2008, at 2:57 PM, Henner Zeller wrote:

> Hi,
> Just had a quick look at SettingsData and I must say that I don't
> really like this. I know that it is easy to get carried away with
> aggregating all kind of things in some simple macros. But a
> super-generic macro with _11_ arguments ? Can you think of doing this
> in a different form, maybe using templates ?

You'll notice  that the super generic  macro property_()
is only used to define the macro property_sset() that is
used to create the StringSet properties. This is because
QVariant doesn't know how to deal with StringSet.

At first I  was trying to  use one major macro  to describe
the rest. However, I  realized that there is  unnecessary overhead
for most  other 'properties',  because {read,write}Entry()
effectively performs  a hasKey() itself.  Therefore, the
super generic  macro is  named property_() while  a much
shorter, more specialized one is called property().

Therefore,  the  super   generic  property_()  could  be
simplified  to 4  args  if necessary.  In  fact, it  can
be  removed  all  together, though  that  would  require
implementing the 2 getters and  2 setters by hand, which
is not  bad, but I  like how  they are defined  with the
same  kind of  format as  the other  'properties'; plus,
property_() may be useful later.

Also, templates  wouldn't really  help here,  because we
are creating  different functions, not simply  using the
same function with  different types. If you  can look at
the old  code, you'll  realize that  it's using  a bunch
of  repeating overloaded  setValue() and  value() member
functions. Now those repeated implementations are simply
inlined  directly  by  macro expansion,  which  is  much
cleaner (and arguably more efficient) than it was before.


> I like in general the aligning related things together in columns. But
> this model totally breaks if one line does not fit on one screen and
> thus each line has to be wrapped - all columns are broken and are
> harder to read than if you would've broken it on decent boundaries
> (esp. the properties around line 151 that have a width of 155
> characters). And yes, I have 1440 pixels on my notebook screen and it
> still does not fit full screen (and no, I am not using a horizontal
> scroll bar).

On 23 Sep 2008, at 3:41 PM, Jan Kundrát wrote:
> That's right, it's getting really near the right edge on my screen
> (which means that it'd be like wrapped twice when I'm on my laptop).
> That sucks.

I'm quite certain  I'll sound like I'm  picking a fight,
but why not disable line wrapping?

      You're suggesting  that we wrap lines  ourselves in
      order  to keep  lines  from  being wrapped,  albeit
      atrociously.

      In most cases, short lines  are good or make sense,
      but in this  case, it's clear that  long lines have
      the benefit of exposing  common structure; for this
      code, you should turn wrapping off.

      If  you  must have  wrapping,  at  least make  some
      shortcuts to toggle that bug.

You're  complaining  about  having to  read  these  long
lines, but  hell, I  *wrote* them!  If I  didn't stumble
through atrociously wrapped lines and inefficient scroll
bars, maybe I'm not the one who's doing things wrong.

In any case, I don't want  to make an issue out of this.
I'm simply  as vehement about leaving  purposefully long
lines alone as you are about breaking them up.

> I suspect Jesper is fine with this, but I am getting unhappy looking
> at this code :-/

I'm sorry I've riled you so. Truly.