~taiite/public-inbox

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
3 3

[PATCH senpai v4] MONITOR user with whom we have an open buffer

Details
Message ID
<20211119155335.10817-1-delthas@dille.cc>
DKIM signature
pass
Download raw message
Patch: +138 -15
---
 app.go         |  13 +++++++
 commands.go    |   3 ++
 irc/events.go  |   8 ++++
 irc/rpl.go     |   6 +++
 irc/session.go | 100 ++++++++++++++++++++++++++++++++++++++++++++-----
 irc/tokens.go  |   7 ++--
 ui/ui.go       |  16 +++++++-
 7 files changed, 138 insertions(+), 15 deletions(-)

diff --git a/app.go b/app.go
index d652b97..7b1efa2 100644
--- a/app.go
+++ b/app.go
@@ -92,6 +92,8 @@ type App struct {
	lastNetID     string
	lastBuffer    string

	monitor map[string]map[string]struct{} // set of targets we want to monitor per netID, best-effort. netID->target->{}

	lastMessageTime time.Time
	lastCloseTime   time.Time
}
@@ -102,6 +104,7 @@ func NewApp(cfg Config) (app *App, err error) {
		events:        make(chan event, eventChanSize),
		cfg:           cfg,
		messageBounds: map[boundKey]bound{},
		monitor:       make(map[string]map[string]struct{}),
	}

	if cfg.Highlights != nil {
@@ -577,6 +580,9 @@ func (app *App) handleIRCEvent(netID string, ev interface{}) {
			s.Close()
		}
		app.sessions[netID] = s
		if _, ok := app.monitor[netID]; !ok {
			app.monitor[netID] = make(map[string]struct{})
		}
		return
	}
	if _, ok := ev.(irc.Typing); ok {
@@ -628,6 +634,10 @@ func (app *App) handleIRCEvent(netID string, ev interface{}) {
			Head: "--",
			Body: ui.PlainString(body),
		})
		for target := range app.monitor[s.NetID()] {
			// TODO: batch MONITOR +
			s.MonitorAdd(target)
		}
	case irc.SelfNickEvent:
		var body ui.StyledStringBuilder
		body.WriteString(fmt.Sprintf("%s\u2192%s", ev.FormerNick, s.Nick()))
