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 596317 - Adium themes always attempt to combine consecutive messages.
Adium themes always attempt to combine consecutive messages.
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: Chat themes
unspecified
Other Linux
: Normal normal
: ---
Assigned To: empathy-maint
Depends on:
Blocks:
 
 
Reported: 2009-09-25 15:13 UTC by Alistair Buxton
Modified: 2009-09-30 14:19 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Alistair Buxton 2009-09-25 15:13:26 UTC
If a theme does not supply NextContent.html/NextContext.html, or if the Info.plist has key "DisableConbineConsecutive" set to true, the theme engine still attempts to combine consecutive messages.

What happens:

If NextContent.html etc cannot be found, Content.html is used instead.

This can lead to confusing behaviour if the theme is not set up to handle consecutive messages - if Content.html does not contain a html element with ID="insert" then all but the first in a series of messages will be hidden.

What should happen:

If NextContent.html (etc) cannot be found, or combining consecutive messages is explicitly disabled, consecutive messages should be handled as any other: Appended directly to the bottom of the chat window.
Comment 1 Xavier Claessens 2009-09-28 07:38:41 UTC
I don't understand what you mean. See "New File Handling" section on that page: http://trac.adium.im/wiki/CreatingMessageStyles.

Fallback are defined by adium theme. If NextContent.html is missing, empathy must fallback to Content.html

I think this bug should be closed INVALID, no?
Comment 2 Xavier Claessens 2009-09-28 07:54:34 UTC
I made a patch to respect "DisableCombineConsecutive" setting:
http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=shortlog;h=refs/heads/adium-fallback
Comment 3 Alistair Buxton 2009-09-28 10:34:23 UTC
This seems to work in a logical way now:

DisableCombineConsecutive is always honoured.
If it is unset and NextContent is present, combining works OK.
If it is unset and NextContent is missing, combining is disabled.

However, you are right, http://trac.adium.im/wiki/CreatingMessageStyles is ambiguous on what should happen if NextContent is missing and DisableCombineConsecutive is unset (it is not a required field) and some themes may rely on the other possible result. I will pose the question to the Adium people and report back. Thanks.
Comment 4 Alistair Buxton 2009-09-28 13:54:15 UTC
I put this question to mathuaerknedam (adium dev.) CombineConsecutive should by default be enabled when NextContent.html is not present.
Comment 5 Xavier Claessens 2009-09-28 14:25:06 UTC
What do you mean? If NextContent.html is not present we can't combine, so we fallback to Contect.html, isn't that right?

Or do you mean for the "consecutive" message class?
Comment 6 Xavier Claessens 2009-09-28 14:26:39 UTC
(In reply to comment #5)
> fallback to Contect.html, isn't that right?
s/c/n/ ---------------^
Comment 7 Alistair Buxton 2009-09-28 16:05:02 UTC
If Next*.html is not present, we must fallback and do the combining using the fallback template. Combining should only be disabled if explicitly done so in the Info.plist.

BTW as a result of this discussion some clarification has been done on http://trac.adium.im/wiki/CreatingMessageStyles
Comment 8 Alistair Buxton 2009-09-28 16:18:13 UTC
Updates on going...

And also note, this means my examples are broken, or rather they would be if they didn't also explicitly disable combining in the Info.plist.
Comment 9 Xavier Claessens 2009-09-29 07:29:16 UTC
So if I understand correctly, with the proposed branch we do fallbacks correctly, but we don't support the last 2 points which are noted "Adium 1.4+". Empathy do not use Resources/Content.html and Resources/Status.html yet.

I think we should consider adium 1.4+ features as another enhencement bug, and restrict this one (and bug #596303) to bug fix only.
Comment 10 Alistair Buxton 2009-09-29 09:08:29 UTC
Agreed.

Proposed branch is not correct however. Fallbacks work correctly (bug #596303) but the combination behaviour is not correct (this bug). 

Combine consecutive should *NOT* be disabled if Next*.html cannot be found. That is, the fallback is directly substituted for the missing file and *everything* else takes place as normal.  The behaviour I describe in the original report is *WRONG* except for the handling of Info.plist.
Comment 11 Xavier Claessens 2009-09-29 10:06:15 UTC
I don't understand how we can combine consecutive if we don't have Next*.html files... Except for the "consecutive" message class.

With my branch, if we have a message that should be joined with previous ones, but Next*.html are missing, I fallback to use Content.html, js function is "appendMessage" and message classes contains "consecutive".

Is that OK? I don't get how my branch is wrong... Could you please check my code from there? http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=blob;f=libempathy-gtk/empathy-theme-adium.c;h=edb0186e36a952900e007c97f52a4c4d7ad46da2;hb=5cf3173b1002db7a7ce30b3f7d9f2ff6f925825e#l604
Comment 12 Alistair Buxton 2009-09-29 13:17:09 UTC
If the Next* is missing, use the fallback with "appendNextMessage" - ie treat the fallback exactly as if it were the Next*
Comment 13 Alistair Buxton 2009-09-29 13:45:02 UTC
Another way to look at it is that once a template has been selected by the fallback rules, it doesn't matter where it came from: It is *always* handled the same way from that point on.

Or in other words, if you take a theme which supplies only Content.html, and you copy Content.html to NextContent.html, the resulting HTML output should be byte for byte identical.
Comment 14 Xavier Claessens 2009-09-30 07:42:34 UTC
Ok branch updated. It should be OK now. Could you please review it?
Comment 15 Alistair Buxton 2009-09-30 13:32:04 UTC
This works perfectly now. Thanks.
Comment 16 Xavier Claessens 2009-09-30 14:19:54 UTC
Great, thanks.

Branch merged to both master and gnome-2-28