Take an optional argument in `/part` #1476

eliemichel · 2017-08-30T14:19:56Z

/part #foo or /close #foo closes the chan #foo even if it is not the active one.

/part and /close without an argument keep their previous meaning.

AlMcKinlay

Looks good, just 1 pedantic code style thing :-)

AlMcKinlay · 2017-08-30T14:27:07Z

src/plugins/inputs/part.js

-	if (chan.type === Chan.Type.LOBBY) {
+	let target, partedChan;
+	if (args.length === 0) {
+		target = chan.name;


Could we simplify this and only have 1 variable, because we are just storing the name twice essentially.

So something like

const target = args.length === 0 ? chan : _.find(network.channels, {name: args[0]});

And then when we want to refer to the name, we can just do target.name. Does that make sense?

xPaw · 2017-08-30T14:50:50Z

src/plugins/inputs/part.js

 	});

-	if (chan.type === Chan.Type.CHANNEL) {
+	if (target.type === Chan.Type.CHANNEL) {
 		this.save();

 		if (network.irc) {
 			const partMessage = args[0] ? args.join(" ") : Helper.config.leaveMessage;


If you provide a target into the command, part message will be wrong here?

Oh yeah, good point.

https://en.wikipedia.org/wiki/List_of_Internet_Relay_Chat_commands#PART

Spec says PART <channels> [<message>] (where channels is a csv). So I guess we want to check if the chan exists, the nthe part message is the rest of the args. If the chan doesn't exist, then it's a part message. Although that could get messy.

How do other clients deal with this? Do they support /part without a chan argument?

Mmh does the lobby chan have a name? If not, it will never by /parted from anywhere but itself.

It does, the same name you see in the UI.

Ok, but what do you expect to happen when somebody does a /part Freenode for instance? For know, it responds You cannot part from networks, which does not seem unreasonable to me. Though, I'm ready to change this behavior if needed.

Well, a part message makes no sense for a query window.

So I'd say that if there is more than 1 arg and we are trying to part a query, don't let the user do it.

I propose the following:

/part #foo bar quits #foo with message "bar".

/part foo bar quits the current window with message "foo bar" even if "foo" is a valid query window..

/part foo with "foo" a valid query window quits the query window foo with no message.

/part foo with "foo" not being a valid query window quits the current window with message "foo".

As a by-product, parting a query window with a leave message is still possible, but only by switching to this window and /part some message-ing.

Sounds fine to me.

parting a query window with a leave message is still possible,

Since part is only sent for channels, it's effectively discarded.

Yup, sounds fine to me also.

also, /part #foo bar displays an error message and do nothing if #foo does not exist as a window. It does not part the current window as it would do for /part foo bar (with no #)

AlMcKinlay

Looks great, thanks for this.

This needs to be squashed before we merge.

xPaw · 2017-08-30T14:52:31Z

src/plugins/inputs/part.js

+	if (typeof target === "undefined") {
+		chan.pushMessage(this, new Msg({
+			type: Msg.Type.ERROR,
+			text: "Cannot part channel " + target.name + ": Channel not found."


target.name is undefined here.

Ah yeah I remember now why I did not go for the ternary at first..

following thelounge#1476 (comment)

MaxLeiter · 2017-08-31T19:17:39Z

src/plugins/inputs/part.js

-	if (chan.type === Chan.Type.LOBBY) {
+	// Whether args[0] is to be considered as a target to part or the first word
+	// of leave message.
+	const targetArgument = args.length > 0 && (_.startsWith(args[0], "#") || args.length === 1);


This is on the server, so args[0].startsWith should work.

ignore this, see @xPaw's comment

xPaw

Checking channel by comparing if the first char is # is only going to bring us problems.

Best and easiest way I see of doing this is checking there's a channel with name args[0], if yes get part starting with args[1], otherwise entire args becomes part message.

dgw · 2017-08-31T20:59:30Z

I agree with @xPaw. Checking for whether args[0] is an existing channel is an easy solution that leaves room for progressive enhancement (e.g. supporting multiple comma-separated channels, checking potential channel names against CHANTYPES instead of hardcoding #, etc.).

Overall, I'm excited to be able to part channels without switching to them! 👍

MaxLeiter · 2017-09-08T16:39:48Z

Can you squash commits, @eliemichel? Great work!

astorije · 2017-09-09T01:14:56Z

Do we need to update https://github.com/thelounge/lounge/blob/master/client/index.html#L788-L796 ?

astorije

Just to make sure we don't merge this with unsquashed commits (I can help with this if you want @eliemichel) and without Help window changes.

eliemichel · 2017-09-10T09:30:24Z

Yey sorry guys I'm kind of in a rush for a deadline now, ping me back if I haven't done this end of next week. ;)

Fix thelounge#1430

astorije · 2017-09-14T00:28:31Z

@eliemichel, since you're busy, I took the liberty to update the help section, and squash your commits. Everything else in the code, I left it untouched :)

I'll let @xPaw or @YaManicKill merge it if they agree with the help changes.

eliemichel · 2017-09-16T12:23:33Z

@astorije Thx!

AlMcKinlay · 2017-09-16T19:29:14Z

@astorije Thx!

No, thank you. This is a great addition, thanks for it :-)

AlMcKinlay requested changes Aug 30, 2017

View reviewed changes

xPaw reviewed Aug 30, 2017

View reviewed changes

AlMcKinlay approved these changes Aug 30, 2017

View reviewed changes

xPaw reviewed Aug 30, 2017

View reviewed changes

xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Aug 30, 2017

xPaw added this to the 2.5.0 milestone Aug 30, 2017

astorije assigned xPaw Aug 31, 2017

eliemichel added a commit to eliemichel/lounge that referenced this pull request Aug 31, 2017

Fix leave message

39787fb

following thelounge#1476 (comment)

MaxLeiter reviewed Aug 31, 2017

View reviewed changes

xPaw requested changes Aug 31, 2017

View reviewed changes

astorije assigned eliemichel and unassigned xPaw Aug 31, 2017

astorije assigned xPaw and unassigned eliemichel Sep 8, 2017

xPaw approved these changes Sep 9, 2017

View reviewed changes

astorije requested changes Sep 10, 2017

View reviewed changes

astorije assigned eliemichel and unassigned xPaw Sep 10, 2017

Take an optional argument in /part

f26c2da

Fix thelounge#1430

astorije force-pushed the pr-part-other branch from 4df1ebc to f26c2da Compare September 14, 2017 00:26

astorije approved these changes Sep 14, 2017

View reviewed changes

astorije assigned xPaw and AlMcKinlay and unassigned eliemichel Sep 14, 2017

xPaw merged commit 4431616 into thelounge:master Sep 14, 2017

eliemichel deleted the pr-part-other branch September 16, 2017 12:23

astorije changed the title ~~Take an optional argument in /part~~ Take an optional argument in /part Sep 29, 2017

AlMcKinlay removed their assignment Mar 12, 2018

xPaw removed their assignment Mar 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take an optional argument in `/part` #1476

Take an optional argument in `/part` #1476

eliemichel commented Aug 30, 2017

AlMcKinlay left a comment

AlMcKinlay Aug 30, 2017

eliemichel Aug 30, 2017

xPaw Aug 30, 2017

AlMcKinlay Aug 30, 2017

eliemichel Aug 30, 2017

xPaw Aug 30, 2017

eliemichel Aug 31, 2017

AlMcKinlay Aug 31, 2017

eliemichel Aug 31, 2017

xPaw Aug 31, 2017

AlMcKinlay Aug 31, 2017

eliemichel Aug 31, 2017

AlMcKinlay left a comment •

edited

xPaw Aug 30, 2017

eliemichel Aug 30, 2017

MaxLeiter Aug 31, 2017

MaxLeiter Aug 31, 2017

xPaw left a comment

dgw commented Aug 31, 2017

MaxLeiter commented Sep 8, 2017

astorije commented Sep 9, 2017

astorije left a comment

eliemichel commented Sep 10, 2017

astorije commented Sep 14, 2017

eliemichel commented Sep 16, 2017

AlMcKinlay commented Sep 16, 2017

Take an optional argument in /part #1476

Take an optional argument in /part #1476

Conversation

eliemichel commented Aug 30, 2017

AlMcKinlay left a 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

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 left a comment • edited

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

xPaw left a comment

Choose a reason for hiding this comment

dgw commented Aug 31, 2017

MaxLeiter commented Sep 8, 2017

astorije commented Sep 9, 2017

astorije left a comment

Choose a reason for hiding this comment

eliemichel commented Sep 10, 2017

astorije commented Sep 14, 2017

eliemichel commented Sep 16, 2017

AlMcKinlay commented Sep 16, 2017

Take an optional argument in `/part` #1476

Take an optional argument in `/part` #1476

AlMcKinlay left a comment •

edited