953876 - Hide "Show custom smilies"-option for libpurple XMPP and MSN and make it default to false

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[Benedikt Pfeifer]

*** Original post on bio 435 at 2010-07-04 11:52:00 UTC ***

This option is on the account properties of XMPP accounts and as far as I can tell it can't even do what it promises at the moment. I think we should hide it.

Comment 1

[Benedikt Pfeifer]

*** Original post on bio 435 at 2012-10-13 15:34:11 UTC ***

MSN has the same problem.

Comment 2

[Benedikt Pfeifer]

Attachment: Patch v1

*** Original post on bio 435 as attmnt 1957 at 2012-10-13 15:37:00 UTC ***

This is an attempt to fix this bug (I admit I was rather interested in how to patch libpurple than in actually fixing this;).
From what I've seen in the conversion script for libpurple localization files, the strings that I removed should be the right ones.

This patch is untested.

Comment 3

[Florian Quèze [:florian]]

*** Original post on bio 435 at 2012-10-13 15:48:08 UTC ***

To just hide the checkbox from the UI and keep the value to "TRUE" all the time, that patch will do.

Wouldn't we want the behavior to be like if the value was FALSE though?
I'm afraid we will need to patch a few more places :-/
See the purple_account_get_bool calls in http://lxr.instantbird.org/instantbird/search?string=custom_smileys

Comment 4

[Benedikt Pfeifer]

Attachment: WIP v2

*** Original post on bio 435 as attmnt 1959 at 2012-10-14 15:47:00 UTC ***

Maybe this is more like it? I've removed all the code that wouldn't be called if the pref was set to FALSE.

I removed |jabber_conv_support_custom_smileys| because according to lxr the only caller was |jabber_message_smileyfy_xhtml|, which I also removed after deleting the only two calls to it.

I was more unsure about |jabber_custom_smileys_isenabled| which was declared in a header (I couldn't find any references to it though), so I left it in and made it always return false instead.

Feedback would be appreciated :)

Comment 5

[Benedikt Pfeifer]

Comment on attachment 8353716 [details] [diff] [review]
Patch v1

*** Original change on bio 435 attmnt 1957 at 2012-10-14 15:47:47 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 6

[Florian Quèze [:florian]]

*** Original post on bio 435 at 2012-10-14 16:04:03 UTC ***

Except when removing/rewriting a whole libpurple file, I tried to avoid removing large blocks of code. When not including some large block of code was really required, I used #if 0 #endif around it. The reason for this is that when removing a large block of code with a patch, any change inside that block on the upstream will bitrot our patch and force us to apply it by hand at the next update. If we limit to one line changes, the only changes that would bitrot our patch are those that touch lines in immediate proximity of these changed lines.

In this case, I think I would just have replaced the purple_account_get_bool(account, "custom_smileys", TRUE) calls by purple_account_get_bool(account, "custom_smileys", FALSE).

Comment 7

[Benedikt Pfeifer]

Attachment: Patch v3

*** Original post on bio 435 as attmnt 1961 at 2012-10-15 06:05:00 UTC ***

(In reply to comment #5)
> Except when removing/rewriting a whole libpurple file, I tried to avoid
> removing large blocks of code. When not including some large block of code was
> really required, I used #if 0 #endif around it. The reason for this is that
> when removing a large block of code with a patch, any change inside that block
> on the upstream will bitrot our patch and force us to apply it by hand at the
> next update. If we limit to one line changes, the only changes that would
> bitrot our patch are those that touch lines in immediate proximity of these
> changed lines.
> 
> In this case, I think I would just have replaced the
> purple_account_get_bool(account, "custom_smileys", TRUE) calls by
> purple_account_get_bool(account, "custom_smileys", FALSE).

I assumed that less code would be better but this reason is convinving ;)

I'm using FALSE as default value now instead.

Comment 8

[Benedikt Pfeifer]

Comment on attachment 8353718 [details] [diff] [review]
WIP v2

*** Original change on bio 435 attmnt 1959 at 2012-10-15 06:05:49 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8353720 [details] [diff] [review]
Patch v3

*** Original change on bio 435 attmnt 1961 at 2012-10-15 06:16:21 UTC ***

Looks good but you missed http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/msn/slpcall.c#240
(and I checked the list at http://lxr.instantbird.org/instantbird/search?string=custom_smileys quickly as you requested f?, not r?, so feel free to double check :))

Comment 10

[Benedikt Pfeifer]

Attachment: Patch v4

*** Original post on bio 435 as attmnt 1962 at 2012-10-15 06:31:00 UTC ***

(In reply to comment #7)
> Comment on attachment 8353720 [details] [diff] [review] (bio-attmnt 1961) [details]
> Patch v3
> 
> Looks good but you missed
> http://lxr.instantbird.org/instantbird/source/purple/libpurple/protocols/msn/slpcall.c#240

That's the list I used. I'm soo annoyed that such stuff happens frequently to me :S

I updated the patch and added a commit message (including "r=fqueze" for convenience, so won't have to edit it if everything else is fine. Let me know if you don't like people doing that).

Comment 11

[Benedikt Pfeifer]

Comment on attachment 8353720 [details] [diff] [review]
Patch v3

*** Original change on bio 435 attmnt 1961 at 2012-10-15 06:31:47 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8353721 [details] [diff] [review]
Patch v4

*** Original change on bio 435 attmnt 1962 at 2012-10-27 02:50:45 UTC ***

> I updated the patch and added a commit message (including "r=fqueze" for
> convenience, so won't have to edit it if everything else is fine. Let me know
> if you don't like people doing that).

I love it! It's the magic secret for priority processing in the checkin-needed list (or even having the patch checked in immediately when r+'ed ;)). Thanks!

The only comment I still have is that this needs a patch for http://hg.instantbird.org/l10n/libpurple/ too to remove the obsolete strings. You of course shouldn't do it by hand; either script it with sed, or re-run the conversion script. If it turns out to be stupidly difficult for you to do, I'll help / do it.

Comment 13

[Florian Quèze [:florian]]

*** Original post on bio 435 at 2012-10-27 03:36:13 UTC ***

http://hg.instantbird.org/instantbird/rev/a32aa1734fd8