Go to move which sender was looking at when message was sent in broadcasts #15570

Carbrex · 2024-06-20T15:46:27Z

#15029
I am not sure if this is the right way to call these functions using site.analysis.study... as I don't see it any other place in the ui code.
Screencast from 20-06-24 07:35:58 PM IST.webm

Include only chapterid and ply in chat messages Fix move not shown in title of chat msg Remove comments pnpm format

…5029

schlawg · 2024-06-20T16:47:28Z

ui/chat/src/ctrl.ts

    if (text == 'You too!' && !this.data.lines.some(l => l.u != this.data.userId)) return false;
    if (text.length > 140) {
      alert('Max length: 140 chars. ' + text.length + ' chars used.');
      return false;
    }
+
+    if (site.analysis?.study?.relay && !site.analysis.study.relay.tourShow()) {


What you have is fine.

I (personally) would not use site.analysis from chat module. Instead, I'd add a generic interface to chat/src/interfaces.ts (i.e. CustomChatHandler but with a better name), or just change ChatPlugin's existing members to optional and add two or three new bits. example:

interface ChatPlugin { ... prepareMsg?: (msg: string) => string; // embeds whatever special shit the // handler wants to embed right before sending a chat cleanMsg?: (msg: string) => string; // strips any special embedded shit // shit from a chat, used when rendering click?: (msg: string) => void; // chat module calls this with the whole // chat message, handler parses and navigates to chapter/ply if it can }

Then all of this code you have here can go in relay module, except for the bits that call opts.plugin?.prepareMsg (prior to sending a chat), cleanMsg (when creating the snab vnode to render one), and click (in a click handler).

There should be a bit in relayTourView where you can create the object that implements these CustomChatHandler/ChatPlugin functions. You would pass this object to the chat module initializer via opts by way of makeChat.

But if this doesn't make sense to you, or you'd rather not do it - feel free to ignore as you've already got it working.

* remove site.analysis and move the msg logic to analyse module * Remove comments and logs

Carbrex · 2024-07-06T20:38:26Z

15570.webm

I have removed site.analysis and moved the code to analysis module. Is this correct?

Carbrex · 2024-07-06T20:40:44Z

ui/analyse/src/view/components.ts

+      if (segs.length == 3) {
+        const [_, chapterId, ply] = segs;
+        ctrl.study.setChapter(chapterId);
+        setTimeout(() => ctrl.jumpToMain(parseInt(ply)), 100);


It doesn't take to the correct move for some reason without the setTimeout.

Probably because there's network in between. The 100ms hack will not always work depending on who uses it and where from.

Carbrex · 2024-07-07T08:03:27Z

It doesn't work if the message is close to the maxLength(140) chars

ornicar · 2024-07-07T09:57:48Z

It also doesn't work if the ID has enough uppercase letter to trigger the shouting detection

ornicar · 2024-07-07T09:59:30Z

The new metadata (separators+ids) will also end up in the moderator tools, where they're not handled.

…t include metadata to calculate isShouting

Carbrex · 2024-07-07T19:37:08Z

Is it ok now?

ornicar · 2024-07-08T09:43:32Z

ui/analyse/src/study/relay/chatHandler.ts

+          }
+        };
+
+        waitForLoadingAndJump();


Sorry but I think this is terrible.

Instead, study.setChapter should return a promise that the caller can use.

Oh yes, that makes much more sense.

Carbrex · 2024-07-09T17:03:12Z

ui/analyse/src/study/studyCtrl.ts

+    if (this.vm.nextPly) {
+      this.ctrl.jumpToMain(this.vm.nextPly);
+      this.vm.nextPly = undefined;
+    } else {
+      // path could be gone (because of subtree deletion), go as far as possible
+      this.ctrl.userJump(this.ctrl.tree.longestValidPath(nextPath));
+    }


I tried to return promise from setChapter but that was too tough for me. What about this?

ornicar · 2024-07-10T07:06:32Z

modules/common/src/main/String.scala

-          case LOWERCASE_LETTER => i - 1
-          case _                => i
-      } > 0
+      if text.contains('\ue666') then


No. The broadcast encoding logic must not spill over unrelated section of the code, such as this one.

isShouting must not even assume it's working on a chat message.