@@ -776,6 +786,8 @@ func (app *App) handleIRCEvent(netID string, ev interface{}) {
	case irc.MessageEvent:
		buffer, line, notification := app.formatMessage(s, ev)
		if buffer != "" && !s.IsChannel(buffer) {
			app.monitor[netID][buffer] = struct{}{}
			s.MonitorAdd(buffer)
			app.win.AddBuffer(netID, "", buffer)
		}
		app.win.AddLine(netID, buffer, notification, line)
@@ -794,6 +806,7 @@ func (app *App) handleIRCEvent(netID string, ev interface{}) {
			if s.IsChannel(target) {
				continue
			}
			s.MonitorAdd(target)
			app.win.AddBuffer(netID, "", target)
			// CHATHISTORY BEFORE excludes its bound, so add 1ms
			// (precision of the time tag) to include that last message.
diff --git a/commands.go b/commands.go
index f81974f..553a3bd 100644
--- a/commands.go
+++ b/commands.go
@@ -317,6 +317,8 @@ func commandDoMsg(app *App, args []string) (err error) {
			Time:            time.Now(),
		})
		if buffer != "" && !s.IsChannel(target) {
			app.monitor[netID][buffer] = struct{}{}
			s.MonitorAdd(buffer)
			app.win.AddBuffer(netID, "", buffer)
		}

@@ -425,6 +427,7 @@ func commandDoQuery(app *App, args []string) (err error) {
	if s.IsChannel(target) {
		return fmt.Errorf("cannot query a channel, use JOIN instead")
	}
	s.MonitorAdd(target)
	i, _ := app.win.AddBuffer(netID, "", target)
	app.win.JumpBufferIndex(i)
	return nil
diff --git a/irc/events.go b/irc/events.go
index 70a6c1d..ffac939 100644
--- a/irc/events.go
+++ b/irc/events.go
@@ -46,6 +46,14 @@ type UserQuitEvent struct {
	Channels []string
}

type UserOnlineEvent struct {
	User string
}

type UserOfflineEvent struct {
	User string
}

type TopicChangeEvent struct {
	Channel string
	Topic   string
diff --git a/irc/rpl.go b/irc/rpl.go
index 6373972..398b5b3 100644
--- a/irc/rpl.go
+++ b/irc/rpl.go
@@ -87,6 +87,12 @@ const (
	errUmodeunknownflag = "501" // :Unknown mode flag
	errUsersdontmatch   = "502" // :Can't change mode for other users

	rplMononline     = "730" // <nick> :target[!user@host][,target[!user@host]]*
	rplMonoffline    = "731" // <nick> :target[,target2]*
	rplMonlist       = "732" // <nick> :target[,target2]*
	rplEndofmonlist  = "733" // <nick> :End of MONITOR list
	errMonlistisfull = "734" // <nick> <limit> <targets> :Monitor list is full.

	rplLoggedin    = "900" // <nick> <nick>!<ident>@<host> <account> :You are now logged in as <user>
	rplLoggedout   = "901" // <nick> <nick>!<ident>@<host> :You are now logged out
	errNicklocked  = "902" // :You must use a nick assigned to you
diff --git a/irc/session.go b/irc/session.go
index 8467f2c..b5e1e7b 100644
--- a/irc/session.go
+++ b/irc/session.go
@@ -70,10 +70,11 @@ const (
	TypingDone
)

// User is a known IRC user (we share a channel with it).
// User is a known IRC user.
type User struct {
	Name *Prefix // the nick, user and hostname of the user if known.
	Away bool    // whether the user is away or not
	Name         *Prefix // the nick, user and hostname of the user if known.
	Away         bool    // whether the user is away or not
	Disconnected bool    // can only be true for monitored users.
}

// Channel is a joined channel.
@@ -123,6 +124,7 @@ type Session struct {
	historyLimit  int
	prefixSymbols string
	prefixModes   string
	monitor       bool

	users          map[string]*User        // known users.
	channels       map[string]Channel      // joined channels.
@@ -130,6 +132,7 @@ type Session struct {
	chReqs         map[string]struct{}     // set of targets for which history is currently requested.
	targetsBatchID string                  // ID of the channel history targets batch being processed.
	targetsBatch   HistoryTargetsEvent     // channel history targets batch being processed.
	monitors       map[string]struct{}     // set of users we want to monitor (and keep even if disconnected).

	pendingChannels map[string]time.Time // set of join requests stamps for channels.
}
@@ -157,6 +160,7 @@ func NewSession(out chan<- Message, params SessionParams) *Session {
		channels:        map[string]Channel{},
		chBatches:       map[string]HistoryEvent{},
		chReqs:          map[string]struct{}{},
		monitors:        map[string]struct{}{},
		pendingChannels: map[string]time.Time{},
	}

@@ -230,16 +234,18 @@ func (s *Session) Names(target string) []Member {
			names = make([]Member, 0, len(c.Members))
			for u, pl := range c.Members {
				names = append(names, Member{
					PowerLevel: pl,
					Name:       u.Name.Copy(),
					Away:       u.Away,
					PowerLevel:   pl,
					Name:         u.Name.Copy(),
					Away:         u.Away,
					Disconnected: u.Disconnected,
				})
			}
		}
	} else if u, ok := s.users[s.Casemap(target)]; ok {
		names = append(names, Member{
			Name: u.Name.Copy(),
			Away: u.Away,
			Name:         u.Name.Copy(),
			Away:         u.Away,
			Disconnected: u.Disconnected,
		})
		names = append(names, Member{
			Name: &Prefix{
@@ -273,7 +279,7 @@ func (s *Session) TypingStops() <-chan Typing {

func (s *Session) ChannelsSharedWith(name string) []string {
	var user *User
	if u, ok := s.users[s.Casemap(name)]; ok {
	if u, ok := s.users[s.Casemap(name)]; ok && !u.Disconnected {
		user = u
	} else {
		return nil
@@ -759,6 +765,7 @@ func (s *Session) handleRegistered(msg Message) (Event, error) {
		nickCf := s.Casemap(msg.Prefix.Name)

		if u, ok := s.users[nickCf]; ok {
			u.Disconnected = true
			var channels []string
			for channelCf, c := range s.channels {
				if _, ok := c.Members[u]; ok {
@@ -773,6 +780,48 @@ func (s *Session) handleRegistered(msg Message) (Event, error) {
				Channels: channels,
			}, nil
		}
	case rplMononline:
		for _, target := range strings.Split(msg.Params[1], ",") {
			prefix := ParsePrefix(target)
			nickCf := s.casemap(prefix.Name)

			if _, ok := s.monitors[nickCf]; ok {
				u, ok := s.users[nickCf]
				if !ok {
					u = &User{
						Name: prefix,
					}
					s.users[nickCf] = u
				}
				if u.Disconnected {
					u.Disconnected = false
					return UserOnlineEvent{
						User: u.Name.Name,
					}, nil
				}
			}
		}
	case rplMonoffline:
		for _, target := range strings.Split(msg.Params[1], ",") {
			prefix := ParsePrefix(target)
			nickCf := s.casemap(prefix.Name)

			if _, ok := s.monitors[nickCf]; ok {
				u, ok := s.users[nickCf]
				if !ok {
					u = &User{
						Name: prefix,
					}
					s.users[nickCf] = u
				}
				if !u.Disconnected {
					u.Disconnected = true
					return UserOfflineEvent{
						User: u.Name.Name,
					}, nil
				}
			}
		}
	case rplNamreply:
		var channel, names string
		if err := msg.ParseParams(nil, nil, &channel, &names); err != nil {
@@ -1115,6 +1164,8 @@ func (s *Session) handleRegistered(msg Message) (Event, error) {
			Code:     code,
			Message:  strings.Join(msg.Params[2:], " "),
		}, nil
	case errMonlistisfull:
		// silence monlist full error, we don't care because we do it best-effort
	default:
		if msg.IsReply() {
			if len(msg.Params) < 2 {
@@ -1158,12 +1209,16 @@ func (s *Session) newMessageEvent(msg Message) (ev MessageEvent, err error) {
}

func (s *Session) cleanUser(parted *User) {
	nameCf := s.Casemap(parted.Name.Name)
	if _, ok := s.monitors[nameCf]; ok {
		return
	}
	for _, c := range s.channels {
		if _, ok := c.Members[parted]; ok {
			return
		}
	}
	delete(s.users, s.Casemap(parted.Name.Name))
	delete(s.users, nameCf)
}

func (s *Session) updateFeatures(features []string) {
@@ -1225,6 +1280,11 @@ func (s *Session) updateFeatures(features []string) {
			if err == nil && linelen != 0 {
				s.linelen = linelen
			}
		case "MONITOR":
			monitor, err := strconv.Atoi(value)
			if err == nil && monitor > 0 {
				s.monitor = true
			}
		case "PREFIX":
			if value == "" {
				s.prefixModes = ""
@@ -1259,3 +1319,23 @@ func (s *Session) endRegistration() {
		s.out <- NewMessage("CAP", "END")
	}
}

func (s *Session) MonitorAdd(target string) {
	targetCf := s.casemap(target)
	if _, ok := s.monitors[targetCf]; !ok {
		s.monitors[targetCf] = struct{}{}
		if s.monitor {
			s.out <- NewMessage("MONITOR", "+", target)
		}
	}
}

func (s *Session) MonitorRemove(target string) {
	targetCf := s.casemap(target)
	if _, ok := s.monitors[targetCf]; ok {
		delete(s.monitors, targetCf)
		if s.monitor {
			s.out <- NewMessage("MONITOR", "-", target)
		}
	}
}
diff --git a/irc/tokens.go b/irc/tokens.go
index ab7b418..aaffc08 100644
--- a/irc/tokens.go
+++ b/irc/tokens.go
@@ -460,9 +460,10 @@ func ParseCaps(caps string) (diff []Cap) {

// Member is a token in RPL_NAMREPLY's last parameter.
type Member struct {
	PowerLevel string
	Name       *Prefix
	Away       bool
	PowerLevel   string
	Name         *Prefix
	Away         bool
	Disconnected bool
}

type members []Member
diff --git a/ui/ui.go b/ui/ui.go
index 97f7148..1f96d76 100644
--- a/ui/ui.go
+++ b/ui/ui.go
@@ -398,15 +398,27 @@ func drawVerticalMemberList(screen tcell.Screen, x0, y0, width, height int, memb
	width--
	clearArea(screen, x0, y0, width, height)

	padding := 1
	for _, m := range members {
		if m.Disconnected {
			padding = runeWidth(0x274C)
			break
		}
	}

	for i, m := range members[*offset:] {
		x := x0
		y := y0 + i
		if m.PowerLevel != "" {
		if m.Disconnected {
			disconnectedSt := tcell.StyleDefault.Foreground(tcell.ColorRed)
			printString(screen, &x, y, Styled("\u274C", disconnectedSt))
		} else if m.PowerLevel != "" {
			x += padding - 1
			powerLevelText := m.PowerLevel[:1]
			powerLevelSt := tcell.StyleDefault.Foreground(tcell.ColorGreen)
			printString(screen, &x, y, Styled(powerLevelText, powerLevelSt))
		} else {
			x++
			x += padding
		}

		var name StyledString

base-commit: 6014ba12270933c74018f2914713ec6fb2ed4a77
-- 
2.17.1
Details
Message ID
<CG342UUPVZUJ.3CGI9Y9LATB3I@vps807759>
In-Reply-To
<20211119155335.10817-1-delthas@dille.cc> (view parent)
DKIM signature
pass
Download raw message
*sending this message from alps, sorry if it does not render properly*

Thanks for the patch. I can't test it right now, but at a glance you might need to rebase on latest master.

> @@ -130,6 +132,7 @@ type Session struct {
>  	chReqs         map[string]struct{}     // set of targets for which history is currently requested.
>  	targetsBatchID string                  // ID of the channel history targets batch being processed.
>  	targetsBatch   HistoryTargetsEvent     // channel history targets batch being processed.
> +	monitors       map[string]struct{}     // set of users we want to monitor (and keep even if disconnected).

Comment is a bit confusing at first, can you s/if/if they are/

Also, instead of a map, how about a "monitored" field in s.users? Since it's only useful for RPL_MONONLINE handling, where you need access to s.users anyway.

>
>  	pendingChannels map[string]time.Time // set of join requests stamps for channels.
>  }
> @@ -773,6 +780,48 @@ func (s *Session) handleRegistered(msg Message) (Event, error) {
>  				Channels: channels,
>  			}, nil
>  		}
> +	case rplMononline:
> +		for _, target := range strings.Split(msg.Params[1], ",") {
> +			prefix := ParsePrefix(target)
> +			nickCf := s.casemap(prefix.Name)

can you add a nil check for prefix

> +
> +			if _, ok := s.monitors[nickCf]; ok {
> +				u, ok := s.users[nickCf]
> +				if !ok {

is ok ever false? I think s.monitors would better be a field in each *User.

> +					u = &User{
> +						Name: prefix,
> +					}
> +					s.users[nickCf] = u
> +				}
> +				if u.Disconnected {
> +					u.Disconnected = false
> +					return UserOnlineEvent{
> +						User: u.Name.Name,
> +					}, nil
> +				}
> +			}
> +		}
> +	case rplMonoffline:
> +		for _, target := range strings.Split(msg.Params[1], ",") {
> +			prefix := ParsePrefix(target)

second nil check here please

> +			nickCf := s.casemap(prefix.Name)
> +
> +			if _, ok := s.monitors[nickCf]; ok {
> +				u, ok := s.users[nickCf]
> +				if !ok {
> +					u = &User{
> +						Name: prefix,
> +					}
> +					s.users[nickCf] = u
> +				}
> +				if !u.Disconnected {
> +					u.Disconnected = true
> +					return UserOfflineEvent{
> +						User: u.Name.Name,
> +					}, nil
> +				}
> +			}
> +		}
>  	case rplNamreply:
>  		var channel, names string
>  		if err := msg.ParseParams(nil, nil, &channel, &names); err != nil {
>  }
>
>  func (s *Session) cleanUser(parted *User) {
> +	nameCf := s.Casemap(parted.Name.Name)
> +	if _, ok := s.monitors[nameCf]; ok {
> +		return
> +	}
>  	for _, c := range s.channels {
>  		if _, ok := c.Members[parted]; ok {
>  			return
>  		}
>  	}
> -	delete(s.users, s.Casemap(parted.Name.Name))
> +	delete(s.users, nameCf)
>  }
>
>  func (s *Session) updateFeatures(features []string) {
> @@ -1259,3 +1319,23 @@ func (s *Session) endRegistration() {
>  		s.out <- NewMessage("CAP", "END")
>  	}
>  }
> +
> +func (s *Session) MonitorAdd(target string) {
> +	targetCf := s.casemap(target)
> +	if _, ok := s.monitors[targetCf]; !ok {
> +		s.monitors[targetCf] = struct{}{}
> +		if s.monitor {

calls to this function before 005 is processed (e.g. the ones you added on RegisteredEvent) will not send MONITOR to the server.
MONITOR should also be sent on 005.

> +			s.out <- NewMessage("MONITOR", "+", target)
> +		}
> +	}
> +}
> +
> +func (s *Session) MonitorRemove(target string) {
> +	targetCf := s.casemap(target)
> +	if _, ok := s.monitors[targetCf]; ok {
> +		delete(s.monitors, targetCf)
> +		if s.monitor {
> +			s.out <- NewMessage("MONITOR", "-", target)
> +		}
> +	}
> +}

can you move up those two functions somewhere between HandleMessage and SendRaw
Details
Message ID
<780165fd62013635e544d4f1210ab8d9@dille.cc>
In-Reply-To
<CG342UUPVZUJ.3CGI9Y9LATB3I@vps807759> (view parent)
DKIM signature
pass
Download raw message
> Also, instead of a map, how about a "monitored" field in s.users?
> Since it's only useful for RPL_MONONLINE handling, where you need
> access to s.users anyway.

> is ok ever false? I think s.monitors would better be a field in each 
> *User.

Sadly we have to store the monitor state outside of Session because 
Session are recreated on disconnect/reconnect and we want to remember 
which users we track between reconnects.

As in, I open a query with bar, then I am disconnected. When I 
reconnect, I want to send MONITOR for bar. So this has to be stored 
outside of Session. Which means outside of User as well.

Thoughts?
Details
Message ID
<d2485d35-019d-7c7f-a76c-bd1e536de25f@hirtz.pm>
In-Reply-To
<780165fd62013635e544d4f1210ab8d9@dille.cc> (view parent)
DKIM signature
pass
Download raw message
Sorry, this mail fell into my spam folder.

On 01/12/2021 13:36, delthas wrote:
> Sadly we have to store the monitor state outside of Session because 
> Session are recreated on disconnect/reconnect and we want to remember 
> which users we track between reconnects.

Talking about "Session.monitors", not "App.monitors" here.

"Session.monitors" does keep track of monitored users within the 
lifetime of the session.
Reply to thread Export thread (mbox)