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 639877 - Retrieve all the past history when scrolling back on a chat window
Retrieve all the past history when scrolling back on a chat window
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: Archives
2.91.x
Other Linux
: Normal enhancement
: 3.8
Assigned To: empathy-maint
: 606754 658888 658889 (view as bug list)
Depends on:
Blocks: 661713
 
 
Reported: 2011-01-18 18:15 UTC by Emilio Pozuelo Monfort
Modified: 2013-01-31 15:53 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Emilio Pozuelo Monfort 2011-01-18 18:15:48 UTC
Right now when you open a chat window, we ask the logger for the last bits of the history. It would be nice if scrolling up would cause all the history to be retrieved.
Comment 1 robert 2011-01-18 18:31:02 UTC
s/all/progressively more, as you keep scrolling up/ - infinite history style
Comment 2 Guillaume Desmottes 2011-01-19 08:18:49 UTC
Yeah, I really like that on the N900.
Comment 3 Guillaume Desmottes 2011-01-19 08:19:28 UTC
*** Bug 606754 has been marked as a duplicate of this bug. ***
Comment 4 Guillaume Desmottes 2011-09-21 10:14:13 UTC
*** Bug 658889 has been marked as a duplicate of this bug. ***
Comment 5 Guillaume Desmottes 2011-10-14 17:00:08 UTC
This is blocked by https://bugs.freedesktop.org/show_bug.cgi?id=41772
Comment 6 Allan Day 2012-03-13 12:56:04 UTC
(In reply to comment #1)
> s/all/progressively more, as you keep scrolling up/ - infinite history style

Or you could use chunking (see how GNOME Documents handles scrolling).

One thing to note - it might be necessary to allow viewing the history for multi-party chats (ie. with more than one participant).
Comment 7 Allan Day 2012-03-13 13:29:59 UTC
Err, make that more than two participants. :)
Comment 8 Guillaume Desmottes 2012-06-20 12:50:20 UTC
We should also support search through logs from the conversation window. So, if I search for "Badger", I'm jumped to the most recent log containing "Badger" and can go back in time to see older occurences.
Comment 9 Debarshi Ray 2012-07-04 17:15:02 UTC
I have started working on this:
http://git.gnome.org/browse/empathy/log/?h=wip/infinite-history
Comment 10 Debarshi Ray 2012-07-05 15:33:43 UTC
I moved the branch to:
http://cgit.freedesktop.org/~debarshir/empathy/log/?h=infinite-history
Comment 11 Debarshi Ray 2012-07-06 13:07:40 UTC
This is now ready to be reviewed.
Comment 12 Debarshi Ray 2012-07-12 16:39:55 UTC
Ping? Would be cool to have it for 3.5.4.
Comment 13 Debarshi Ray 2012-08-29 08:44:38 UTC
Rebased the branch on top of current Git master.
Comment 14 Guillaume Desmottes 2012-08-29 09:50:56 UTC
I tested the branch with a not patched theme (the ubuntu theme), the scrollbar goes top down when trying to scroll up. Is that something we could avoid ?

We don't usually split function call on one line per argument. We just wrap if the call is longer than 80 chars.

theme_adium_add_message() is getting a bit hard to understand. Would be good to document its arguments; especially @add_html_func and @js_funcs.

I'd use #define rather than magic numbers when indexing js_funcs.

theme_adium_prepend_html: could you please document a bit the part of the code loading and executing a js script?
Comment 15 Guillaume Desmottes 2012-08-29 10:04:01 UTC
empathy-chat
---------------------

I guess the 2 last commits can be squashed together.

+	gboolean           watch_scroll;
+	guint              max_page_size;
+	guint              scroll_offset;
Please document the semantic of those variables


The if/else block you added to got_filtered_messages_cb() isn't that clear to me. Some more comments about the logic would be good.


chat_scrollable_connect(): connecting signals in an idle callback looks weird to me. Can't we connect it right away and just ignore it when we don't need it?

Maybe rename chat_add_logs() to chat_fetch_more_logs() or something?

chat_add_logs: I'm not a fan of creating the walker there. Can't we create it earlier? It will probably be used anyway so there is no harm creating it sooner in the object.
Comment 16 Debarshi Ray 2012-08-29 17:52:00 UTC
(In reply to comment #14)
> I tested the branch with a not patched theme (the ubuntu theme), the scrollbar
> goes top down when trying to scroll up. Is that something we could avoid ?


> We don't usually split function call on one line per argument. We just wrap if
> the call is longer than 80 chars.

Fixed.

> theme_adium_add_message() is getting a bit hard to understand. Would be good to
> document its arguments; especially @add_html_func and @js_funcs.

I added some documentation.

I replaced theme_adium_append_html with theme_adium_prepend_html and called it theme_adium_add_html. This simplifies things a bit.

> I'd use #define rather than magic numbers when indexing js_funcs.

I used enums to preserve the symbol names.
Comment 17 Debarshi Ray 2012-08-29 17:54:54 UTC
(In reply to comment #15)
> +    gboolean           watch_scroll;
> +    guint              max_page_size;
> +    guint              scroll_offset;
> Please document the semantic of those variables

Done.

> The if/else block you added to got_filtered_messages_cb() isn't that clear to
> me. Some more comments about the logic would be good.

Done.

> chat_scrollable_connect(): connecting signals in an idle callback looks weird
> to me. Can't we connect it right away and just ignore it when we don't need it?

I explained the reasoning behind the current code in a comment.

> Maybe rename chat_add_logs() to chat_fetch_more_logs() or something?
 
> chat_add_logs: I'm not a fan of creating the walker there. Can't we create it
> earlier? It will probably be used anyway so there is no harm creating it
> sooner in the object.

Ok, I moved it to the constructed method.


Please note that currently I can not use Empathy because it is refusing to log into password based accounts even though the password exists in the keyring, nor does it actually prompt with a dialog to enter it. So I am unable to make bigger changes as I won't be able to test them.
Comment 18 Guillaume Desmottes 2012-08-30 11:50:55 UTC
Thanks Rishi.

Your branch looks good to me. It's now only blocked by a tp-logger release and the approval of the release team if we want to get this in 3.6 as we are post feature freeze already.
Comment 19 Debarshi Ray 2012-09-03 17:02:43 UTC
Rebased the branch against master.
Comment 20 Guillaume Desmottes 2012-09-04 09:14:52 UTC
No telepathy-logger release for 3.5.91 so this will be for 3.7.
Comment 21 Debarshi Ray 2013-01-10 15:42:22 UTC
Rebased the branch against master.

This depends on https://bugs.freedesktop.org/show_bug.cgi?id=54270 being merged and released.
Comment 22 Debarshi Ray 2013-01-15 16:27:35 UTC
The logger bits have now been merged.
Comment 23 Debarshi Ray 2013-01-21 15:43:42 UTC
Now that telepathy-logger 0.8 has been released, I have merged and pushed this to master. Thanks a lot for the reviews!
Comment 24 Guillaume Desmottes 2013-01-31 15:53:29 UTC
*** Bug 658888 has been marked as a duplicate of this bug. ***