Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(presence-tracker): Use presence package #23020

Merged
merged 87 commits into from
Jan 23, 2025

Conversation

tylerbutler
Copy link
Member

@tylerbutler tylerbutler commented Nov 7, 2024

Summary of changes

The presence-tracker example has been updated to use the presence package. It now uses LatestValueManagers for mouse and focus tracking.

Each client also has a slider that controls the allowable latency of the mouse updates. This enables quick demos of the differences in behavior with higher latencies as well as demonstrating runtime adjustments of the allowable latency.

Known issues:

  • The failing test checks that when a second client joins the first client's client list is updated. This failure seems to indicate that the multi-client setup is not working correctly. However, given that things are working in manual testing, I suggest merging this with a follow-up task to figure out the multi-client piece. Follow-up issue: AB#28502

@github-actions github-actions bot added area: examples Changes that focus on our examples dependencies Pull requests that update a dependency file base: main PRs targeted against main branch labels Nov 7, 2024
Copy link
Collaborator

@msfluid-bot msfluid-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Coverage Summary

No packages impacted by the change.


Baseline commit: 5d813e5
Baseline build: 310291
Happy Coding!!

Code coverage comparison check passed!!

@jason-ha
Copy link
Contributor

jason-ha commented Nov 7, 2024

For reference here is branch where I updated Presence to have legacy support and then migrated example's MouseTracker. (I left FocusTracker alone just so there were parallel examples of Signals versus Presence.)
https://github.com/jason-ha/FluidFramework/tree/presence/encapsulated-support
key commit is feat(client): Legacy experimental Presence support
with example use

@@ -2,9 +2,17 @@

**_This demo is a work-in-progress_**

**Presence Tracker** is an example that demonstrates how transient state of audience members can be tracked among other audience members using signals. It does so using fluid-framework's `FluidContainer`, `IServiceAudience`, and `Signaler`.
**Presence Tracker** is an example that demonstrates how the @fluidframework/presence package can be used to share data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I always recommend line-breaking along sentence boundaries for ease of reviewing future content changes.

https://github.com/microsoft/FluidFramework/wiki/Markdown-Best-Practices#line-breaks-along-sentence-boundaries

Comment on lines 17 to 19
// Define the schema of our Container.
// This includes the DataObjects we support and any initial DataObjects we want created
// when the Container is first created.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: any reason this shouldn't be a TSDoc comment?

/**
* A states workspace that the FocusTracker will use to share focus states with other session clients.
*/
readonly statesWorkspace: PresenceStates<any>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any? {} with a lint disable is better than any.
At one point I think I had a type for empty states (PresenceStates<{}>) because we get {} complaints. Then most cases disappeared and I removed it as likely hard to discover and easy enough for any users to declare themselves.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This confuses me a bit. What does PresenceStates<{}> represent? I thought it was "an empty workspace," but the trackers accept a workspace that already has a state key defined. Does PresenceStates<{}> instead represent "a workspace with unknown states?"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried unknown but that wasn't permitted type-wise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back to {}.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{} is the correct type. It is a workspace with no defined entries. In this case it is a lint rule that is overzealous. (There are special treatments for {} in TypeScript, but in this case, I think we know what we are doing.)

Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170508 links
    1603 destination URLs
    1842 URLs ignored
       0 warnings
       0 errors


@tylerbutler tylerbutler merged commit 17da2df into microsoft:main Jan 23, 2025
43 checks passed
@tylerbutler tylerbutler deleted the presence-tracker-revamp branch January 23, 2025 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: examples Changes that focus on our examples base: main PRs targeted against main branch dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants