GNOME Bugzilla – Bug 740169
Add support for smileys via Unicode-characters
Last modified: 2015-02-18 18:06:52 UTC
First: Sorry, if this is the wrong place for new features. In the age of Unicode and UTF-8 the behaviour of sending smileys via images is a bit antiquated IMO. So I added support for smileys via Unicode-characters. Mobile mailers like GMail for Android already support it natively and I have encountered no problems while using it since half a year. I finally formatted my changes in 3 patches but they may be squashed if included into master. I added a new preference to enable Unicode-smileys, so nothing should change without interaction. My only problem is in the third patch: In e-emoticon-tool-button.c, I have not found a way to get the instance of EHTMLEditorView to call the function e_html_editor_view_get_unicode_smileys as in e-html-editor-actions.c
Created attachment 290751 [details] [review] add unicode_character to the structs The equivalent might be discussable...
Created attachment 290752 [details] [review] add new preference
Created attachment 290753 [details] [review] finally use the unicode_character if chosen by the user Problem: I have not found a way to get the instance of EHTMLEditorView in e-emoticon-tool-button.c to call the function e_html_editor_view_get_unicode_smileys as in e-html-editor-actions.c
Thanks for a bug report and patch. This is the right place for such things. I will let the detailed review to Tomas, and the help to you or corrections, if any at all. I'll write below only some small things after a brief patch reading. The idea as such looks fine, though I'd like to know whether also other clients will understand these characters properly, without claiming about missing fonts or anything (a Thunderbird, an Exchange OWA and an Outlook on Windows come on mind). That should be assured before committing.
Review of attachment 290752 [details] [review]: ::: data/evolution.convert @@ +115,3 @@ composer-magic-links = /apps/evolution/mail/composer/magic_links composer-magic-smileys = /apps/evolution/mail/composer/magic_smileys +composer-unciode-smileys = /apps/evolution/mail/composer/unciode_smileys Do not add this one, the evolution.convert is to migrate data from GConf to GSettings. This key didn't exist in the previous version, thus the addition here doesn't make sense. ::: e-util/e-html-editor-view.c @@ +7367,3 @@ + * + * Returns: @TRUE when Unicode-characters should be preferred, @FALSE otherwise. + */ add "Since: 3.14" into the new function comments
(In reply to comment #3) > Problem: > I have not found a way to get the instance of EHTMLEditorView in > e-emoticon-tool-button.c to call the function > e_html_editor_view_get_unicode_smileys as in e-html-editor-actions.c Does this mean that some invocation of the addition of the smiley doesn't work, when the unicode-smileys are enabled?
(In reply to comment #4) > Thanks for a bug report and patch. This is the right place for such things. > > I will let the detailed review to Tomas, and the help to you or corrections, if > any at all. I'll write below only some small things after a brief patch > reading. The idea as such looks fine, though I'd like to know whether also > other clients will understand these characters properly, without claiming about > missing fonts or anything (a Thunderbird, an Exchange OWA and an Outlook on > Windows come on mind). That should be assured before committing. Sorry, haven't found the time to reply. I have now tested the following clients: - Evolution (of course) - GMail - AppleMail - Thunderbird under Windows - Microsoft Outlook and Outlook WebApp - Android GMail - iOS mail app - Windows Phone mail app All look pretty good and show the smileys as intended :-)
Created attachment 291670 [details] [review] add new preference (In reply to comment #5) > Review of attachment 290752 [details] [review]: > > ::: data/evolution.convert > @@ +115,3 @@ > composer-magic-links = /apps/evolution/mail/composer/magic_links > composer-magic-smileys = /apps/evolution/mail/composer/magic_smileys > +composer-unciode-smileys = /apps/evolution/mail/composer/unciode_smileys > > Do not add this one, the evolution.convert is to migrate data from GConf to > GSettings. This key didn't exist in the previous version, thus the addition > here doesn't make sense. > > ::: e-util/e-html-editor-view.c > @@ +7367,3 @@ > + * > + * Returns: @TRUE when Unicode-characters should be preferred, @FALSE > otherwise. > + */ > > add "Since: 3.14" into the new function comments Ok, didn't know...
(In reply to comment #6) > (In reply to comment #3) > > Problem: > > I have not found a way to get the instance of EHTMLEditorView in > > e-emoticon-tool-button.c to call the function > > e_html_editor_view_get_unicode_smileys as in e-html-editor-actions.c > > Does this mean that some invocation of the addition of the smiley doesn't work, > when the unicode-smileys are enabled? That means that you will currently see Unicode-Smileys in the popup "Insert Emoticon" even though it might be disabled. I have just found another "bug": The action for the popup is obviously shared with the header of the submenu "Emoticon" below "Insert". That's why there is now simply a Unicode-smiley. I have just looked into it and haven't found a way to set another label for that button than for the menu. I even didn't find the place where those actions are reused
(In reply to comment #9) > I have just looked into it and haven't found a way to set another label for > that button than for the menu. I even didn't find the place where those actions > are reused It's intended and expected, not an issue. The GtkAction keeps all widgets it is assigned to in sync. I do not see any issue in having images in menu, but placing unicode strings into body. Those unicode strings are even hard to read, when they are small.
I'll left the review to Tomas. I would personally prefer one patch, currently we have three, attached in a "wrong" order. But, as I said, I left it to Tomas.
When will Tomas take a look?
Hi Jonan, first of all let me thank you for you patches. I found some glitches. 1) When I have the unicode smileys disabled I still have the UI smiley chooser showing the unicode smileys (not the image ones) and after clicking on of the it it inserts the image one. 2) The "magic smileys" does not work. The smileys are not replaced as you type (i.e. you write ":-)" and it will replace it with ☺) The other thing (that I don't have strong opinion about) is that when the image smiley is inserted and we switch the composer mode to "Plain Text" the smiley is replaced with its text representation. The thing is that the unicode smiley is just text, so I don't know if it is worth replacing it. Actually I think that we should enable the UI smiley chooser (when unicode smileys are enabled) even in the "Plain Text" mode. What do you guys think about that?
(In reply to comment #13) > 1) When I have the unicode smileys disabled I still have the UI smiley chooser > showing the unicode smileys (not the image ones) and after clicking on of the > it it inserts the image one. Does it actually make sense to change the actions? I think the images are fine, and it doesn't make much difference whether the image is inserted into the message body or the text representation or the unicode character (also see below). > The other thing (that I don't have strong opinion about) is that when the image > smiley is inserted and we switch the composer mode to "Plain Text" the smiley > is replaced with its text representation. The thing is that the unicode smiley > is just text, so I don't know if it is worth replacing it. The 'text representation' may be related to the settings. If users want unicode smileys, then the text is not a three-letter-token, but a single unicode letter, in both text/plain and text/html parts. With images it's different, but should work like now, use the three-letter-token when switching forth and back between plain and HTML modes. > Actually I think > that we should enable the UI smiley chooser (when unicode smileys are enabled) > even in the "Plain Text" mode. What do you guys think about that? Yeah, it makes sense to enable emoticon picker and the Insert->Emoticon menu in both Plain and HTML modes. In HTML either an icon or a unicode letter will be inserted, while in plain either a three-letter-tag or a unicode letter will be inserted, based on the user's preferences. Adding the user preference into the composer itself also makes sense, thus users have it simpler to change. It would be nice to change the message body on the option change, if needed, but not necessary from my point of view. As I said above, I would not change the associated actions, I would keep them with images only.
(In reply to comment #14) > (In reply to comment #13) > > 1) When I have the unicode smileys disabled I still have the UI smiley chooser > > showing the unicode smileys (not the image ones) and after clicking on of the > > it it inserts the image one. > > Does it actually make sense to change the actions? I think the images are fine, > and it doesn't make much difference whether the image is inserted into the > message body or the text representation or the unicode character (also see > below). I think you would have to reopen the editor as that is chosen while creating the buttons. But if you want, I can also revert to the images regardless of the settings and the inserted form of the smileys. > > The other thing (that I don't have strong opinion about) is that when the image > > smiley is inserted and we switch the composer mode to "Plain Text" the smiley > > is replaced with its text representation. The thing is that the unicode smiley > > is just text, so I don't know if it is worth replacing it. > > The 'text representation' may be related to the settings. If users want unicode > smileys, then the text is not a three-letter-token, but a single unicode > letter, in both text/plain and text/html parts. With images it's different, but > should work like now, use the three-letter-token when switching forth and back > between plain and HTML modes. Do you want me to change anything here? Don't use text/plain much myself, so isn't that heavy tested... > > Actually I think > > that we should enable the UI smiley chooser (when unicode smileys are enabled) > > even in the "Plain Text" mode. What do you guys think about that? > > Yeah, it makes sense to enable emoticon picker and the Insert->Emoticon menu in > both Plain and HTML modes. In HTML either an icon or a unicode letter will be > inserted, while in plain either a three-letter-tag or a unicode letter will be > inserted, based on the user's preferences. Adding the user preference into the > composer itself also makes sense, thus users have it simpler to change. It > would be nice to change the message body on the option change, if needed, but > not necessary from my point of view. As I said above, I would not change the > associated actions, I would keep them with images only. Should I change that in these patches or is that another issue?
I'd like you to change these things: a) Insert->Emoticon and the toolbar button for emoticons (which might be moved elsewhere) will be visible in both the Plain and the HTML modes. b) The Emoticon actions will have always images, but will paste into the body an image or a text or a unicode letter, based on the users preference and the currently set editing mode (Plain or HTML) c) The option to change user's preference regarding unicode letters will be present in the composer as well, under an Options menu I hope I didn't overlook anything from the above.
Created attachment 296880 [details] [review] add new preference Permament in the preferences and temporary in the composer under the menu "Options"
Created attachment 296881 [details] [review] finally use the unicode_character if chosen by the user Now always shows the images, but the inserted content depends on the user's choice
(In reply to Milan Crha from comment #16) > I'd like you to change these things: > a) Insert->Emoticon and the toolbar button for emoticons (which might be > moved elsewhere) will be visible in both the Plain and the HTML modes. > b) The Emoticon actions will have always images, but will paste into the body > an image or a text or a unicode letter, based on the users preference and > the currently set editing mode (Plain or HTML) > c) The option to change user's preference regarding unicode letters will be > present in the composer as well, under an Options menu > > I hope I didn't overlook anything from the above. Implemented b) and c). a) isn't fully related to Unicode smileys, so I opened a new entry: https://bugzilla.gnome.org/show_bug.cgi?id=744562 Currently working on an implementation, will post it there
Review of attachment 296880 [details] [review]: Thanks for the patch update. I spotted only two typos when I was reading this patch. It can be fixed just before the commit. ::: e-util/e-html-editor-view.c @@ +3436,3 @@ + * EHTMLEditorView:unciode-smileys + * + * Determines whether Unciode characters should be used for smileys. typos, 'unciode' should be 'unicode'. Twice above.
Review of attachment 296881 [details] [review]: This patch looks fine too, only two minor things noticed: ::: e-util/e-html-editor-view.c @@ +1425,3 @@ +emoticon_insert_span (EHTMLEditorView *view, + EEmoticon *emoticon, + gchar *html) this should be 'const gchar *' @@ +1660,1 @@ + g_free(html); here is missing a whitespace, after the 'g_free'
I asked release team for an approval to include these changes in the upcoming release (we are under UI freeze now).
Created attachment 297025 [details] [review] add new preference Fixed
Created attachment 297026 [details] [review] finally use the unicode_character if chosen by the user Fixed
Review of attachment 297025 [details] [review]: Thanks, it looks fine. I'm still waiting for an approval to commit this for 3.13.91. ::: mail/mail-config.ui @@ +255,3 @@ <child> + <object class="GtkCheckButton" id="chkUnicodeSmileys"> + <property name="label" translatable="yes">Use Unicode characters for smileys</property> Hmm, a shortcut character (an underscore in front of one of them) is missing here, I'll try to not forget of it, in case there is a free character available in that tab. I overlooked this in the previous review, I'm sorry.
Review of attachment 297026 [details] [review]: Looks good too (after just a brief patch reading).
By the way, we have face-laugh as a sequence ":-))", but I think the more common sequence is ":-D". At least my likely-smart-phone uses that sequence. Would it make sense to change the sequence? What do you think?
I received an approve, thus I committed this to sources. I added the shortcut in Edit->Preferences->Composer Preferences and changed the face-laugh sequence to :-D. I also filled bug #744734 to document this in user documentation. Created commit 99913ec in evo master (3.13.91+)