[Evolution-hackers] https://bugzilla.gnome.org/show_bug.cgi?id=697632
Hi All, Probably I have managed to cook a fix for bug 697632. Code review is required to confirm the fix. I am not sure how to create patch. I took a diff and attaching here. File: pluginshttp://bazaar.launchpad.net/%7Esamarjit-adhikari/evolution/evolution-3.6.2/files/353/plugins /mail-to-taskhttp://bazaar.launchpad.net/%7Esamarjit-adhikari/evolution/evolution-3.6.2/files/353/plugins/mail-to-task /mail-to-task.c Function: do_mail_to_event [branch Where I have check in my code] http://bazaar.launchpad.net/~samarjit-adhikari/evolution/evolution-3.6.2/revision/353 [Reason Behind the Change] We are not doing any g_object_ref for folder object in this function. Thus unref should not be present as well. This is a dangling g_object_unref on folder object. But one confusion I have. Do we need to add g_object_ref anywhere on folder object? Do let me know your view as well. With regards, Samarjit Proposed_Fix_697632.diff Description: Binary data ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] https://bugzilla.gnome.org/show_bug.cgi?id=697632
On Mon, 2013-04-15 at 12:20 +0530, samarjit Adhikari wrote: Probably I have managed to cook a fix for bug 697632. Code review is required to confirm the fix. I am not sure how to create patch. I took a diff and attaching here. Hi, thanks for the work on this. If it's related to a bug, then attach patch there, not to the mailing list. From a quick review: a) the '//' is a C++ comment, you get a compiler warning on it; if the unref is not needed, then just delete it. b) how did you get the diff? I suppose it's not from a git clone (with 'git diff'), because the 'git diff' shows also a function name in which you did the change, while your diff doesn't do that. c) Good catch, the e_mail_reader_get_folder() doesn't return reffed folder, but the mail-to-task still unrefs the folder. I guess it's some artifact from the time when the function was introduced. I see few more places in the mail-to-task.c which does the unref, please update your patch and attach it to bugzilla. Thanks and bye, Milan ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] https://bugzilla.gnome.org/show_bug.cgi?id=697632
Thanks Milan for the review. Good catch, the e_mail_reader_get_folder() doesn't return reffed folder, but the mail-to-task still unrefs the folder. I guess it's some artifact from the time when the function was introduced. I see few more places in the mail-to-task.c which does the unref, please update your patch and attach it to bugzilla. That's the confusion I have. So will the good fix be a g_object_ref inside e_mail_reader_get_folder on folder object? or is it ok to delete the unref. In that case I need to delete all unref consistently in mail-to-task. With regards, Samarjit On Mon, Apr 15, 2013 at 1:55 PM, Milan Crha mc...@redhat.com wrote: On Mon, 2013-04-15 at 12:20 +0530, samarjit Adhikari wrote: Probably I have managed to cook a fix for bug 697632. Code review is required to confirm the fix. I am not sure how to create patch. I took a diff and attaching here. Hi, thanks for the work on this. If it's related to a bug, then attach patch there, not to the mailing list. From a quick review: a) the '//' is a C++ comment, you get a compiler warning on it; if the unref is not needed, then just delete it. b) how did you get the diff? I suppose it's not from a git clone (with 'git diff'), because the 'git diff' shows also a function name in which you did the change, while your diff doesn't do that. c) Good catch, the e_mail_reader_get_folder() doesn't return reffed folder, but the mail-to-task still unrefs the folder. I guess it's some artifact from the time when the function was introduced. I see few more places in the mail-to-task.c which does the unref, please update your patch and attach it to bugzilla. Thanks and bye, Milan ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] https://bugzilla.gnome.org/show_bug.cgi?id=697632
On Mon, 2013-04-15 at 14:14 +0530, samarjit Adhikari wrote: That's the confusion I have. So will the good fix be a g_object_ref inside e_mail_reader_get_folder on folder object? or is it ok to delete the unref. In that case I need to delete all unref consistently in mail-to-task. I'm fine with deleting the g_object_unref() calls on folders returned by e_mail_reader_get_folder() in mail-to-task code which run in the main thread (like a response to an action), but if the folder is meant to be passed into a thread, then for thread safety ref the folder before running the thread and unref it at the end of the thread. (I'm not much helpful, right?) :) Definitely do not change e_mail_reader_get_folder(), it should be left as is. Bye, Milan ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] https://bugzilla.gnome.org/show_bug.cgi?id=697632
Thanks for holding my hands and helping me to move forward. With regards, Samarjit On Mon, Apr 15, 2013 at 2:24 PM, Milan Crha mc...@redhat.com wrote: On Mon, 2013-04-15 at 14:14 +0530, samarjit Adhikari wrote: That's the confusion I have. So will the good fix be a g_object_ref inside e_mail_reader_get_folder on folder object? or is it ok to delete the unref. In that case I need to delete all unref consistently in mail-to-task. I'm fine with deleting the g_object_unref() calls on folders returned by e_mail_reader_get_folder() in mail-to-task code which run in the main thread (like a response to an action), but if the folder is meant to be passed into a thread, then for thread safety ref the folder before running the thread and unref it at the end of the thread. (I'm not much helpful, right?) :) Definitely do not change e_mail_reader_get_folder(), it should be left as is. Bye, Milan ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
