Confirming, should be an easy one for the almighty Milan who has already written the same patch for day view...

Created attachment 117991 [details] [review]
proposed evo patch

for evolution;

An easy one. Even it doesn't work best with long summaries. Fortunately regardless of this patch :)

Won't this look funny with:
- zero length summary and/or zero length location (or are they filtered somewhere so they won't actually end up in an icalcomponent)?
- NULL or zero length summary and a non NULL location (or are preceding spaces stripped in printing)?

free_text looks like a kludge to me? Why is it needed?

(In reply to comment #3)
> Won't this look funny with:
> - zero length summary and/or zero length location (or are they filtered
> somewhere so they won't actually end up in an icalcomponent)?
> - NULL or zero length summary and a non NULL location (or are preceding spaces
> stripped in printing)?

There are 4 possible results:
a) ""
b) " (location)"
c) "summary"
d) "summary (location)"

Is anything wrong with that? I do not think so. It would be totally bad to pretend location be a summary in b), thus those brackets. Preceding space? No problem at all, but if you really would like to, then no problem, some
   text = g_concat (text, *test ? " " : "", "(", location, ")", NULL);
would fix it quite simply.

> free_text looks like a kludge to me? Why is it needed?

Because I do not want to allocate new memory when it is not needed. Where I can influence unnecessary allocations there I influence it. My habit. You do not like it, do you?

s/test/text

(In reply to comment #4)
> There are 4 possible results:
> a) ""
> b) " (location)"
> c) "summary"
> d) "summary (location)"

You did check for zero length location. I should have realized that.

> Preceding space? No
> problem at all, but if you really would like to, then no problem, some
>    text = g_concat (text, *test ? " " : "", "(", location, ")", NULL);
> would fix it quite simply.

Might just look sloppy, unaligned on paper.

> > free_text looks like a kludge to me? Why is it needed?
> 
> Because I do not want to allocate new memory when it is not needed. Where I can
> influence unnecessary allocations there I influence it. My habit. You do not
> like it, do you?

You can not return either NULL or a newly allocated string? In that case you could always simply and safely g_free the return of get_summary_with_location().

I haven't often seen "I'll tell you whether you should free my return" functions. But maybe those are quite normal. It just surprised me.

> Might just look sloppy, unaligned on paper.

OK, if approved, I'll commit changed patch, the perfect one. :)

> You can not return either NULL or a newly allocated string? ...

When there is no location set, then function simply returns summary or empty string. Not reallocate something, what's already allocated. Thus cannot return NULL for this case (nonempty summary), but because I do not hold the pointer, I say "do not free it".

It's same strange as using 'char *' for 'const char *' returns, which is on couple other places. Something similar is with g_string_free, even from the other side, you've right. It can be strange on a first look, and I know it doesn't help here at all, but sometimes such strange thing can save quite much.

What's the status of this? I guess we cleared things with Paul, it's only waiting for an approve and a little change with g_strconcat, right? Paul, please confirm, I'll commit if I didn't overlook anything. Thanks.

Looks good to commit. I hope your goin to remove free_text and then commit.

Slightly modified committed to trunk. Committed revision 37099.

I didn't plan to remove it, but if you really like it without that...