What is your suggested way? Do I add 128 to ascii value of each character of chapterId in encodeMsg and then get back the chapterId by subtracting 128? So the chapterId doesn't affect the shouting mechanism.

let encodedChapterId = ''; for (let i = 0; i < chapterId.length; i++) { encodedChapterId += String.fromCharCode(chapterId.charCodeAt(i) + 128); } chapterId = encodedChapterId;

Something like this?

isShouting should only be applied to the text that was actually typed by the user.

ornicar · 2024-07-10T09:41:32Z

Let's face it, the whole thing is a hack, as we're stuffing metadata into a string that is supposed to contain user-typed text.
As a result every part of the code that treats the string as what it's supposed to be (user text) will be broken, and require more hacks to work around the embedded metadata.

isShouting is just the beginning, there are more, such as spam detection, garbage detection and various moderation tools, and others I don't have in mind yet.

Carbrex · 2024-07-10T12:08:13Z

Yes, I agree. Should I try to send chapterId and ply along with the user msg in site.pubsub.emit('socket.send', 'talk', text); this part and save it in the db and not encode in the actual user text?

ornicar · 2024-07-11T09:24:03Z

That will add non-trivial code to lila-ws and lila, and require a new storage format for these messages.

At the moment I don't see a simple and clean implementation for that feature. I like to keep things as simple and clean as possible, and I don't think the use case is important enough to justify the extra complexity.

It's never fun to turn down new features, and I understand that's it's frustrating to throw away work that's already done. I've done it myself many times when realizing that there are no simple solutions to the problem I'm trying to solve.

Carbrex · 2024-07-11T09:32:09Z

Ok, thanks for taking the time to review the pr and making necessary changes! It's a shame we couldn't work out a solution. But I am happy that I started understanding more ui code in the process.
Thanks again, ornicar and schlawg for your help.

schlawg · 2024-07-12T15:33:06Z

Your work is not wasted, and we can work out a solution.

Thibault is right that this deserves to be separate from the chat text, but I disagree that use cases don't support the work required to add chat metadata. The idea was we start with broadcasts and then make it work with studies more generally.

Linking study chat to board states is arguably a friendlier annotation mechanism than provided by the analysis UI itself. For example, a teacher creates a series of informative chat messages linked to various board states. Linking chat is not just about revealing what JoeBob97 was freaking out about when looking at yesterday's broadcast chat.

I'm sorry that I missed this exchange, and I'm sad to see this PR closed. I should not have recommended we fly under the radar and embed directly into the text. I hope db.msg_msg gets a general meta sub-document or something to support the linking I've described later this year. If you'd prefer not to take that on, I do expect the work you've done in this PR will be useful at that point.

ornicar · 2024-07-12T16:05:09Z

For example, a teacher creates a series of informative chat messages linked to various board states

This use case is covered by the ability to add comments to study positions, I think.

A chat line meta object is tempting, and would simplify (while adding its own complexity) existing hacks to sneak the title and status of the user into its message.

The reason things are the way they currently are, is that we have about a billion of chat messages stored in the database. It uses 53GB on each database replica. And that's with storing each line as a single string:

UserName Hello world!

with some hacks for metadata:

  private val UserLineRegex =
    """(?s)([\w-~]{2,}+)([ """ + s"$trollChar$deletedChar$patronChar$flairChar$patronFlairChar" + """])(.++)""".r

Storing lines as an object instead of a string would more than double the storage size.
Unless we keep both formats and only use objects when necessary...

schlawg · 2024-07-12T16:30:47Z

This use case is covered by the ability to add comments to study positions, I think.

It is for contributors, but comments have different discoverability being tucked away in the variations/chapters. Allowing chat as an additional navigation layer could be useful. Maybe very useful.

Carbrex added 2 commits June 20, 2024 21:11

Update chatCtrl to include round and game information in chat messages

221cda1

Include only chapterid and ply in chat messages Fix move not shown in title of chat msg Remove comments pnpm format

Merge branch 'lichess-org:master' into broadcast-chat-game-navigate-1…

3547b56

…5029

schlawg approved these changes Jun 20, 2024

View reviewed changes

Carbrex added 3 commits July 6, 2024 23:13

