After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 773731 - Port CamelMedium to CamelNameValueArray
Port CamelMedium to CamelNameValueArray
Status: RESOLVED FIXED
Product: evolution-data-server
Classification: Platform
Component: Mailer
unspecified
Other Linux
: Normal normal
: ---
Assigned To: evolution-mail-maintainers
Evolution QA team
Depends on:
Blocks: 764065
 
 
Reported: 2016-10-31 18:44 UTC by Corentin Noël
Modified: 2016-11-01 11:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[evolution] Port to new new CamelMedium API (4.34 KB, patch)
2016-10-31 18:44 UTC, Corentin Noël
accepted-commit_now Details | Review
[EDS] The new CamelMedium API (10.45 KB, patch)
2016-10-31 18:45 UTC, Corentin Noël
accepted-commit_now Details | Review
ema patch (1.70 KB, patch)
2016-10-31 21:04 UTC, Milan Crha
accepted-commit_now Details | Review

Description Corentin Noël 2016-10-31 18:44:19 UTC
This new CamelNameValueArray is really neat to use!
Comment 1 Corentin Noël 2016-10-31 18:44:56 UTC
Created attachment 338844 [details] [review]
[evolution] Port to new new CamelMedium API
Comment 2 Corentin Noël 2016-10-31 18:45:24 UTC
Created attachment 338845 [details] [review]
[EDS] The new CamelMedium API
Comment 3 Milan Crha 2016-10-31 20:03:26 UTC
Review of attachment 338844 [details] [review]:

The patch looks good, it builds fine as well. Feel free to commit to the wip/camel-more-gobject branch

::: src/em-format/e-mail-part-headers.c
@@ +93,1 @@
+		camel_name_value_array_get (headers, ii, &header_name, &header_value);

being over-paranoic, I would assign to both header_xxx variables NULL and check for non-NULL of both, before passing them to the g_ascii_strcasecmp(). I know it wasn't the case before.
Comment 4 Milan Crha 2016-10-31 20:12:18 UTC
Review of attachment 338845 [details] [review]:

I found only the coding style issues, otherwise looks good. Similar to the evo part, please commit to the wip/camel-more-gobject branch.

Also, I didn't notice it when reading the evolution patch, but it would be nice to check the return value of camel_name_value_array_get() there as well.

::: src/camel/camel-medium.h
@@ +74,3 @@
 						 const gchar *name);
+	CamelNameValueArray *
+	(*get_headers)		(CamelMedium *medium);

coding style: indent one more tab to the right

@@ +90,3 @@
 void		camel_medium_remove_header	(CamelMedium *medium,
 						 const gchar *name);
+const gchar *camel_medium_get_header		(CamelMedium *medium,

coding style: missing tab before function name (beware of tabs on the right of the function name)

@@ +93,2 @@
 						 const gchar *name);
+CamelNameValueArray *	camel_medium_get_headers	(CamelMedium *medium);

coding style: the return type is too long, add the function name to the next line, but at the same indentation level as the previous functions (two tabs on the left).
Comment 5 Milan Crha 2016-10-31 20:17:28 UTC
This change breaks evolution-mapi, but do not worry, I'll adapt it.
Comment 6 Milan Crha 2016-10-31 21:02:06 UTC
What if we rename camel_medium_get_headers() to camel_medium_dup_headers()? Then it'll be clear that the returned pointer is not tight to the internal headers any more. See how the CamelMessageInfo accessors do it. I'd say it's a clear API, from which you understand the life time of the return value without much need to read the documentation.
Comment 7 Milan Crha 2016-10-31 21:04:55 UTC
Created attachment 338852 [details] [review]
ema patch

for evolution-mapi;
Comment 8 Milan Crha 2016-11-01 11:43:55 UTC
This is in wip/camel-more-gobject branches of respective products right now.

(In reply to Milan Crha from comment #6)
> What if we rename camel_medium_get_headers() to camel_medium_dup_headers()?

And I did this one as well.

I'm closing this bug report.