*** Original post on bio 958 by KillerCookie <mk3ll3r AT gmail.com> at 2011-08-02 16:46:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***

*** Original post on bio 958 as attmnt 765 by mk3ll3r AT gmail.com at 2011-08-02 16:46:00 UTC *** When opening a new IM-session the log of the last session should be displayed. I even think this might be the default behavior because most people i know really like this feature. But i'll also be happy to have it as an opt-in option or an addon. The content of the log should have a different look than new messages to prevent confusion and the scrollbar should jump down to the most recent message so you don't have to do it yourself. The content of the displayed log might be customizable by providing options like: - show the last session - show x messages of the last session To clearly show what i mean i attached a screenshot of Pidgin which provides this functionality as a plugin shipped with the default package.

*** Original post on bio 958 at 2011-10-23 02:40:46 UTC *** Confirming as wanted, although this probably needs us to have better logs first.

*** Original post on bio 958 by Andreas Schleifer <webmaster AT segaja.de> at 2012-01-26 09:44:17 UTC *** I would like to see these feature aswell.

*** Original post on bio 958 as attmnt 1411 at 2012-04-28 17:47:00 UTC *** Here's a WIP and I'd like to have feedback if I'm on the right track here. Coding style nits and the like are not necessary yet. I chose to get a fixed number of messages as context from the logs; this means that messages from more than one session can be shown if the sessions were too short. I also like the idea to have this customizable as suggested in comment 0 (a fixed number of messages/last session/no context).

*** Original post on bio 958 at 2012-04-28 17:50:41 UTC *** One more thing: The messages are not shown as context messages yet. Setting "context = true" on each message failed with something like "not possible on wrapped object". Any ideas?

*** Original post on bio 958 at 2012-04-28 18:11:55 UTC *** By the way I'd also be happy to move the code to get the N last messages from the log (or messages of the N last sessions) into the logger and only call the function with the number of context messages we want from the conversation binding.

