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 740169 - Add support for smileys via Unicode-characters
Add support for smileys via Unicode-characters
Status: RESOLVED FIXED
Product: evolution
Classification: Applications
Component: Composer
3.14.x (obsolete)
Other Linux
: Normal enhancement
: ---
Assigned To: Tomas Popela
Evolution QA team
Depends on:
Blocks: 744562
 
 
Reported: 2014-11-15 14:20 UTC by Jonas Hahnfeld
Modified: 2015-02-18 18:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add unicode_character to the structs (4.62 KB, patch)
2014-11-15 14:21 UTC, Jonas Hahnfeld
committed Details | Review
add new preference (10.88 KB, patch)
2014-11-15 14:21 UTC, Jonas Hahnfeld
reviewed Details | Review
finally use the unicode_character if chosen by the user (8.77 KB, patch)
2014-11-15 14:22 UTC, Jonas Hahnfeld
reviewed Details | Review
add new preference (10.08 KB, patch)
2014-11-27 17:05 UTC, Jonas Hahnfeld
reviewed Details | Review
add new preference (12.96 KB, patch)
2015-02-15 16:36 UTC, Jonas Hahnfeld
reviewed Details | Review
finally use the unicode_character if chosen by the user (7.19 KB, patch)
2015-02-15 16:37 UTC, Jonas Hahnfeld
reviewed Details | Review
add new preference (12.96 KB, patch)
2015-02-17 15:03 UTC, Jonas Hahnfeld
committed Details | Review
finally use the unicode_character if chosen by the user (7.20 KB, patch)
2015-02-17 15:04 UTC, Jonas Hahnfeld
committed Details | Review

Description Jonas Hahnfeld 2014-11-15 14:20:28 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
Comment 1 Jonas Hahnfeld 2014-11-15 14:21:27 UTC
Created attachment 290751 [details] [review]
add unicode_character to the structs

The equivalent might be discussable...
Comment 2 Jonas Hahnfeld 2014-11-15 14:21:47 UTC
Created attachment 290752 [details] [review]
add new preference
Comment 3 Jonas Hahnfeld 2014-11-15 14:22:50 UTC
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
Comment 4 Milan Crha 2014-11-18 11:42:20 UTC
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.
Comment 5 Milan Crha 2014-11-18 11:44:36 UTC
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
Comment 6 Milan Crha 2014-11-18 11:47:43 UTC
(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?
Comment 7 Jonas Hahnfeld 2014-11-27 17:00:03 UTC
(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 :-)
Comment 8 Jonas Hahnfeld 2014-11-27 17:05:51 UTC
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...
Comment 9 Jonas Hahnfeld 2014-11-27 17:10:39 UTC
(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
Comment 10 Milan Crha 2014-11-28 07:47:46 UTC
(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.
Comment 11 Milan Crha 2014-11-28 07:48:46 UTC
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.
Comment 12 André Klapper 2014-12-19 22:09:19 UTC
When will Tomas take a look?
Comment 13 Tomas Popela 2015-01-06 10:30:55 UTC
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?
Comment 14 Milan Crha 2015-01-06 11:50:16 UTC
(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.
Comment 15 Jonas Hahnfeld 2015-01-26 19:10:58 UTC
(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?
Comment 16 Milan Crha 2015-02-05 14:26:20 UTC
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.
Comment 17 Jonas Hahnfeld 2015-02-15 16:36:21 UTC
Created attachment 296880 [details] [review]
add new preference

Permament in the preferences and temporary in the composer under the menu "Options"
Comment 18 Jonas Hahnfeld 2015-02-15 16:37:19 UTC
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
Comment 19 Jonas Hahnfeld 2015-02-15 16:39:29 UTC
(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
Comment 20 Milan Crha 2015-02-17 14:04:16 UTC
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.
Comment 21 Milan Crha 2015-02-17 14:07:26 UTC
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'
Comment 22 Milan Crha 2015-02-17 14:40:52 UTC
I asked release team for an approval to include these changes in the upcoming release (we are under UI freeze now).
Comment 23 Jonas Hahnfeld 2015-02-17 15:03:46 UTC
Created attachment 297025 [details] [review]
add new preference

Fixed
Comment 24 Jonas Hahnfeld 2015-02-17 15:04:11 UTC
Created attachment 297026 [details] [review]
finally use the unicode_character if chosen by the user

Fixed
Comment 25 Milan Crha 2015-02-18 09:50:58 UTC
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.
Comment 26 Milan Crha 2015-02-18 09:53:14 UTC
Review of attachment 297026 [details] [review]:

Looks good too (after just a brief patch reading).
Comment 27 Milan Crha 2015-02-18 09:55:45 UTC
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?
Comment 28 Milan Crha 2015-02-18 18:02:25 UTC
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+)