Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Auto away when no clients are connected #775

Merged
merged 1 commit into from
Mar 31, 2017
Merged

Auto away when no clients are connected #775

merged 1 commit into from
Mar 31, 2017

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Dec 3, 2016 •
edited
Loading

2 issues:

  • Setting away message to empty should disable auto-away (and disable by default?)
  • Should keep in mind away message set by using /away command. However this is not blocking issue for this PR.

Auto away is useful for people that use lounge along with Znc as it has checks for away clients.

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Dec 3, 2016
xPaw
xPaw commented Dec 4, 2016
View reviewed changes
src/client.js Outdated

if (_.size(this.attachedClients) === 0) {
this.networks.forEach(function(network) {
network.irc.raw("AWAY", this.awayMessage);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be client from upper scope.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand MDN's article on arrow functions, this would come for free if forEach was using one of them. I haven't used them much, so I might be wrong.

@astorije
Copy link
Member

astorije commented Dec 5, 2016

Setting away message to empty should disable auto-away (and disable by default?)

What do you mean by this?

Shouldn't this come with an entry in the client settings? We probably don't want to force everyone to be away when no client is connected :)

astorije
astorije requested changes Dec 5, 2016
View reviewed changes
src/client.js Outdated
@@ -63,6 +63,7 @@ function Client(manager, name, config) {
config = {};
}
_.merge(this, {
awayMessage: "no users in the lounge",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that this branch is leaving the status of patch, we should probably have a better default. Actually, empty by default since it will be disabled by default.

src/client.js Outdated
@@ -457,11 +458,23 @@ Client.prototype.quit = function() {
};

Client.prototype.clientAttach = function(socketId) {
if (_.size(this.attachedClients) === 0) {
this.networks.forEach(function(network) {
network.irc.raw("AWAY");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xPaw, I'm seeing this and wondering: could this PR be improved once #745 is merged?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really related directly.

src/client.js Outdated

if (_.size(this.attachedClients) === 0) {
this.networks.forEach(function(network) {
network.irc.raw("AWAY", this.awayMessage);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand MDN's article on arrow functions, this would come for free if forEach was using one of them. I haven't used them much, so I might be wrong.

src/plugins/irc-events/connection.js Outdated
@@ -20,6 +21,10 @@ module.exports = function(irc, network) {
}), true);
}

if (_.size(client.attachedClients) === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to use Map for client.attachedClients?
I noticed we use it like this in a couple places, and we could use client.attachedClients.size directly (actually, this is one of the arguments of Object vs. Map used in the MDN article!).

@xPaw xPaw force-pushed the patches/auto-away branch 3 times, most recently from 118bb90 to 58eff93 Compare December 9, 2016 21:12
maxpoulin64
maxpoulin64 reviewed Dec 10, 2016
View reviewed changes
Copy link
Member

@maxpoulin64 maxpoulin64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine to me, but I'm a bit unsure as if this should be directly tied to the away message. I could very well set myself as away leaving my computer on and everything. If my connection happens to drop or anything, then I'd be marked back. I think this one should be a checkbox or field in the settings and be independent of the away message.

@xPaw
Copy link
Member Author

xPaw commented Dec 11, 2016

@maxpoulin64 See the second todo in main post. I wanted to incorporate your /away message into it.

@xPaw xPaw added this to the 2.3.0 milestone Dec 17, 2016
@xPaw
Copy link
Member Author

xPaw commented Dec 18, 2016

Updated the PR to also account for network /away messages, which aren't overriden by default auto away.

@astorije
Copy link
Member

Just curious, if we merge this, /away state will be automatically disabled as soon as user is connected to The Lounge?

@xPaw
Copy link
Member Author

xPaw commented Dec 18, 2016 •
edited
Loading

@astorije Nope, there's code to prevent that. Look closely for client.awayMessage vs network.awayMessage

@ghost
Copy link

ghost commented Jan 28, 2017

Any updates on getting this merged into master?

@AlMcKinlay
Copy link
Member

@MuffinMedic We are currently doing final checks on version 2.2.0, and after that we will start looking at this again.

@astorije
Copy link
Member

@xPaw, can we have a front-end checkbox for this? We already do server-synced stuff with custom highlights so we should be able to do that for this one too.
And I know #275 was addressing that sort of things but I don't think this is going to be worked on any time soon, while I would love to see your PR working end-to-end (i.e. not having to edit the config file on the server to enable this!).

@astorije
Copy link
Member

FYI, rebased this on master and got rid of the merge conflict.

@xPaw
Copy link
Member Author

xPaw commented Mar 29, 2017

We already do server-synced stuff with custom highlights so we should be able to do that for this one too.

We'd don't though?

@astorije
Copy link
Member

We'd don't though?

Well I'll be damned, I thought so but I see this is client-side only! I guess this is good to go then, but I wish there was a way to do settings better here. Quite the undertaking, but that would be such a great enhancement to solve globally.

@xPaw
Copy link
Member Author

xPaw commented Mar 29, 2017

Settings branch exists, but it needs major work.

@xPaw xPaw merged commit 332bbad into master Mar 31, 2017
@xPaw xPaw deleted the patches/auto-away branch March 31, 2017 16:15
@xPaw
Copy link
Member Author

xPaw commented Mar 31, 2017

🎉

@xPaw xPaw mentioned this pull request Apr 15, 2017
Closed
@AlMcKinlay AlMcKinlay mentioned this pull request Apr 17, 2017
Closed
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
@xPaw
Merge pull request thelounge#775 from thelounge/patches/auto-away
efb9cd3 
Auto away when no clients are connected
@Jay2k1 Jay2k1 mentioned this pull request Jan 22, 2018
Closed
@AlMcKinlay AlMcKinlay removed their assignment Mar 12, 2018
@ghost ghost mentioned this pull request Apr 11, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxpoulin64 maxpoulin64 maxpoulin64 left review comments

@astorije astorije astorije approved these changes

@AlMcKinlay AlMcKinlay AlMcKinlay approved these changes

Assignees
No one assigned
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@xPaw @astorije @AlMcKinlay @maxpoulin64