Initial status message condensing #759

AlMcKinlay · 2016-11-23T17:06:27Z

This probably isn't done, and needs a lot more testing from other people to see if I've done anything bad.

Another good thing to add would be how IRC cloud shows it (it makes a little sentence explaining some of the parts/joins in it).

But this is just an initial one.

List of stuff still to do before merging:

MaxLeiter · 2016-11-23T17:17:20Z

Is it not possible to do something more similar to IRCCloud, where we just combine all the parts and joins into a line or two.

I do kind of like the button, however I see it being difficult on mobile. I also think the text could be better (we shouldn't use etc)

xPaw · 2016-11-23T17:25:13Z

client/js/lounge.js

@@ -276,9 +278,27 @@ $(function() {
 		return msg;
 	}

-	function buildChannelMessages(channel, messages) {
+	function appendMessage(container, chan, type, msg) {
+		if (!$(msg).hasClass("message") && type !== "lobby") {


Topic changes and other stuff? I think whitelist is better suited here?

Hmmm, I guess. I guess it depends what we want to condense. I was thinking of condensing everything, but I guess people might prefer just condensing parts, joins, quits. What about kicks?

Hmm yeah, maybe multiple mode changes and kicks be a good idea to condense too. But whitelist is still safer in this case. So we don't unknowingly break anything if we add new features.

Aight, I've done joins, parts, quits, modes, kicks and nick changes.

xPaw · 2016-11-23T17:25:37Z

client/js/lounge.js

@@ -292,7 +312,7 @@ $(function() {
 	}

 	function renderChannelMessages(data) {
-		var documentFragment = buildChannelMessages(data.id, data.messages);
+		var documentFragment = buildChannelMessages(data.id, data.messages, data.type);


Just pass data

AlMcKinlay · 2016-11-23T17:52:26Z

Is it not possible to do something more similar to IRCCloud, where we just combine all the parts and joins into a like or two?

That's what I meant when I talked about irc cloud in the pr. It's a bit
more complex, but would be a nice next step. This is the first step. Fwiw,
you can expand the irc cloud one as well, so it is just a first step
towards replicating that feature.

MaxLeiter · 2016-11-23T18:00:26Z

Can we remove the button and have the actual text be the button? I think that's what IRCCloud does

spangborn · 2016-11-23T18:01:47Z

I think it might be better to change the text to be a summary of what's condensed, something like

Foo and bar left the channel.

And then when you open it up, you can see the timestamps and quit messages, or whatever detail gets tucked away.

AlMcKinlay · 2016-11-23T18:11:29Z

I think it might be better to change the text to be a summary of what's condensed

@spangborn As I've already said, that is a good next step, but I think this is a useful first step before it. I won't have the time to add that just now, and this is better than nothing.

Can we remove the button and have the actual text be the button? I think that's what IRCCloud does

@MaxLeiter Happy to make the text clickable as well, but I don't want to remove the button. The button is a key indicator that you can expand it.

AlMcKinlay · 2016-11-24T08:39:05Z

Alright, I think I've updated it with people's feedback (except the natural language explanation of what's inside the condensed bit).

I think we need to remember that it's good to make incremental improvements, and as @astorije has said a lot, we don't want to wait until things have all of the features that we want to put them in.

Adding the natural "x, y and z joined, a and b left" will be a good next step, and I will take some time to work that out, but that has multiple issues:

We need to have a discussion as to how we want to word it, and that's going to end up very subjective, so would bog down this PR for aaaages
It's a bit more expensive computationally than this, because we'll need to re-write the wording everytime we get a new one
It's just more complicated in general.

So yes, that will come. But I want to do that separately, once this one is done.

But yeah, the best thing is if some people can run this and see if there are any bugs I've missed. It seems to work quite well so far.

I've removed "etc." from the wording, because that's a bit awkward. If anyone has a better idea for how we should word it for this PR (before we do the natural language explanation), that would be great.

xPaw · 2016-11-24T08:42:47Z

client/js/lounge.js

-			docFragment.append(buildChatMessage({
-				chan: channel,
+	function appendMessage(container, chan, type, msg) {
+		if ($(msg).is(".join, .part, .quit, .mode, .kick, .nick") && type !== "lobby") {


Compare type, you don't need to use DOM.

Type here is actually the type of channel. But I can check against the message.

Ah wait, the message we get through is the dom element anyway. We don't get the information of the message without it being an element.

xPaw · 2016-11-24T08:44:27Z

client/views/msg_condensed.tpl

+<div class="msg {{type}}{{#if self}} self{{/if}}{{#if highlight}} highlight{{/if}} closed" id="msg-{{id}}">
+	<span class="time"></span>
+	<span class="from"><span class="expander"></span></span>
+	<span class="text">Condensed joins/parts</span>


You can count all the unique events and display like 5x join, 1x quit, 2x mode. As of right now youre not saying correctly which events were condensed.

Would you be happy with me just saying "Condensed joins/parts/mode/nick changes" for this PR?

No, that's just wrong. Print actual events (even without counter)

Print actual events (even without counter)

What do you mean?

Find out which events are in this condensed message and print these.

Oh. Meh, that's more work. But fine, I'll do it.

AlMcKinlay · 2016-11-24T09:23:50Z

Alright, I've added a simple "1 join, 1 part" as the message, so that we at least have some sort of information.

I still think it's a good idea to do a natural language one at some point, but still not this PR, because we need to figure out what the wording should be.

AlMcKinlay · 2016-11-24T09:33:49Z

So, current text will show (if they are all there) "1 join, 1 part, 1 quit, 1 mode change, 1 nick change and 1 kick". They will all pluralise if there are multiple, and if there are 0, it'll just not show at all. Think this is a good one for just now?

AlMcKinlay · 2016-11-24T16:01:19Z

Alrighty, I have a bug now, which is fun. It doesn't work perfectly with the date separator. When there is a part/join straight after a date separator, the expanded elements show as below the date, but the condenser element shows as behind it. The condenser should show after the date separator (unless part of it is in the past, I guess?)

xPaw · 2016-12-17T08:20:24Z

Trying this PR out. I think it's pointless to condense messages if it only condenses one message.

It also condensed topic_set_by but didn't say it did.

AlMcKinlay · 2016-12-17T10:30:50Z

think it's pointless to condense messages if it only condenses one message.

My reason for doing this was to make it consistent (it felt a bit odd having it look different when there was only 1) but I can change it.

It also condensed topic_set_by but didn't say it did.

Good point, I'll remove it.

xPaw · 2016-12-17T10:32:03Z

My reason for doing this was to make it consistent (it felt a bit odd having it look different when there was only 1) but I can change it.

Ideally we would do something similar to IRCCloud.

AlMcKinlay · 2016-12-17T10:46:50Z

Ideally we would do something similar to IRCCloud.

Yeah,that's the ultimate plan. But happy to remove the condensing for 1 thing before that happens.

I'll hopefully get these bugs fixed this week.

astorije · 2016-12-17T18:57:50Z

This is really nice on busy channels! Good job 👏
I'm on #ansible and #freenode, it's the wild west in there 😅.

I do agree with not condensing less than 2 items (I'd even say 3 or 5 if it were me). If the goal is to reduce noise on busy channels, then channels with very little activity should would not benefit from this much.

In terms of messages, I think we should only condense joins, parts and quits together, at least at first. I don't think topic changes, mode changes or other kind of messages are as noisy as those 3. (Actually, camping in those 2 channels mentioned above),

I'm happy to re-evaluate the need to condense nicks and modes, or others, but starting with joins/parts/quits is a strong improvement already and lets us experiment how it behaves that way.

One bug though: When unfolding the messages, all messages are now clickable. I agree that the summary should be clickable (though when we start listing nicks in the summary, there will be a choice to make), but not everything it's condensing IMO.

In terms of icons, can you right and down carets at the message level (where it's clickable) to give it a dropdown purpose?

Folded	Unfolded

AlMcKinlay · 2016-12-19T21:33:13Z

Alrighty.

I have done all of the things people asked (I'm not sure about not condensing modes and nick changes, but I'll see how it goes for a while).

There is 1 definite bug left, and 1 possible bug left (I need to test that overnight).

Feel free to retry guys.

AlMcKinlay · 2016-12-20T08:03:09Z

Having used this for only a few hours, it almost defeats the purpose if we don't include mode changes and nick changes.

It breaks up the condensing so much, that we don't save a lot of space. So I'm going to add it back in.

AlMcKinlay · 2016-12-20T08:33:53Z

Example of what it looks like in a quiet channel without mode/nick changes condensed. IT just looks messy, IMO.

AlMcKinlay · 2016-12-21T19:53:54Z

So, I added nick changes and mode changes back in, and I fixed the bugs. I think that's everything we had wrong with it. So, it should be ready for proper review.

astorije · 2017-08-06T17:08:22Z

client/js/renderPreview.js

@@ -47,7 +47,7 @@ function renderPreview(preview, msg) {
 	container.trigger("keepToBottom");
 }

-$("#chat").on("click", ".toggle-button", function() {
+$("#chat").on("click", ".text .toggle-button", function() {


This makes sure that the thumbnail toggle handler does not react to condensed toggle clicks. It is possible because text class is now absent from the condensed template.

astorije · 2017-08-06T17:09:28Z

client/themes/morning.css

 #chat .toggle-content {
 	background: #242a33;
+	color: #f3f3f3;
 }


All this makes the toggle button the same color than neighboring text. It was silly of me to color it differently, and it looked really bad on the condensed toggle.

astorije · 2017-08-06T17:10:10Z

client/views/msg_condensed.tpl

 	<span class="content">
-		<span class="text condensed-msg"></span>
+		<span class="condensed-text"></span>


Removed text class here to differentiate normal messages from condensed messages. See above for a further explanation.

xPaw · 2017-08-08T06:53:33Z

client/js/condensed.js

@@ -33,5 +34,6 @@ function updateText(condensed, addedTypes) {
 			text += obj[messageType] > 1 ? "s" : "";
 		}
 	}
-	condensed.find(".condensed-msg").text(text);
+	condensed.find(".condensed-text")
+		.html(text + templates.msg_condensed_toggle());


@astorije So the potential issue I see with this, is that unlike previews, condensed text can change and thus move the button. A case can happen when you want to click the button, and someone leaves the channel at that moment and the button moves as a result of increased text.

Unless the entire line is clickable, which makes my comment irrelevant.

Unless the entire line is clickable

Yep, entire line of text, not just the toggle button ;-)

So bounding box would roughly be:

(I quickly edited this with Mac's preview, not set background on actual link, so picture may differ from actual product 😅)

xPaw · 2017-08-09T10:07:23Z

@astorije the default state for the setting appears to be nothing.

astorije · 2017-08-10T05:16:41Z

Ah, missed that @xPaw, it shouldn't as the default is supposed to be "shown" (i.e. expanded), but I'll look into it.
While I'm here, should the default be expanded or collapsed? I'd actually prefer collapsed, and I think that's what @YaManicKill did, so my commit might have canceled that. If that's okay by you, I'll put back to "collapsed" as default.
Also, if collapsed by default, should I switch the 2 options in the UI? I like the idea of order being "everything"/"less things"/"nothing", but I also think that defaulting to something else than first option is meh. I know I'm grasping at straws though 😅

fnutt · 2017-08-10T11:26:48Z

While I'm here, should the default be expanded or collapsed? I'd actually prefer collapsed, and I think that's what @YaManicKill did, so my commit might have canceled that. If that's okay by you, I'll put back to "collapsed" as default.

I think default should be collapsed. This will highlight the future (not everyone will click on settings and change this because they don't know of it), as well as this is probably what most people would like anyway.

Also, if collapsed by default, should I switch the 2 options in the UI? I like the idea of order being "everything"/"less things"/"nothing", but I also think that defaulting to something else than first option is meh. I know I'm grasping at straws though 😅

I agree about having "everything"/"less things"/"nothing". What I don't agree about is the lack of informational text. Can we put "joins/parts/quits/nick changes" somewhere to make it very easy to see what the settings are for? Please mind I have not seen what the tooltip on ? says.

astorije · 2017-08-10T13:32:17Z

Hey @christer88, thanks for your feedback! 😊

I think default should be collapsed.

Cool, your explanation makes sense!

I agree about having "everything"/"less things"/"nothing".

👍

What I don't agree about is the lack of informational text. Can we put "joins/parts/quits/nick changes" somewhere to make it very easy to see what the settings are for? Please mind I have not seen what the tooltip on ? says.

It turns out, this is exactly what this ❔ tooltip is for :)

In practice, this is exactly the text that @YaManicKill put in a title attribute on hover, except that this is on an instant tooltip, both "hoverable" and clickable so that mobiles can display it. Same thing, just a bit more reachable by the user!

astorije · 2017-08-13T19:35:49Z

@xPaw, fixed that issue with default option, as well as another one.
Also (/Cc @christer88), I (re-)made condensed the default.

Finally, did the last thing I wanted to do, i.e. made the strings friendlier and closer to what we have on expanded view:

It's not the cleanest code for that, but it will be amended by #1271 and/or #1047 anyway. Also, I did clean up this file a little bit to balance the karma 😅

Next step for me is to do a full review again, will do that shortly.

…ettings

… (with transition), reuse thumbnail toggle style

astorije

I'm not a huge fan of implementation and I suspect things will go wrong in the near future (@xPaw mentioned that #1041 is going to be a pain now), but feature itself is 👌 and I'm sure we can improve over time.

Huge 💯 to @YaManicKill for making this a reality!

My last issue with this is that selecting the option in the settings does not do anything until restarted. Solutions involve re-rendering, probably very expensive, or refreshing the page, which would be a poor experience until #1283 gets done.
I won't be blocking because of this context, but will definitely open a issue for it after this PR gets merged :)

@xPaw, with the recent changes I have made, do you want to give it another try before we merge?

xPaw · 2017-08-14T08:18:48Z

Whatever, let's merge this and do work on top of master. This has been stalled for too long.

AlMcKinlay · 2017-08-14T08:22:46Z

Wait, this is actually finally merged? Holy crap. I'm so happy. No more rebasing.

…joins Initial part/join condensing

xPaw reviewed Nov 23, 2016

View reviewed changes

AlMcKinlay force-pushed the yamanickill/condense-joins branch from 3d85a96 to 76c8c76 Compare November 24, 2016 08:36

AlMcKinlay added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Nov 24, 2016

xPaw reviewed Nov 24, 2016

View reviewed changes

AlMcKinlay force-pushed the yamanickill/condense-joins branch 2 times, most recently from 770bac9 to 9d59cde Compare November 24, 2016 09:33

AlMcKinlay force-pushed the yamanickill/condense-joins branch 2 times, most recently from 68f446d to 2d5cfd4 Compare November 24, 2016 15:58

AlMcKinlay added the Meta: Do Not Merge This PR should not be merged. label Nov 24, 2016

AlMcKinlay force-pushed the yamanickill/condense-joins branch from 2d5cfd4 to 684ca37 Compare December 19, 2016 21:32

AlMcKinlay force-pushed the yamanickill/condense-joins branch from 684ca37 to 2fe7280 Compare December 20, 2016 06:37

astorije reviewed Aug 6, 2017

View reviewed changes

xPaw reviewed Aug 8, 2017

View reviewed changes

astorije mentioned this pull request Aug 13, 2017

More contrast for text colors #1383

Closed

astorije force-pushed the yamanickill/condense-joins branch from 9f18c36 to 3a1db41 Compare August 13, 2017 18:05

Condense Joins

499dfdf

astorije force-pushed the yamanickill/condense-joins branch 2 times, most recently from b3f634f to 2ccac7b Compare August 13, 2017 19:28

astorije added 4 commits August 13, 2017 16:14

Replace status message abbr with a tooltipped icon

0cdc2a0

Improve UI of expanded/condensed status message selection in client s…

b508783

…ettings

Move condense caret to the right, animate transition, add hover state…

853e646

… (with transition), reuse thumbnail toggle style

Add some human-friendliness to the condensed status messages

43a8604

astorije force-pushed the yamanickill/condense-joins branch from 2ccac7b to 43a8604 Compare August 13, 2017 20:17

astorije approved these changes Aug 14, 2017

View reviewed changes

xPaw merged commit 28e32dc into master Aug 14, 2017

xPaw deleted the yamanickill/condense-joins branch August 14, 2017 08:21

astorije mentioned this pull request Aug 23, 2017

Use away-notify to show updates on users away state #845

Merged

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Merge pull request thelounge#759 from thelounge/yamanickill/condense-…

c14baff

…joins Initial part/join condensing

astorije changed the title ~~Initial part/join condensing~~ Initial status message condensing Sep 29, 2017

AlMcKinlay removed their assignment Mar 12, 2018

xPaw removed their assignment Mar 12, 2018

fnutt mentioned this pull request Mar 1, 2021

Ignore messages by type #4166

Open

Initial status message condensing #759

Initial status message condensing #759

Conversation

AlMcKinlay commented Nov 23, 2016 • edited by astorije Loading

MaxLeiter commented Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlMcKinlay commented Nov 23, 2016 • edited Loading

MaxLeiter commented Nov 23, 2016

spangborn commented Nov 23, 2016

AlMcKinlay commented Nov 23, 2016

AlMcKinlay commented Nov 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlMcKinlay commented Nov 24, 2016

AlMcKinlay commented Nov 24, 2016

AlMcKinlay commented Nov 24, 2016

xPaw commented Dec 17, 2016

AlMcKinlay commented Dec 17, 2016

xPaw commented Dec 17, 2016

AlMcKinlay commented Dec 17, 2016

astorije commented Dec 17, 2016

AlMcKinlay commented Dec 19, 2016

AlMcKinlay commented Dec 20, 2016

AlMcKinlay commented Dec 20, 2016

AlMcKinlay commented Dec 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

astorije Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw commented Aug 9, 2017

astorije commented Aug 10, 2017

fnutt commented Aug 10, 2017

astorije commented Aug 10, 2017

astorije commented Aug 13, 2017

astorije left a comment

Choose a reason for hiding this comment

xPaw commented Aug 14, 2017

AlMcKinlay commented Aug 14, 2017

AlMcKinlay commented Nov 23, 2016 •

edited by astorije

Loading

MaxLeiter commented Nov 23, 2016 •

edited

Loading

AlMcKinlay commented Nov 23, 2016 •

edited

Loading

xPaw Aug 8, 2017 •

edited

Loading

astorije Aug 9, 2017 •

edited

Loading