Thanks for sending a patch! Here are a few comments/questions.
On Thursday, June 22nd, 2023 at 12:48, sitting33 <me@sit.sh> wrote:
> ---> This also prevents marking the buffer as read when the window/tab isn't> focused. When the focus gets restored, it gets marked.> > I also added the server name in the title, since it fits there anyway.
Nice! Would it be possible to split this patch into two, one to change the
title, and the other one with the focus stuff?
I wonder if we want to display the total number of unread buffers, or only the
number of unread highlights. The former sounds a bit spammy: high-traffic
channels have new messages every few seconds...
> Thanks for the awesome webchat!> > Implements: https://todo.sr.ht/~emersion/gamja/134> > components/app.js | 77 +++++++++++++++++++++++++++++++++++++++++++----> 1 file changed, 71 insertions(+), 6 deletions(-)> > diff --git a/components/app.js b/components/app.js> index 3987749..a5b6aeb 100644> --- a/components/app.js> +++ b/components/app.js> @@ -585,14 +585,40 @@ export default class App extends Component {> this.whoChannelBuffer(buf.name, buf.server);> }> > - if (buf.type !== BufferType.SERVER) {> - document.title = buf.name + ' · ' + this.baseTitle;> - } else {> - document.title = this.baseTitle;> - }> + this.updateDocumentTitle();> });> }> > + updateDocumentTitle() {> + const currentBuffer = State.getBuffer(this.state, this.state.activeBuffer);> + const currentServerName = getServerName(this.state.servers.get(currentBuffer.server));> +> + let resultingTitle = "";> +> + // add unread buffers counter> + let unreadBuffersCounter = 0;> + for (const buffer of this.state.buffers.values()) {> + if (buffer.unread > Unread.NONE) unreadBuffersCounter += 1;
Style nit: braces are mandatory.
> + }> + if (unreadBuffersCounter > 0) {> + resultingTitle += "(";> + resultingTitle += unreadBuffersCounter;> + resultingTitle += ") ";
Nit: can be simplified with format strings:
resultingTitle += `(${unreadBuffersCounter}) `;
> + }> +> + // add current buffer name> + if (currentBuffer.type !== BufferType.SERVER) {> + resultingTitle += currentBuffer.name + ' · ';> + }> +> + resultingTitle += currentServerName + ' · ';> +> + // add the base title (document.title)> + resultingTitle += this.baseTitle;> +> + document.title = resultingTitle;> + }> +> prepareChatMessage(serverID, msg) {> // Treat server-wide broadcasts as highlights. They're sent by server> // operators and can contain important information.> @@ -717,7 +743,7 @@ export default class App extends Component {> let prevReadReceipt = buf.prevReadReceipt;> let receipts = { [ReceiptType.DELIVERED]: receiptFromMessage(msg) };> > - if (this.state.activeBuffer !== buf.id) {> + if (this.state.activeBuffer !== buf.id || !document.hasFocus()) {> unread = Unread.union(unread, msgUnread);> } else {> receipts[ReceiptType.READ] = receiptFromMessage(msg);> @@ -738,6 +764,10 @@ export default class App extends Component {> this.sendReadReceipt(client, stored);> }> return { unread, prevReadReceipt };> + }, () => {> + // don't update the title if we're getting the history> + const chatHistoryBatch = irc.findBatchByType(msg, "chathistory");> + if (!chatHistoryBatch) this.updateDocumentTitle();
Style nit: braces are mandatory
Here we're updating the title when receiving a new message, but there are other
situations where the number of unread messages changes. It's possible to grep
for "unread" buffer state changes to list these. For instance, when receiving
a MARKREAD message (another client has marked a message as read).
> });> }> > @@ -1912,13 +1942,48 @@ export default class App extends Component {> }> }> > + onDocumentFocus() {> + const buffer = State.getBuffer(this.state, this.state.activeBuffer);> + const client = this.clients.get(buffer.server);> +> + const lastMessage = buffer.messages[buffer.messages.length - 1];> + if (!lastMessage) return;
Style nit: braces are mandatory
> +> + const lastReceipt = getReceipt(buffer, ReceiptType.READ);
The buffer can be null here I believe, before logging in.
> + const newReceipt = receiptFromMessage(lastMessage);> + if (lastReceipt && lastReceipt.time === newReceipt.time) {> + return;> + }> +> + this.setBufferState(buffer, buffer => {> + const stored = {> + name: buffer.name,> + server: client.params,> + unread: Unread.NONE,> + receipts: { [ReceiptType.READ]: newReceipt },> + };> + if (this.bufferStore.put(stored)) {> + this.sendReadReceipt(client, stored);> + }> +> + return {> + unread: Unread.NONE,> + };> + }, () => {> + this.updateDocumentTitle();> + });
This sounds like duplicated code with switchBuffer(). Maybe we can consolidate
the common bits into a single function?
(Also this doesn't dismiss notifications.)
> + }> +> componentDidMount() {> this.baseTitle = document.title;> setupKeybindings(this);> +> + document.addEventListener("focus", this.onDocumentFocus.bind(this));> }> > componentWillUnmount() {> document.title = this.baseTitle;> + document.removeEventListener("blur", this.onDocumentFocus.bind(this));
Shouldn't this be "focus" instead of "blur"? Also this won't work if bind() is
called here: each call to bind() will return a new function. We need to bind()
when constructing the component.
> }> > render() {> -- > 2.41.0
> Nice! Would it be possible to split this patch into two, one to change the> title, and the other one with the focus stuff?
I also thought about that, but came to the conclusion that one would be
useless without the other. It's possible, though. I'll send another revision
soon, should I split the changes?
> I wonder if we want to display the total number of unread buffers, or only> the number of unread highlights. The former sounds a bit spammy:> high-traffic channels have new messages every few seconds...
I believe that the best solution would be to let the user choose. I will look
into it in the next revision.
> Style nit: braces are mandatory.
By the way, have you considered adding an eslint config? ESLint can detect such
stylistic errors, too.
> Here we're updating the title when receiving a new message, but there are> other situations where the number of unread messages changes. It's possible> to grep for "unread" buffer state changes to list these. For instance, when> receiving a MARKREAD message (another client has marked a message as read).
Thanks, I will!
> The buffer can be null here I believe, before logging in.
Makes sense, I will add a check for that.
> This sounds like duplicated code with switchBuffer(). Maybe we can> consolidate the common bits into a single function?> > (Also this doesn't dismiss notifications.)
You're right. I'll fix that.
> Shouldn't this be "focus" instead of "blur"? Also this won't work if bind()> is called here: each call to bind() will return a new function. We need to> bind() when constructing the component.
That's also right. Thanks for catching it.