remove site.analysis and move code to analyse module (#2)

70c82d3

* remove site.analysis and move the msg logic to analyse module * Remove comments and logs

remove comments

2caefd0

Fix lint errors

f7155e1

Carbrex force-pushed the broadcast-chat-game-navigate-15029 branch from 9fc0e71 to f7155e1 Compare July 6, 2024 17:58

Carbrex added 3 commits July 7, 2024 00:53

Change function names and fix ui when report button is also visible

233373e

Merge branch 'master' into broadcast-chat-game-navigate-15029

3d356b6

Fix jump not taking to correct move if move is of different chapter

4bf6e75

Carbrex commented Jul 6, 2024

View reviewed changes

Carbrex marked this pull request as ready for review July 6, 2024 20:41

Merge branch 'master' into broadcast-chat-game-navigate-15029

13636b7

ornicar added 4 commits July 7, 2024 11:38

move relay chat code to the relay dir

3e43346

fix repeatedly hardcoded relay chat separator

0031e1d

ui/chat cannot know about relay and hardcode its separator

4c8aced

move more relay code to the relay dir

02ed544

Carbrex marked this pull request as draft July 7, 2024 12:31

Carbrex added 2 commits July 7, 2024 20:00

Jump to correct move only if chapter has loaded

accf84c

Don't add metadata if text length exceeds 140 after encoding and don'…

9ff3d6e

…t include metadata to calculate isShouting

Carbrex marked this pull request as ready for review July 7, 2024 19:37

ornicar reviewed Jul 8, 2024

View reviewed changes

Use studyvm to go to a given move after chapter is fetched

d124f2b

Carbrex commented Jul 9, 2024

View reviewed changes

ornicar reviewed Jul 10, 2024

View reviewed changes

ornicar added 3 commits July 10, 2024 09:17

Merge branch 'master' into broadcast-chat-game-navigate-15029

cdadf9a

ui/common throttlePromise now returns a Promise for chaining

eda345a

pnpm lint fixes

1a77d80

Carbrex closed this Jul 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go to move which sender was looking at when message was sent in broadcasts #15570

Go to move which sender was looking at when message was sent in broadcasts #15570

Carbrex commented Jun 20, 2024

schlawg Jun 20, 2024

Carbrex commented Jul 6, 2024 •

edited

Loading

Carbrex Jul 6, 2024

ornicar Jul 7, 2024

Carbrex commented Jul 7, 2024

ornicar commented Jul 7, 2024 •

edited

Loading

ornicar commented Jul 7, 2024

Carbrex commented Jul 7, 2024

ornicar Jul 8, 2024

Carbrex Jul 8, 2024

Carbrex Jul 9, 2024

ornicar Jul 10, 2024 •

edited

Loading

Carbrex Jul 10, 2024

Carbrex Jul 10, 2024

ornicar Jul 10, 2024

ornicar commented Jul 10, 2024 •

edited

Loading

Carbrex commented Jul 10, 2024 •

edited

Loading

ornicar commented Jul 11, 2024

Carbrex commented Jul 11, 2024

schlawg commented Jul 12, 2024

ornicar commented Jul 12, 2024 •

edited

Loading

schlawg commented Jul 12, 2024

Go to move which sender was looking at when message was sent in broadcasts #15570

Go to move which sender was looking at when message was sent in broadcasts #15570

Conversation

Carbrex commented Jun 20, 2024

Choose a reason for hiding this comment

Carbrex commented Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carbrex commented Jul 7, 2024

ornicar commented Jul 7, 2024 • edited Loading

ornicar commented Jul 7, 2024

Carbrex commented Jul 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ornicar Jul 10, 2024 • edited Loading

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

ornicar commented Jul 10, 2024 • edited Loading

Carbrex commented Jul 10, 2024 • edited Loading

ornicar commented Jul 11, 2024

Carbrex commented Jul 11, 2024

schlawg commented Jul 12, 2024

ornicar commented Jul 12, 2024 • edited Loading

schlawg commented Jul 12, 2024

Carbrex commented Jul 6, 2024 •

edited

Loading

ornicar commented Jul 7, 2024 •

edited

Loading

ornicar Jul 10, 2024 •

edited

Loading

ornicar commented Jul 10, 2024 •

edited

Loading

Carbrex commented Jul 10, 2024 •

edited

Loading

ornicar commented Jul 12, 2024 •

edited

Loading