*** Original post on bio 958 at 2012-04-28 18:40:32 UTC *** (In reply to comment #4) > One more thing: > The messages are not shown as context messages yet. Setting "context = true" on > each message failed with something like "not possible on wrapped object". Any > ideas? This is because the number of messages you actually end up adding from the logs needs to be added to _readCount (which is currently set in _showFirstMessages) before you call addMsg for the first time. You have set it up to call _showContextFromLogs before _showFirstMessages and add the messages from there. So you would have to set _readCount to the number of messages you are adding before calling addMsg in _showContextFromLogs. However it might be tidier to combine _showContextFromLogs with _showFirstMessages instead, and set _readCount set to the total number of what will be context messages. You could still split off the part of _showContextFromLogs which actually fetches the messages from the logs (rather than displaying them) into a separate function. (Whether this should be in the logger file I don't know.) I think the idea of a pref for the number of context messages to fetch is good. (Maybe set to -1 to fetch the entire last session.)

*** Original post on bio 958 at 2012-04-28 22:36:05 UTC *** (In reply to comment #3) > Created attachment 8353163 [details] [diff] [review] (bio-attmnt 1411) [details] > WIP > > Here's a WIP and I'd like to have feedback if I'm on the right track here. > Coding style nits and the like are not necessary yet. So my overall feedback without looking at the details: - First, thanks for working on this, and I'm glad you could get something that seems to work with so few code changes! - Your idea of putting in the logger the code to fetch the last N messages (comment 5) seems good. Maybe add a method like this to imILogger: void getLastNMessagesFromLogEnumerator(in nsISimpleEnumerator, [optional] out unsigned long messageCount, [retval, array, size_is(messageCount)] out prplIMessage messages); - Pushing the same idea forward a bit, I wonder if it's really the UI's job to care about this. Wouldn't it make sense to have the getMessages method of the imIConversation also return context messages read from the disk? Maybe add an optional boolean parameter aIncludeContextMessagesFromPreviousSession (the name seems a bit long :-() to the getMessages method? - Adding a hidden pref to customize how many messages are retrieved seems a good idea. Would it be OK to also make that pref control how many context messages from the current session are redisplayed when redisplaying a conversation that was on hold (I tend to think "yes", but I suspect others may disagree)? If the answer is yes, then we have an opportunity to make the number of context messages displayed consistent. - I'm a bit worried that this introduces synchronous disk I/O when displaying a conversation for the first time. I'm afraid fixing this would require API changes that are outside of the scope of this bug though. > I chose to get a fixed number of messages as context from the logs; This sounds good. > this means that messages > from more than one session can be shown if the sessions were too short. Should we insert a context message between them?

*** Original post on bio 958 at 2012-04-28 23:12:27 UTC *** (In reply to comment #7) > > this means that messages > > from more than one session can be shown if the sessions were too short. > > Should we insert a context message between them? I meant system message of course here.

*** Original post on bio 958 at 2012-04-29 13:21:32 UTC *** (In reply to comment #7) > - Your idea of putting in the logger the code to fetch the last N messages > (comment 5) seems good. Maybe add a method like this to imILogger: > void getLastNMessagesFromLogEnumerator(in nsISimpleEnumerator, > [optional] out unsigned long messageCount, > [retval, array, size_is(messageCount)] out prplIMessage > messages); Yes, please do this. :) > - Pushing the same idea forward a bit, I wonder if it's really the UI's job to > care about this. Wouldn't it make sense to have the getMessages method of the > imIConversation also return context messages read from the disk? Maybe add an > optional boolean parameter aIncludeContextMessagesFromPreviousSession (the name > seems a bit long :-() to the getMessages method? This does seem like it would make more sense. Instead of adding a boolean parameter, couldn't the conversation just read from the pref directly whether it needs to return context messages or not? > - Adding a hidden pref to customize how many messages are retrieved seems a > good idea. Would it be OK to also make that pref control how many context > messages from the current session are redisplayed when redisplaying a > conversation that was on hold (I tend to think "yes", but I suspect others may > disagree)? If the answer is yes, then we have an opportunity to make the number > of context messages displayed consistent. Are you suggesting here that we put a cap on the number of messages that are available when a conversation is on hold? I'm not necessarily against this, but I think we need someway to shut this off (although, I guess the data would still be available in the logs...). A difficult situation, I think, is when you were pinged -- you'd obviously want this to be in the conversation. > > this means that messages > > from more than one session can be shown if the sessions were too short. > > Should we insert a context message between them? I don't think so, showing them as context (and in time bubble, it'll show the time difference between...) should be enough information. Although when rejoining a MUC this will probably be kind of strange as your "context" could potentially have a big gap in it. Thanks for looking at this. :)

*** Original post on bio 958 at 2012-04-29 13:35:48 UTC *** (In reply to comment #7) > good idea. Would it be OK to also make that pref control how many context > messages from the current session are redisplayed when redisplaying a > conversation that was on hold (I tend to think "yes", but I suspect others may > disagree)? If the answer is yes, then we have an opportunity to make the number > of context messages displayed consistent. I think this would be the correct behaviour once infinite scroll lands. Currently it really makes too many assumptions about when and why people put a conversation on hold. For example - It may be an ongoing conversation, but with large gaps in time between messages, so you put it on hold until a new message arrives. Then you'll be annoyed if the previous conversation goes "missing". - You may put a channel on hold because there is something you wish to respond to later, but you haven't got time at the moment. Again, it's counterintuitive to have to go to logs. Even if you go to the logs, the right log to look at will be the current conversation, despite the fact that that conversation is already open and doesn't contain the messages. Confusing.

*** Original post on bio 958 at 2012-04-29 14:33:50 UTC *** (In reply to comment #9) > (In reply to comment #7) > > - Adding a hidden pref to customize how many messages are retrieved seems a > > good idea. Would it be OK to also make that pref control how many context > > messages from the current session are redisplayed when redisplaying a > > conversation that was on hold (I tend to think "yes", but I suspect others may > > disagree)? If the answer is yes, then we have an opportunity to make the number > > of context messages displayed consistent. > Are you suggesting here that we put a cap on the number of messages that are > available when a conversation is on hold? I'm not necessarily against this, but > I think we need someway to shut this off (although, I guess the data would > still be available in the logs...). A difficult situation, I think, is when you > were pinged -- you'd obviously want this to be in the conversation. I was suggesting limiting only the number of context messages. If you are pinged, that's most likely in an unread message. aleth's comment 10 about the cases where this could be annoying makes sense though (as I said in my original, I expected some disagreement on this).

*** Original post on bio 958 by Michael de Raadt <salvetore AT gmail.com> at 2012-05-02 03:44:33 UTC *** I'm looking forward to this.

*** Original post on bio 958 at 2012-05-02 09:45:24 UTC *** (In reply to comment #9) > > Should we insert a context message between them? > I don't think so, showing them as context (and in time bubble, it'll show the > time difference between...) should be enough information. Although when > rejoining a MUC this will probably be kind of strange as your "context" could > potentially have a big gap in it. I'm not sure that'll be enough, but I think what is being exposed here is a flaw in the current message display, not something that needs to be added "between conversations". Basically, what is missing at the moment is a message (of some sort) that a new day has begun. So this is probably a separate bug. Time bubbles already show the elapsed time between messages, one could simply enhance the JS to handle longer gaps in time better ("2 days" or "2 weeks" rather than thousands of hours ;)) But other message styles don't have this at all, and it might be nice anyway to also display the dates in this case. (e.g. something like "^^ Saturday 5-12-2011 vv Tuesday 1-5-2012"? Obviously finding a nice style for this won't be easy.)

*** Original post on bio 958 at 2012-05-02 09:52:19 UTC *** (In reply to comment #13) > Time bubbles already show the elapsed time between messages, one could simply > enhance the JS to handle longer gaps in time better ("2 days" or "2 weeks" > rather than thousands of hours ;)) "days" is already a supported time unit: http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/downloads/DownloadUtils.jsm#104 Otherwise I agree that we need to add a system message in the conversation when the day changes (maybe only when displaying the conversation, it doesn't seem useful to store in the logs). I think the difficult part is finding a way to format it correctly for the current locale. I thought we had a bug on this already, but I can't find it.

*** Original post on bio 958 as attmnt 1437 at 2012-05-04 11:20:00 UTC *** I moved the code to the logger and UIConversation, as suggested by flo and added a preference value for the amount of messages to show. I hope I got the IDL changes right (I can't test this). I added another parameter to tell the logger how many messages it should load, since it seemed to me that a preference for the number of context messages in a conversation isn't something for the logger to read. The conversation now reads this pref and requests as many messages as necessary. The discussions about system messages and date strings seems a bit unrelated, should we have a different bug for this? Known problems: * Can have duplicated messages (i.e. they are the messages array, but in the logs already too), I haven't figured out the details yet :( * Bubbles have wrong colours (from the logs, all my bubbles are blue while other people are red)

Comment on attachment 8353163 [details] [diff] [review] WIP *** Original change on bio 958 attmnt 1411 at 2012-05-04 11:20:40 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 958 at 2012-05-04 11:22:41 UTC *** (In reply to comment #15) > The discussions about system messages and date strings seems a bit unrelated, > should we have a different bug for this? Sure, that was kind of the point of my comment ;)

*** Original post on bio 958 at 2012-05-04 11:38:13 UTC *** (In reply to comment #15) > * Bubbles have wrong colours (from the logs, all my bubbles are blue while > other people are red) This is probably due to msg.conversation.isChat not being set correctly for these messages. Fixing this may depend on bug 954668 (bio 1236), or on setting msg.conversation correctly.

Comment on attachment 8353189 [details] [diff] [review] WIP 2 moved code to the logger and UIConversation *** Original change on bio 958 attmnt 1437 at 2012-05-04 22:08:42 UTC *** >diff -r ec1dde7b6d05 chat/components/public/imILogger.idl >@@ -84,4 +85,8 @@ > nsISimpleEnumerator getLogsForConversation(in prplIConversation aConversation); > nsISimpleEnumerator getSystemLogsForAccount(in imIAccount aAccount); > nsISimpleEnumerator getSimilarLogs(in imILog aLog); >+ void getLastNMessagesFromLogEnumerator(in nsISimpleEnumerator logEnumerator, I know I suggested it, but that's a really long method name. Would 'getLastMessages' be clear enough? A comment would be needed to explain what the parameters are then. >diff -r ec1dde7b6d05 chat/components/src/imConversations.js >@@ -54,6 +55,7 @@ > this.id = ++gLastUIConvId; > this._observers = []; > this._messages = []; >+ this._contextFromLogs = []; > this.changeTargetTo(aPurpleConversation); > let iface = Ci["prplIConv" + (aPurpleConversation.isChat ? "Chat" : "IM")]; > this._interfaces = this._interfaces.concat(iface); >@@ -160,10 +162,24 @@ > observer.observe(this, "unread-message-count-changed", > this._unreadIncomingMessageCount.toString()); > }, >+ _isContextFromLogsLoaded: false, >+ get contextMessagesFromLogs() { >+ if(!this._isContextFromLogsLoaded) { >+ let contextFromLogsCount = >+ Services.prefs.getIntPref("messenger.conversations.contextMessagesFromLogs"); >+ let convLogEnumerator = Services.logs.getLogsForConversation(this); >+ this._contextFromLogs = >+ Services.logs.getLastNMessagesFromLogEnumerator(convLogEnumerator, contextFromLogsCount); >+ this._isContextFromLogsLoaded = true; >+ } >+ return this._contextFromLogs; It seems you could simplify by giving _contextFromLogs a null default value in the prototype, and testing in this method if (!this._contextFromLogs) You wouldn't need _isContextFromLogsLoaded at all. > getMessages: function(aMessageCount) { >- if (aMessageCount) >- aMessageCount.value = this._messages.length; >- return this._messages; >+ if (aMessageCount) { >+ aMessageCount.value = this.contextMessagesFromLogs.length + >+ this._messages.length; >+ } >+ return this.contextMessagesFromLogs.concat(this._messages); I think it would be clearer to store what you are going to return in a variable, and then you would have only one .length. >diff -r ec1dde7b6d05 chat/components/src/logger.js >+ let logIndex = 0, >+ lastMsgs = []; Nit: I don't think we have frequently used this style; I think we would usually have one let per line, so maybe either add a let on the second line, or remove the line break? (I don't really mind if you want to change it though.) >+ while(logIndex < logs.length && lastMsgs.length <= aRequestedMessageCount) { >+ let logConv = logs[logIndex].getConversation(); >+ // This happens when the log's format is not JSON. >+ if(!logConv) >+ break; >+ lastMsgs = logConv.getMessages().concat(lastMsgs); >+ logIndex++; Coding style: ++logIndex And Patrick already said that we usually have a space between the keyword and ( for if, while, for, ... I'm really happy to see this moving forward! :-)

*** Original post on bio 958 as attmnt 1452 at 2012-05-07 11:31:00 UTC *** The logger now has a method to load N messages from before a given time in the past. The conversation passes the time of its first message to that method to avoid requesting messages from the current conversation's log file. I'll add comments on the IDL and check for coding style again before requesting review (and upload something new if needed). Is the approach OK in your opinion? Feedback on the IDL part would be great, I picked PRTime from above in this file and hope it's valid here too as it is getting used with similar numbers as for the time of messages and logs.

*** Original post on bio 958 at 2012-05-08 08:05:49 UTC *** I think it would be good to: * not count system messages (but do not overdo things by loading a thousand system messages (when someone only wanted ten context messages) just because there was no other activity in a channel), * tell when you were not in a MUC and might have missed messages. By showing a system message maybe: "You didn't attend this conversation between <time> and <time> and might have missed messages here.", "You did not attend this conversation for <period of time> and might have missed messages", ..?

*** Original post on bio 958 at 2012-05-09 09:43:10 UTC *** "<flo> I don't like the approach of comparing timestamps (as I assume them to be generally unreliable), but that may still be OK in this case" [1]. Does "may still be OK in this case" mean I should to push the fix forward with the current approach? I'm uncertain how to interpret the conversation that followed, about wrong timestamps and "what is it good for". I don't think I should care about possibly wrong timestamps here, by the way. I rather think we could/should put some sanity checks for the timestamps or system clock elsewhere (e.g. complain at startup if system time < time of last shutdown ?). [1] http://log.bezut.info/instantbird/120507/#m134

Comment on attachment 8353205 [details] [diff] [review] WIP3 *** Original change on bio 958 attmnt 1452 at 2012-05-16 21:47:10 UTC was without comment, so any subsequent comment numbers will be shifted ***

*** Original post on bio 958 at 2012-05-16 21:52:44 UTC *** (In reply to comment #20) > * not count system messages (but do not overdo things by loading a thousand > system messages (when someone only wanted ten context messages) just because > there was no other activity in a channel), This sounds like a good idea. But maybe if the previous 5 logs or so contain nothing but system messages, stop going back further. > * tell when you were not in a MUC and might have missed messages. By showing a > system message maybe: "You didn't attend this conversation between <time> and > <time> and might have missed messages here.", "You did not attend this > conversation for <period of time> and might have missed messages", ..? I don't think the period of time is very interesting (and anyway a message style like Bubbles will add it automatically). What is useful is knowing that you left the conversation and rejoined it, so that it is clear where there might be missing messages. So maybe something simple like "Conversation ended" and "Conversation joined"? (which will be timestamped automatically, though maybe one should include the date.)

*** Original post on bio 958 at 2012-05-16 22:07:27 UTC *** Regarding timestamps, I think in this case it is OK to use them, as the logs are sorted by timestamps anyway. The important thing would be to check that strange timestamps don't cause any errors (apart from possibly not showing the perfect context).

Comment on attachment 8353205 [details] [diff] [review] WIP3 *** Original change on bio 958 attmnt 1452 at 2012-05-28 17:27:58 UTC *** It seems like it would work fine (as long as we assume that if the system's clock is completely messed up it's the user's fault/problem). Two things that may need more work: - The getConversation call will block on disk I/O, do we need to add a way to limit how many files we will attempt to open? - How badly do things break if a file we try to open contains only unparsable junk? (failing to display previous messages if there's junk on the disk is OK; breaking the conversation to the point that displaying the current unread messages is impossible is not.)

*** Original post on bio 958 at 2012-06-02 18:55:35 UTC *** (In reply to comment #24) > Comment on attachment 8353205 [details] [diff] [review] (bio-attmnt 1452) [details] > WIP3 > > It seems like it would work fine (as long as we assume that if the system's > clock is completely messed up it's the user's fault/problem). > > Two things that may need more work: > > - The getConversation call will block on disk I/O, do we need to add a way to > limit how many files we will attempt to open? Most likely yes (see my comment about the logs with system messages). > - How badly do things break if a file we try to open contains only unparsable > junk? (failing to display previous messages if there's junk on the disk is OK; > breaking the conversation to the point that displaying the current unread > messages is impossible is not.) There are bugs blocking this one about handling broken logs now. I'll post my ideas on that there. I'd see to have it ready soon or are there concerns that it might be too risky so short before a release? At the moment I have e.g. no idea how it performs if someone has 10 auto-joined channels that all want to load their context messages at connect.

*** Original post on bio 958 at 2012-09-04 13:20:54 UTC *** Does it make sense to show the most recent messages no matter how long ago they were sent? Maybe it's a better idea to use messages from the last 2h, 12h, 24h, .. and limit the number in a similar way (e.g. up to 20 messages from the last 12 hours)?

*** Original post on bio 958 at 2012-09-04 14:18:29 UTC *** (In reply to comment #26) > Maybe it's a better idea to use messages from the last 2h, 12h, 24h, .. and > limit the number in a similar way (e.g. up to 20 messages from the last 12 > hours)? I think that's the kind of refinement of details we can play with when this is in nightlies.

*** Original post on bio 958 by Kissaki <kissaki0 AT googlemail.com> at 2012-09-07 15:58:10 UTC *** (In reply to comment #26) > Does it make sense to show the most recent messages no matter how long ago they > were sent? > > Maybe it's a better idea to use messages from the last 2h, 12h, 24h, .. and > limit the number in a similar way (e.g. up to 20 messages from the last 12 > hours)? No. I would want it there very time. Even if it is just for being remembered, when I last talked with that person and about what. It may not be important what you talked about, but it most certainly is *when*. Definitely if you don't quite remember the person.

*** Original post on bio 958 at 2012-11-05 23:34:42 UTC *** Realistically, this isn't going to be ready for 1.3, moving to 1.4-wanted.

*** Original post on bio 958 by Michael de Raadt <salvetore AT gmail.com> at 2013-04-10 00:18:15 UTC *** What does this last status change mean? Are you not planning to work on this feature any more?

*** Original post on bio 958 at 2013-04-10 00:58:56 UTC *** (In reply to comment #30) > What does this last status change mean? Are you not planning to work on this > feature any more? I'm not sure what you mean, do you mean comment 29? It just means that it won't ready soon. We definitely want this though.

*** Original post on bio 958 by Michael de Raadt <salvetore AT gmail.com> at 2013-04-10 01:43:10 UTC *** I received an email stating that the "Status whiteboard" had removed "[1.4-wanted]" and nothing had been added. What |Removed |Added ---------------------------------------------------------------------------- Status Whiteboard|[1.4-wanted] |

*** Original post on bio 958 at 2013-04-10 02:19:15 UTC *** Yes, that doesn't really mean anything. It just means that we were hoping to do this for 1.4, but it isn't going to happen for 1.4 since we're hoping to release 1.4 in a few weeks.

*** Original post on bio 958 by Michael de Raadt <salvetore AT gmail.com> at 2013-04-10 02:31:53 UTC *** OK. No worries. Florian: I hope to see you at the GSoC Mentor Summit.

*** Original post on bio 958 at 2013-04-10 09:38:17 UTC *** (In reply to comment #34) > Florian: I hope to see you at the GSoC Mentor Summit. Mozilla has lots of GSoC mentors, so the chances of me going again to the Summit soon are small ;).

What's the latest status on this feature request? Sure would love to have it implemented!

Mass-removing myself from being assignee of any Chat related bug as I'm no longer active.