Re: koffice

On Saturday 31 October 2009 17:25:23 Thorsten Zachmann wrote:

> There is a lot of code duplication in there (the above part is repeated 4
> times) . How about creating a function to remove the code duplication?

There is indeed a lot of code duplication in there.  Creating a function would
work, but I have an even better plan that I was going to investigate and then
send a mail to koffice-devel about this evening.

If you look at the details, there are also a lot of subtle differences between
the four cases, but I guess those can be overcome with some creative use of
static arrays.

The proposal is about that as far as I can see, there is no enum Edge {left,
top, right, bottom}, which would be very useful to have.  I see this in many
places in KOffice that they are redefined over and over again.  Not many
places allow data to be fetched with reference to which edge it is applied to.

When I asked in #koffice about whether to design the border API like this:

BorderStyle border.getStyle(Edge edge);

or like this:

BorderStyle border.getLeftStyle();
BorderStyle border.getTopStyle();
BorderStyle border.getRightStyle();
BorderStyle border.getBottomStyle();

...the voices where overwhelmingly to using the second way.  I don't like that
very much, but I've gotten so much flak lately for not following the community
that I relented, and it wasn't very important for me.

But I would really like is to introduce a common way for all koffice to
enumerate edges, and have that used in all places.  I think we could save a
lot of code and complexity.

Update:
A more creative grep revealed that there is actually something like this in
KoTableCellStyle.  The enum is called Side instead of Edge, but that's a
trivial difference.

I propose that we introduce the following somewhere in kobase (which should
perhaps be called koutils or something since there are no base classes in
there but utilities):

enum Side {
  LeftSide,
  TopSide,
  RightSide,
  BottomSide
};
const Side sides[] = {LeftSide, TopSide, RightSide, BottomSide};

or Edge instead of Side if you prefer that.

        -Inge








_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Sat October 31 2009, Inge Wallin wrote:
> If you look at the details, there are also a lot of subtle differences
>  between  the four cases, but I guess those can be overcome with some
>  creative use of static arrays.
>

I don't see any need for static arrays here. If you create a function taking a
KoBorder::BorderData and 2 points as parameters no static arrays are needed.

That does not mean that I'm against the proposal you made in you mail. I think
it an enum would have benefits and would reduce code duplication, but let the
ones which were for the solution we have now explain why they prefer it.

Still the function reduces the code duplication in both scenarios.

Thorsten
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel

On Sunday 01 November 2009 06:29:50 Thorsten Zachmann wrote:
> On Sat October 31 2009, Inge Wallin wrote:
> > If you look at the details, there are also a lot of subtle differences
> >  between  the four cases, but I guess those can be overcome with some
> >  creative use of static arrays.
>
> I don't see any need for static arrays here. If you create a function
>  taking a KoBorder::BorderData and 2 points as parameters no static arrays
>  are needed.

That's not enough.  If you look at the calculations of innerOffset and
outerOffset, you will notice that there is the same difference between the
four cases as between these four pairs:

(-1, 0),  (0, -1), (1, 0), (0, 1)

There is also a bug in there, I can see now.  The linewidths of the horizontal
lines should be multiplied by zoomY, not zoomX.

So at least these multipliers and the zoom factor has to be sent as parameters
as well.  But that's not an argument against your suggestion.  I'll do
something about it today.

> That does not mean that I'm against the proposal you made in you mail. I
>  think it an enum would have benefits and would reduce code duplication,
>  but let the ones which were for the solution we have now explain why they
>  prefer it.

Yes, I'm waiting for that.

        -Inge
_______________________________________________
koffice-devel mailing list
koffice-devel@...
https://mail.kde.org/mailman/listinfo/koffice-devel