New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Sep 17, 2017

Conversation

Projects
5 participants
@MaxLeiter
Member

MaxLeiter commented Dec 28, 2016

Probably need better wording for the settings option. Handles duplicate states (i.e. if someone goes away multiple times in a row it will only be shown once).

image

is now away
<i class="away-message">({{{parse text}}})</i>
{{else}}
is back

This comment has been minimized.

@MaxLeiter

MaxLeiter Dec 28, 2016

Member

is now back?

<div class="col-sm-6">
<label class="opt">
<input type="checkbox" name="away">
Show aways/backs

This comment has been minimized.

@MaxLeiter

MaxLeiter Dec 28, 2016

Member

Must be a better way to word this

This comment has been minimized.

@dgw

dgw Dec 28, 2016

Contributor

Ideas: "Show when users go away", "Show away notifications", "Notify on user presence changes", "Show user presence changes".

@@ -16,4 +17,5 @@ function User(attr, prefixLookup) {
// TODO: Remove this
this.name = this.nick;
this.mode = (this.modes && this.modes[0]) || "";
this.away = this.away;

This comment has been minimized.

@MaxLeiter

MaxLeiter Dec 28, 2016

Member

Not necessary, just added for uniformity... should remove

@dgw

👍
Tried to help with wording in line notes.

<div class="col-sm-6">
<label class="opt">
<input type="checkbox" name="away">
Show aways/backs

This comment has been minimized.

@dgw

dgw Dec 28, 2016

Contributor

Ideas: "Show when users go away", "Show away notifications", "Notify on user presence changes", "Show user presence changes".

is now away
<i class="away-message">({{{parse text}}})</i>
{{else}}
is now back

This comment has been minimized.

@dgw

dgw Dec 28, 2016

Contributor

I'm partial to wording it as "went away" and "came back", but whatever.

@astorije

This comment has been minimized.

Member

astorije commented Jan 4, 2017

No matter what I try, multiple clients and users running this PR on 3 different IRC servers (including Freenode and the demo server), I cannot get to have anything showing up. Using scripts/run-pr.sh so the webpacking is done correctly.
Any idea? Any chance this PR could be currently broken? :/


EDIT: Of course, I enabled the checkbox lol...

@astorije

This is a great addition, but sadly I can't test it 😅

Left a few minor changes. Also not a fan of the duplicate management. We don't do this for nick changes, joins/parts, etc. so I would not do it here either, and rely either on #759 or when we "hide" messages on the backend (instead of just CSS).

<div class="col-sm-6">
<label class="opt">
<input type="checkbox" name="away">
Show when users go away

This comment has been minimized.

@astorije

astorije Jan 4, 2017

Member

As opposed to @dgw's opinion, I think we should go with consistency, therefore something like Show away/back changes or the likes. It's not pretty, but it sticks out compared with the other options right now.

<i class="away-message">({{{parse text}}})</i>
{{else}}
came back
{{/if}}

This comment has been minimized.

@astorije

astorije Jan 4, 2017

Member

/nick shows present tense (x is now known as y) so we could just go with x is away / x is back for consistency.

Also, I believe on The Lounge /away with no reason sets an away message of (one whitespace) so I think the message should now show up at all if trimmed text is empty.

This comment has been minimized.

@MaxLeiter

MaxLeiter Jan 5, 2017

Member

Is there an easy way to tell what 'text' contains?

This comment has been minimized.

@astorije

astorije Jan 8, 2017

Member

I think you would have to define a helper for that which would do a stupid check like text.trim().length === 0. Maybe @xPaw would have a better idea?

This comment has been minimized.

@xPaw

xPaw Jan 8, 2017

Member

Do we trim other messages like kicks? If not, don't do it here.

This comment has been minimized.

@astorije

astorije Jan 8, 2017

Member

I'm not saying we should trim text. I'm saying the check to know if there is a text should take this into account. This is because when using The Lounge, sending /away will send a " " reason so anyone else using The Lounge would see nick is now away ( ).
In such situation, it should not display the parens.

"use strict";
let _ = require("lodash");
let Msg = require("../../models/msg");

This comment has been minimized.

@astorije

astorije Jan 4, 2017

Member

const instead.

module.exports = function(irc, network) {
let client = this;
irc.on("away", function(data) {

This comment has been minimized.

@astorije

astorije Jan 4, 2017

Member

Arrow function? :)

@astorije astorije added this to the 2.3.0 milestone Jan 4, 2017

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Jan 4, 2017

No idea why not working, will investigate. Regarding duplicate messages, people can't change their nick to the same nick, part twice, join twice, etc (without actions in between)

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Jan 9, 2017

Addressed all changes except the empty parenthesis if no away message is supplied (@astorije)

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Jan 20, 2017

@astorije even if these messages condensed, I still don't see a reason to show multiple away's from the same user - how about they're hidden if the message is the same, and shown if not?

@astorije

This comment has been minimized.

Member

astorije commented Feb 13, 2017

Testing this on testnet.inspircd.org using 2 accounts, I cannot see status change when using /away with no argument or when using /back.
Using /away foobar does show astorije is away (foobar) correctly however.

@astorije astorije added this to IRC Version 3.1 TODO in IRCv3 Feb 19, 2017

@astorije

This comment has been minimized.

Member

astorije commented Mar 28, 2017

@MaxLeiter, did you see my last message?

@dgw

This comment has been minimized.

Contributor

dgw commented Apr 25, 2017

That's a mess of commits…

@McInkay

This comment has been minimized.

Member

McInkay commented Apr 26, 2017

So, what's the current state of this? Is it still not working for @astorije entirely?

@astorije

This comment has been minimized.

Member

astorije commented Apr 26, 2017

Right, it wouldn't work at all on Freenode (but I believe this is normal as not supported there) and not working properly on that other network.
@MaxLeiter, do you think you can/want to work on this before v2.3.0 or should we remove it from the milestone? :(

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Apr 26, 2017

Ill get it done

@xPaw xPaw removed this from the 2.3.0 milestone Apr 27, 2017

@astorije astorije added this to the 2.4.0 milestone May 8, 2017

@astorije

This comment has been minimized.

Member

astorije commented Jun 12, 2017

@MaxLeiter, ping on this? Would be really nice to put this behind us!

@astorije

This comment has been minimized.

Member

astorije commented Jun 27, 2017

@MaxLeiter, any chance you could fix conflicts / wrap up this PR? :)

const Msg = require("../../models/msg");
module.exports = function(irc, network) {
let client = this;

This comment has been minimized.

@astorije

astorije Jun 27, 2017

Member

To fix ESLint, use const here.

away = true;
}
network.channels.forEach((chan, index) => {
let user = _.find(chan.users, {name: data.nick});

This comment has been minimized.

@astorije

astorije Jun 27, 2017

Member

To fix ESLint, use const here.

return;
}
let msg = new Msg({

This comment has been minimized.

@astorije

astorije Jun 27, 2017

Member

To fix ESLint, use const here.

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Jul 3, 2017

@xPaw if you could take a look at this that'd be great -- I'm lost. Basically, the empty /away command should emit /away <space>, however -somewhere- that " " is being stripped so it's being received by the IRC server as /away, which is really /back.

@xPaw

This comment has been minimized.

Member

xPaw commented Jul 3, 2017

@MaxLeiter are you talking about you sending /away not receiving it? If so, why is that a problem for you in this particular PR?

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Jul 3, 2017

I guess so, @xPaw, because for some reason /away no longer works with this PR.

});
chan.pushMessage(client, msg);
if (index === network.channels.length - 1) { // Last iteration, let's update the users status

This comment has been minimized.

@xPaw

xPaw Jul 10, 2017

Member

User object is a child of channel, you have to update it for every channel.

@McInkay

This comment has been minimized.

Member

McInkay commented Aug 23, 2017

The conflicts don't look that difficult, so if @MaxLeiter doesn't have time to do that, I'd be happy to fix the conflicts.

@astorije astorije assigned McInkay and unassigned MaxLeiter Aug 25, 2017

@astorije

This comment has been minimized.

Member

astorije commented Aug 25, 2017

@YaManicKill, @MaxLeiter mentioned he wouldn't be able to take this up, so I assigned you to it, whenever you have a moment :)

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Sep 8, 2017

Rebased + updated to condense. Ready for review
image

Recommend testing on testnet.inspircd.org

@astorije

This is really cool and works well! Too bad Freenode doesn't support this!

Only thing (which we don't control since it's on the IRC server side of things, I think): message doesn't show up when user marks themselves as away/back (as opposed to a nick change for example). Is this expected from the spec? Is it something we can do but don't at the moment?

Either way, I'm all up for it. Because code has changed quite a bit since you got @YaManicKill's approval, I'll let him or @xPaw give a review and merge.

👏

@astorije astorije requested a review from McInkay Sep 8, 2017

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Sep 8, 2017

Don't think we can control that, @astorije. I could make it show up but it wouldn't be too pretty -- I don't think it's necessary.

@astorije

This comment has been minimized.

Member

astorije commented Sep 8, 2017

I think setting yourself as away/back should show, but since it would have nothing to do with away-notify, don't worry about it on this PR.

Glad we're actually going to merge this thing!! 😅

@astorije astorije requested a review from xPaw Sep 15, 2017

@astorije

This comment has been minimized.

Member

astorije commented Sep 15, 2017

@YaManicKill or @xPaw, could one of you review this? :)

@xPaw

This comment has been minimized.

Member

xPaw commented Sep 15, 2017

@MaxLeiter Can you also mention away messages in help tooltip in the setting for status messages?

@xPaw

xPaw approved these changes Sep 15, 2017

@xPaw xPaw removed their assignment Sep 15, 2017

@astorije

This comment has been minimized.

Member

astorije commented Sep 16, 2017

@MaxLeiter, could you squash?

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Sep 16, 2017

Done

text: away || "",
time: data.time,
from: data.nick,
mode: chan.getMode(data.nick)

This comment has been minimized.

@xPaw

xPaw Sep 16, 2017

Member

You can do user.mode here, as you've already found the user.

@astorije astorije removed the request for review from McInkay Sep 17, 2017

@astorije astorije assigned MaxLeiter and unassigned McInkay Sep 17, 2017

@astorije

This comment has been minimized.

Member

astorije commented Sep 17, 2017

Will merge once you address @xPaw's suggestion :)

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Sep 17, 2017

Done

@astorije astorije merged commit 90cb79a into thelounge:master Sep 17, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@astorije astorije moved this from IRC Version 3.1 TODO to IRC Version 3.1 Completed in IRCv3 Sep 17, 2017

@MaxLeiter MaxLeiter deleted the MaxLeiter:MaxLeiter/away-notify branch Sep 19, 2017

@astorije astorije changed the title from Use away-notify to show updates on users away state to Use `away-notify` to show updates on users away state Sep 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment