partial implementation of #729

This propagates CS AMODE changes to the actual modes, but not the other
way around.

Also fixes #909.

irc/channel.go

@@ -1260,22 +1260,20 @@ func (channel *Channel) SendSplitMessage(command string, minPrefixMode modes.Mod
 	})
 }
 
-func (channel *Channel) applyModeToMember(client *Client, mode modes.Mode, op modes.ModeOp, nick string, rb *ResponseBuffer) (result *modes.ModeChange) {
+func (channel *Channel) applyModeToMember(client *Client, change modes.ModeChange, rb *ResponseBuffer) (applied bool, result modes.ModeChange) {
-	target := channel.server.clients.Get(nick)
+	target := channel.server.clients.Get(change.Arg)
 	if target == nil {
-		rb.Add(nil, client.server.name, ERR_NOSUCHNICK, client.Nick(), utils.SafeErrorParam(nick), client.t("No such nick"))
+		rb.Add(nil, client.server.name, ERR_NOSUCHNICK, client.Nick(), utils.SafeErrorParam(change.Arg), client.t("No such nick"))
-		return nil
+		return
 	}
+	change.Arg = target.Nick()
 
 	channel.stateMutex.Lock()
 	modeset, exists := channel.members[target]
 	if exists {
-		if modeset.SetMode(mode, op == modes.Add) {
+		if modeset.SetMode(change.Mode, change.Op == modes.Add) {
-			result = &modes.ModeChange{
+			applied = true
-				Op:   op,
+			result = change
-				Mode: mode,
-				Arg:  nick,
-			}
 		}
 	}
 	channel.stateMutex.Unlock()

irc/chanserv.go

@@ -238,7 +238,16 @@ func csAmodeHandler(server *Server, client *Client, command string, params []str
 		}
 	case modes.Add, modes.Remove:
 		if len(affectedModes) > 0 {
-			csNotice(rb, fmt.Sprintf(client.t("Successfully set mode %s"), change.String()))
+			csNotice(rb, fmt.Sprintf(client.t("Successfully set persistent mode %s%s on %s"), string(change.Op), string(change.Mode), change.Arg))
+			// #729: apply change to current membership
+			for _, member := range channel.Members() {
+				if member.Account() == change.Arg {
+					applied, change := channel.applyModeToMember(client, change, rb)
+					if applied {
+						announceCmodeChanges(channel, modes.ModeChanges{change}, chanservMask, rb)
+					}
+				}
+			}
 		} else {
 			csNotice(rb, client.t("No changes were made"))
 		}
@@ -275,14 +284,14 @@ func csOpHandler(server *Server, client *Client, command string, params []string
 	if clientAccount == target.Account() {
 		givenMode = modes.ChannelFounder
 	}
-	change := channelInfo.applyModeToMember(client, givenMode, modes.Add, target.NickCasefolded(), rb)
+	applied, change := channelInfo.applyModeToMember(client,
-	if change != nil {
+		modes.ModeChange{Mode: givenMode,
-		//TODO(dan): we should change the name of String and make it return a slice here
+			Op:  modes.Add,
-		//TODO(dan): unify this code with code in modes.go
+			Arg: target.NickCasefolded(),
-		args := append([]string{channelName}, strings.Split(change.String(), " ")...)
+		},
-		for _, member := range channelInfo.Members() {
+		rb)
-			member.Send(nil, fmt.Sprintf("ChanServ!services@%s", client.server.name), "MODE", args...)
+	if applied {
-		}
+		announceCmodeChanges(channelInfo, modes.ModeChanges{change}, chanservMask, rb)
 	}
 
 	csNotice(rb, fmt.Sprintf(client.t("Successfully op'd in channel %s"), channelName))
@@ -326,14 +335,15 @@ func csRegisterHandler(server *Server, client *Client, command string, params []
 	server.snomasks.Send(sno.LocalChannels, fmt.Sprintf(ircfmt.Unescape("Channel registered $c[grey][$r%s$c[grey]] by $c[grey][$r%s$c[grey]]"), channelName, client.nickMaskString))
 
 	// give them founder privs
-	change := channelInfo.applyModeToMember(client, modes.ChannelFounder, modes.Add, client.NickCasefolded(), rb)
+	applied, change := channelInfo.applyModeToMember(client,
-	if change != nil {
+		modes.ModeChange{
-		//TODO(dan): we should change the name of String and make it return a slice here
+			Mode: modes.ChannelFounder,
-		//TODO(dan): unify this code with code in modes.go
+			Op:   modes.Add,
-		args := append([]string{channelName}, strings.Split(change.String(), " ")...)
+			Arg:  client.NickCasefolded(),
-		for _, member := range channelInfo.Members() {
+		},
-			member.Send(nil, fmt.Sprintf("ChanServ!services@%s", client.server.name), "MODE", args...)
+		rb)
-		}
+	if applied {
+		announceCmodeChanges(channelInfo, modes.ModeChanges{change}, chanservMask, rb)
 	}
 }
 

irc/handlers.go

@@ -1530,38 +1530,25 @@ func cmodeHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Res
 	}
 	// process mode changes, include list operations (an empty set of changes does a list)
 	applied := channel.ApplyChannelModeChanges(client, msg.Command == "SAMODE", changes, rb)
+	announceCmodeChanges(channel, applied, client.NickMaskString(), rb)
 
-	// save changes
+	return false
-	var includeFlags uint
-	for _, change := range applied {
-		includeFlags |= IncludeModes
-		if change.Mode == modes.BanMask || change.Mode == modes.ExceptMask || change.Mode == modes.InviteMask {
-			includeFlags |= IncludeLists
-		}
-	}
-
-	if includeFlags != 0 {
-		channel.MarkDirty(includeFlags)
 }
 
+func announceCmodeChanges(channel *Channel, applied modes.ModeChanges, source string, rb *ResponseBuffer) {
 	// send out changes
 	if len(applied) > 0 {
-		prefix := client.NickMaskString()
 		//TODO(dan): we should change the name of String and make it return a slice here
-		args := append([]string{channel.name}, strings.Split(applied.String(), " ")...)
+		args := append([]string{channel.name}, applied.Strings()...)
-		rb.Add(nil, prefix, "MODE", args...)
+		rb.Add(nil, source, "MODE", args...)
-		for _, session := range client.Sessions() {
-			if session != rb.session {
-				session.Send(nil, prefix, "MODE", args...)
-			}
-		}
 		for _, member := range channel.Members() {
-			if member != client {
+			for _, session := range member.Sessions() {
-				member.Send(nil, prefix, "MODE", args...)
+				if session != rb.session {
+					session.Send(nil, source, "MODE", args...)
+				}
 			}
 		}
 	}
-	return false
 }
 
 // MODE <client> [<modestring> [<mode arguments>...]]
@@ -1606,7 +1593,8 @@ func umodeHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Res
 	}
 
 	if len(applied) > 0 {
-		rb.Add(nil, cDetails.nickMask, "MODE", targetNick, applied.String())
+		args := append([]string{targetNick}, applied.Strings()...)
+		rb.Add(nil, cDetails.nickMask, "MODE", args...)
 	} else if hasPrivs {
 		rb.Add(nil, server.name, RPL_UMODEIS, targetNick, target.ModeString())
 		if target.HasMode(modes.LocalOperator) || target.HasMode(modes.Operator) {
@@ -2122,7 +2110,8 @@ func applyOper(client *Client, oper *Oper, rb *ResponseBuffer) {
 		client.server.snomasks.Send(sno.LocalOpers, fmt.Sprintf(ircfmt.Unescape("Client opered up $c[grey][$r%s$c[grey], $r%s$c[grey]]"), newDetails.nickMask, oper.Name))
 
 		rb.Broadcast(nil, client.server.name, RPL_YOUREOPER, details.nick, client.t("You are now an IRC operator"))
-		rb.Broadcast(nil, client.server.name, "MODE", details.nick, applied.String())
+		args := append([]string{details.nick}, applied.Strings()...)
+		rb.Broadcast(nil, client.server.name, "MODE", args...)
 	} else {
 		client.server.snomasks.Send(sno.LocalOpers, fmt.Sprintf(ircfmt.Unescape("Client deopered $c[grey][$r%s$c[grey]]"), newDetails.nickMask))
 	}

irc/modes.go

@@ -256,13 +256,28 @@ func (channel *Channel) ApplyChannelModeChanges(client *Client, isSamode bool, c
 				continue
 			}
 
-			change := channel.applyModeToMember(client, change.Mode, change.Op, nick, rb)
+			success, change := channel.applyModeToMember(client, change, rb)
-			if change != nil {
+			if success {
-				applied = append(applied, *change)
+				applied = append(applied, change)
 			}
 		}
 	}
 
+	var includeFlags uint
+	for _, change := range applied {
+		switch change.Mode {
+		case modes.BanMask, modes.ExceptMask, modes.InviteMask:
+			includeFlags |= IncludeLists
+		case modes.ChannelFounder, modes.ChannelAdmin, modes.ChannelOperator, modes.Halfop, modes.Voice:
+			// these are never persisted currently, but might be in the future (see discussion on #729)
+		default:
+			includeFlags |= IncludeModes
+		}
+	}
+	if includeFlags != 0 {
+		channel.MarkDirty(includeFlags)
+	}
+
 	// #649: don't send 324 RPL_CHANNELMODEIS if we were only working with mask lists
 	if len(applied) == 0 && !alreadySentPrivError && (maskOpCount == 0 || maskOpCount < len(changes)) {
 		args := append([]string{details.nick, chname}, channel.modeStrings(client)...)

irc/modes/modes.go

@@ -28,10 +28,6 @@ var (
 // ModeOp is an operation performed with modes
 type ModeOp rune
 
-func (op ModeOp) String() string {
-	return string(op)
-}
-
 const (
 	// Add is used when adding the given key.
 	Add ModeOp = '+'
@@ -55,43 +51,36 @@ type ModeChange struct {
 	Arg  string
 }
 
-func (change *ModeChange) String() (str string) {
-	if (change.Op == Add) || (change.Op == Remove) {
-		str = change.Op.String()
-	}
-	str += change.Mode.String()
-	if change.Arg != "" {
-		str += " " + change.Arg
-	}
-	return
-}
-
 // ModeChanges are a collection of 'ModeChange's
 type ModeChanges []ModeChange
 
-func (changes ModeChanges) String() string {
+func (changes ModeChanges) Strings() (result []string) {
 	if len(changes) == 0 {
-		return ""
+		return
 	}
 
+	var builder strings.Builder
+
 	op := changes[0].Op
-	str := changes[0].Op.String()
+	builder.WriteRune(rune(op))
 
 	for _, change := range changes {
 		if change.Op != op {
 			op = change.Op
-			str += change.Op.String()
+			builder.WriteRune(rune(op))
 		}
-		str += change.Mode.String()
+		builder.WriteRune(rune(change.Mode))
 	}
 
+	result = append(result, builder.String())
+
 	for _, change := range changes {
 		if change.Arg == "" {
 			continue
 		}
-		str += " " + change.Arg
+		result = append(result, change.Arg)
 	}
-	return str
+	return
 }
 
 // Modes is just a raw list of modes

irc/modes/modes_test.go

@@ -219,6 +219,22 @@ func TestHighestChannelUserMode(t *testing.T) {
 	}
 }
 
+func TestModeChangesString(t *testing.T) {
+	m := ModeChanges{
+		ModeChange{Op: Add, Mode: RegisteredOnly},
+		ModeChange{Op: Add, Mode: Key, Arg: "beer"},
+		ModeChange{Op: Add, Mode: BanMask, Arg: "shivaram"},
+	}
+	assertEqual(m.Strings(), []string{"+Rkb", "beer", "shivaram"}, t)
+
+	m = ModeChanges{
+		ModeChange{Op: Add, Mode: RegisteredOnly},
+		ModeChange{Op: Remove, Mode: Key, Arg: "beer"},
+		ModeChange{Op: Add, Mode: BanMask, Arg: "shivaram"},
+	}
+	assertEqual(m.Strings(), []string{"+R-k+b", "beer", "shivaram"}, t)
+}
+
 func BenchmarkModeString(b *testing.B) {
 	set := NewModeSet()
 	set.SetMode('A', true)