GNOME Bugzilla – Bug 495123
Refactor composer headers
Last modified: 2013-09-13 00:56:36 UTC
Here's some refactoring work that came from my "mbarnes-composer" branch. It's unrelated to Bonobo. This patch cuts the size of e-msg-composer-hdrs.c in half by splitting much of its functionality into a set of smaller and simpler "header" classes. Each "header" class implements a type of header field in the composer window. Header class hierarchy: GObject | +-- EComposerHeader (abstract) | +-- EComposerFromHeader | +-- EComposerNameHeader | +-- EComposerTextHeader | +-- EComposerPostHeader There's also a supporting widget: GtkComboBox | +-- EAccountComboBox Here's a brief walkthrough of everything: EAccountComboBox This is a combo box that renders an EAccountList. This is the list of accounts in the "From:" header of the composer window. It listens for changes to the EAccountList and updates itself automatically. EComposerHeader This is an abstract base class for composer headers. It consists of two GtkWidget pointers called "title_widget" and "input_widget", representing the left and right sides of a header row (respectively) in the composer window. Subclasses will populate these two pointers with appropriate widgets and provide a suitable API for manipulating them. EComposerFromHeader This class implements the "From:" header in the composer window. title_widget => GtkLabel input_widget => EAccountComboBox EComposerNameHeader This class implements the "To:", "Cc:" and "Bcc:" headers in the composer window. title_widget => GtkButton that opens an ENameSelectorDialog input_widget => ENameSelectorEntry EComposerTextHeader This class implements the "Reply-To:" and "Subject:" headers in the composer window. title_widget => GtkLabel input_widget => GtkEntry EComposerPostHeader This class implements the "Post To:" header in the composer window. title_widget => GtkButton that opens an EMFolderSelector input_widget => GtkEntry
Created attachment 98787 [details] [review] Proposed patch Fair warning: this is a 4,500 line patch! Best to test the functionality first before reviewing the code in detail.
hmm, didn't we once have some kind of ldtp tests around that could be used for regression tests? sigh...
Point me to them if you know of any!
Matt, I see that the only way to test it is to apply to trunk and take the bugs. No way you can expect me/anyone to review this :). Lets get it in asap for 2.21.3 and watch out for bugs and fix them please. As a additional check, it would be nice, if Milan also could test the patch before it hits trunk.
Milan, can you give this patch a quick smoketest before I commit? The only thing I'm a little nervous about is the "Post-To" header since I don't have an NNTP server handy to test against. Should probably look into setting one up for myself.
Err, I cannot apply this patch to trunk smoothly, probably because of white-spaces cleanup. Very pity, many chunks failed. So I briefly read the code, not sure if I skipped some parts or not, because I got lost a few times. There are things, I do not understand completely, but I'm not gtk-educated much, so I can be wrong, but I see this: a) You use header->input_widget in composer_header_constructor, but that widget can be NULL in that moment, or not? b) You assign to the header->input_widget a GtkHBox instance, but it is not able to receive focus, I think, so the composer_header_button_clicked_cb will print a warning if called? (Maybe it cannot happen, I don't know) And here are some questions, not necessary require answers, just in case. Why are you showing HEADER_CC in e_msg_composer_hdrs_set_reply_to? Do you define EAccount *account twice in build_message? I saw kmaaras fixing such things when clearing warnings. I see that in calls to e_msg_composer_hdrs_get_from_account you sometimes believe it can return NULL, sometimes not. What is the probability it will? I know, the old code was almost same, so this is optional. account_combo_box_has_dupes can stop immediately one find first dupe, right? account_combo_box_test_account can check for NULL as well (I'm not sure if necessary). e_account_combo_box_set_session is not called anywhere in the code, is it ok? also, is it OK to have such thing global for all instances? If the main session cannot change through all live of the program, then it's fine I guess. Why e_msg_composer_hdrs_set_from_account (and subsequent functions) returns gboolean? Prepared for future, I guess.
Created attachment 99547 [details] [review] Revised patch Here's an updated patch with trailing whitespace removed. This revision applied cleanly to trunk.
(In reply to comment #6) > a) You use header->input_widget in composer_header_constructor, but that > widget can be NULL in that moment, or not? EComposerHeader is abstract, which means you cannot create instances of it. Subclasses like EComposerNameHeader and EComposerTextHeader, however, are not abstract. They populate 'header->input_widget' in their own constructors, and then call composer_header_constructor() to initialize stuff common to all headers. > b) You assign to the header->input_widget a GtkHBox instance, but it is not > able to receive focus, I think, so the composer_header_button_clicked_cb will > print a warning if called? (Maybe it cannot happen, I don't know) That's a good point. The GtkHBox is only used in EComposerFromHeader as a hack to also support signatures. Fortunately that header uses a label instead of a button, so composer_header_button_clicked_cb() will never be called for it. We're okay for now, but I'll see if clean up the hack. I copied it from the original code. > Why are you showing HEADER_CC in e_msg_composer_hdrs_set_reply_to? That's how the original code works, though it kinda looks like a typo now that you mention it. Maybe HEADER_REPLY_TO was supposed to be shown instead of HEADER_CC. > Do you define EAccount *account twice in build_message? I saw kmaaras fixing > such things when clearing warnings. Good catch! Probably an oversight on my part. I'll fix that. > I see that in calls to e_msg_composer_hdrs_get_from_account you sometimes > believe it can return NULL, sometimes not. What is the probability it will? I > know, the old code was almost same, so this is optional. It could be that there are no accounts configured, and therefore nothing selected in the "From:" combo box. > account_combo_box_has_dupes can stop immediately one find first dupe, right? It could, but the total number of EAccounts is usually < 10. I believe the original logic is written that way. > account_combo_box_test_account can check for NULL as well (I'm not sure if > necessary). I suppose it could. I don't *think* an EAccountList can contain NULL, but we can always fix the logic if that situation occurs. > e_account_combo_box_set_session is not called anywhere in the code, is it ok? > also, is it OK to have such thing global for all instances? If the main > session cannot change through all live of the program, then it's fine I > guess. Oops, that was supposed to get called from mail_session_init(). I must have forgotten to copy that over from my composer branch. Good catch again! > Why e_msg_composer_hdrs_set_from_account (and subsequent functions) returns > gboolean? Prepared for future, I guess. e_msg_composer_hdrs_set_from_account() takes an account name string, which may or may not in the EAccountComboBox. So it returns TRUE if it found the corresponding EAccount in the combo box, and FALSE otherwise. Wow, thanks for the detailed review! I'll work on fixing the glitches you found. And hopefully you can test it using the updated patch.
(In reply to comment #8) > EComposerHeader is abstract, which means you cannot create instances of it. > Subclasses like EComposerNameHeader and EComposerTextHeader, however, are not > abstract. They populate 'header->input_widget' in their own constructors, and > then call composer_header_constructor() to initialize stuff common to all > headers. Not true, see composer_from_header_init, composer_text_header_init (too late for constructor? I do not know what is the call-order, unfortunately) and composer_name_header_constructor (calls the parent constructor first, before populating input_widget, even button=TRUE, so doesn't matter). > > Why are you showing HEADER_CC in e_msg_composer_hdrs_set_reply_to? > > That's how the original code works, though it kinda looks like a typo now that > you mention it. Maybe HEADER_REPLY_TO was supposed to be shown instead of > HEADER_CC. I thought about that, but I didn't realized in original code such think. > > account_combo_box_has_dupes can stop immediately one find first dupe, right? > > It could, but the total number of EAccounts is usually < 10. > I believe the original logic is written that way. We can improve, probably :) > > account_combo_box_test_account can check for NULL as well (I'm not sure if > > necessary). > > I suppose it could. I don't *think* an EAccountList can contain NULL, but we > can always fix the logic if that situation occurs. This means "when it crashed", right? ;) > > Why e_msg_composer_hdrs_set_from_account (and subsequent functions) returns > > gboolean? Prepared for future, I guess. > > e_msg_composer_hdrs_set_from_account() takes an account name string, which may > or may not in the EAccountComboBox. So it returns TRUE if it found the > corresponding EAccount in the combo box, and FALSE otherwise. OK, but again, nowhere used. New question: the new patch is another +40KB, what does it happened?
I do not know what happened, but I cannot apply the patch :( patching file plugins/exchange-operations/exchange-mail-send-options.c patching file composer/e-composer-header.c patching file composer/e-composer-from-header.c patching file composer/e-msg-composer-hdrs.h patching file composer/e-composer-common.h patch: **** malformed patch at line 707: Index: composer/e-composer-header.h
Created attachment 99563 [details] [review] Revised patch This patch should work better. Sorry about that.
Good, applied smoothly. Apparent of comment #9: a) you forgot there that twice declared "EAccount *account;" in composer/e-msg-composer.c::build_message b) regression, I did a bug fix about showing email addresses in the hint of respective entry, but this patch throws it away. c) create an email with all items filled except of Reply-To. I have all these visible, with Reply-To too. Save the attachment. Close composer window and reopen it from draft folder. I can see only From, Reply-To, Cc, Bcc, Post-To and Subject. In View menu is checked only From, Post-To, Reply to. And when I click to 'To', then it hides Cc and Bcc. d) See enabled and disabled items in the menu. I can hide From. Anyway, srag has right, just fix above issues (if they are issues) and commit to trunk. More people will look into this, more regressions/bugs will be found.
forget c) and d) above, it works same as before patch, probably because of filled "post-to" field. So no new issue :)
Fixed (a) and (b) in comment #12. For (b) I added Milan's tooltip enhancement directly into the EComposerNameHeader and EComposerTextHeader classes. Committed to trunk (revision 34600). I have a bit more work in my composer branch that can be merged into trunk, so I'm leaving this bug open for the time being.
Created attachment 99859 [details] [review] Second patch Here's a few more pieces from my "mbarnes-composer" branch. This cleans up the GtkHBox hack that Milan called me on in comment #6. I also added another widget -- ESignatureComboBox -- which should be self-explanatory (similar to EAccountComboBox, renders an ESignatureList). Various other minor tinkerings too.
In signature_combo_box_refresh_cb, isn't it better to ref and unref actual signature at the end of that function, just to be sure the signature will not be freed while changing model? I'm not sure if it matter, but just in case. Even based on e_signature_combo_box_set_active you expects the pointer will not change, which can be true, I do not know. One visual thing I noticed, the combo was much wider, before patch. Otherwise OK, do it same as with that before, people will yell on some regression, if any (commit to trunk).
(In reply to comment #16) > In signature_combo_box_refresh_cb, isn't it better to ref and unref actual > signature at the end of that function, just to be sure the signature will not > be freed while changing model? I'm not sure if it matter, but just in case. Great observation! That probably also needs to be fixed in EAccountComboBox. I'll make sure to do that before I commit. > One visual thing I noticed, the combo was much wider, before patch. The signature combo was packed to expand before, which I think is wrong. Try expanding the composer window and you'll see what I mean. I only want the account combo to expand, so I packed the signature combo not to. So now it's only as wide as the widest signature name. Thanks for the quick review!
Committed to trunk (revision 34620) with the ESignatureComboBox and EAccountComboBox fixes that Milan pointed out. There is yet another part to this refactoring work but it's not quite done yet.
Hi Lately I've been hitting a problem with Evolution built from SVN: evolution: symbol lookup error: /opt/gnome2/lib/evolution/2.22/components/libevolution-mail.so: undefined symbol: e_account_combo_box_set_session Running on 32bit x86 Ubuntu 7.10 (Gutsy). Looks like something is trying to use e_account_combo_box_set_session which does not exist. Please let me know if there is anything I need to to do work around this problem or if you require more information then please tell me how you want me to collect it.
@liam: ubuntu does not provide up-to-date versions but uses old stable releases, so this is probably unrelated.
Liam, try to checkout newest version of Evolution and recompile. Be sure your revision includes this: http://svn.gnome.org/viewvc/evolution?view=revision&revision=34608
Thanks Andre, I'm currently updating from SVN before each build attempt. Milan thanks, I've checked and I do have that file. I've run diff against it and there are no differences. Perhaps I'm building it wrong. I'm using these instructions: http://www.go-evolution.org/index.php?title=Compiling_Evolution_from_SVN&printable=yes Is there perhaps an official build guide that I can refer to?
I guess you look into the official guide. Are you sure you did ./autogen.sh and ./configure after the checkout/update? Because files for e-account-combo-box are new there, so should be added into Makefile-s (look at widgets/misc). And if they are not, then it can show such message as for you, I guess. Please try reconfigure Evolution.
Hi Milan Thanks for pointing out the ./configure step. This was missing from my building process. It now compiles fine. Now I just need to get the evolution data server working :-)
Closing this. I have the final phase of refactoring work done but the risk of regressions is too high for this late in the development cycle. I'll get this into 2.23 along with the rest of my composer rewrite.