-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Real-time Collaboration: Add user and selection information to awareness #74728
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
Changes from 12 commits
816e55c
f81ed74
6e3eb80
2836b7e
cf05627
2c1d5cf
8772cb2
979e04a
e68cfde
77cad8f
0e12107
1b19ef1
215942c
4f37149
e9cc4bc
63a11d7
71c7741
f6a7cc0
45c439d
9200272
a2f7a09
d1066e9
96c6e6a
40bba56
9fb1542
266fd2c
2d6ca82
335f32f
4e6bff0
731eea5
e8af246
daa079a
fa964ed
ec820f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -192,7 +192,7 @@ export const getEntityRecord = | |
| recordWithTransients, | ||
| { | ||
| // Handle edits sourced from the sync manager. | ||
| editRecord: ( edits ) => { | ||
| editRecord: ( edits, options = {} ) => { | ||
| if ( ! Object.keys( edits ).length ) { | ||
| return; | ||
| } | ||
|
|
@@ -206,6 +206,7 @@ export const getEntityRecord = | |
| meta: { | ||
| undo: undefined, | ||
| }, | ||
| options, | ||
| } ); | ||
| }, | ||
| // Get the current entity record (with edits) | ||
|
|
@@ -232,6 +233,9 @@ export const getEntityRecord = | |
| key | ||
| ); | ||
| }, | ||
| // Get the current user. | ||
| getCurrentUser: async () => | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kept this here in the existing handlers. I like this place personally. We could rename it from We can change it if we want to later on. |
||
| await resolveSelect.getCurrentUser(), | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -6,9 +6,11 @@ import type * as Y from 'yjs'; | |||||
| /** | ||||||
| * Internal dependencies | ||||||
| */ | ||||||
| import type { ObjectID, ObjectType } from '../types'; | ||||||
| import type { ObjectID, ObjectType, RecordHandlers } from '../types'; | ||||||
| import type { AwarenessState } from './awareness-state'; | ||||||
| import { PostEditorAwarenessState } from './post-editor-awareness-state'; | ||||||
| import type { UserInfo, WordPressUserInfo } from './awareness-types'; | ||||||
| import { getBrowserName, getNewUserColor } from '../user-utils'; | ||||||
|
|
||||||
| const awarenessInstances: Map< string, AwarenessState > = new Map(); | ||||||
|
|
||||||
|
|
@@ -26,6 +28,28 @@ function getAwarenessInstance( | |||||
| return awarenessInstances.get( getAwarenessId( objectType, objectId ) ); | ||||||
| } | ||||||
|
|
||||||
| function getUserInfo( | ||||||
| awareness: AwarenessState, | ||||||
| wpUser: WordPressUserInfo | ||||||
| ): UserInfo { | ||||||
| const states = awareness.getStates(); | ||||||
| // TODO: There is a timing issue here. The other users aren't yet synced, and as a result the same color could be assigned to multiple users. | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chriszarate - this is the timing issue that I mentioned. The other users aren't yet synced and so the colour that's picked doesn't account for someone else using that. Eventually they do sync so it's not the end of the world. We can just have this be changed when a user is added. Besides this, I've not noticed any problems. My hunch is that, this was always there but due to the way we were instantiating it earlier it wasn't being run into. There was just enough time for that first sync to finish.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd guess that the "workaround" for the current awareness implementation is called here, where we I remember this being necessary before syncing: // getCurrentUser() returns an empty user object for a short time after load.
// In that case, wait and try again.Have you seen that happen in testing? Either way, a short delay to set up awareness after connecting may be an easy workaround.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tested the current vip-real-time-collaboration plugin awareness implementation locally, and it looks like we typically run the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had tried the timeout approach and it wasn't working consistently. That's why I mentioned that this was always there but due to the way we instantiated awareness it wasn't being run into. I'm honestly not a fan of timeouts because they are arbitrary, and don't exactly solve the core problem. I think the better solution would be to identify the properties that can be impacted by this quirk. Then, as users are added ( That's why for now, I marked this as a TODO so it could be revisited in another PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noting that I tries this again and it didn't work unfortunately.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay! Maybe we could listen for our first "update" event before setting up the local awareness (ideally) or fallback to a timeout in order to make it work consistently with and without other users present. I agree that a timeout alone isn't very robust. I'm fine with pushing this to another PR, since the worst-case seems to be overlapping colors.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's another idea I had in mind that I wanted to experiment with. Gonna punt this to another PR as it's not too urgent at the moment given awareness isn't even used in the UI. |
||||||
| const otherUserColors = Array.from( states.entries() ) | ||||||
| .filter( | ||||||
| ( [ clientId, state ] ) => | ||||||
| state.userInfo && clientId !== awareness.clientID | ||||||
| ) | ||||||
| .map( ( [ , state ] ) => state.userInfo.color ) | ||||||
| .filter( Boolean ); | ||||||
|
|
||||||
| return { | ||||||
| ...wpUser, | ||||||
| browserType: getBrowserName(), | ||||||
| color: getNewUserColor( otherUserColors ), | ||||||
| enteredAt: Date.now(), | ||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Get the post editor awareness instance for the given post ID and post type. | ||||||
| * @param postId Post ID. | ||||||
|
|
@@ -49,32 +73,41 @@ export function getPostEditorAwareness( | |||||
|
|
||||||
| /** | ||||||
| * Create an awareness instance for the given object type and object ID. | ||||||
| * @param objectType Object type. | ||||||
| * @param objectId Object ID. | ||||||
| * @param ydoc Yjs document. | ||||||
| * @param objectType Object type. | ||||||
| * @param objectId Object ID. | ||||||
| * @param ydoc Yjs document. | ||||||
| * @param recordHandlers Record handlers. | ||||||
| * @return Awareness instance. | ||||||
| */ | ||||||
| export async function createAwareness( | ||||||
| objectType: ObjectType, | ||||||
| objectId: ObjectID | null, | ||||||
| ydoc: Y.Doc | ||||||
| ydoc: Y.Doc, | ||||||
| recordHandlers: RecordHandlers | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than passing in The record handlers are very powerful / dangerous and ideally should not be passed around.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think one of my previous commits shows me going back and forth with this for the same reason. I'm fine just passing in the |
||||||
| ): Promise< AwarenessState | undefined > { | ||||||
| if ( objectId && objectType.startsWith( 'postType/' ) ) { | ||||||
| const awareness = new PostEditorAwarenessState( ydoc ); | ||||||
| awareness.setUp(); | ||||||
|
|
||||||
| // TODO: Is there still a need to memoize the current user? | ||||||
| const currentUser = await recordHandlers.getCurrentUser(); | ||||||
|
ingeniumed marked this conversation as resolved.
Outdated
|
||||||
| const userInfo = getUserInfo( awareness, currentUser ); | ||||||
|
|
||||||
| awareness.setUp( recordHandlers, userInfo ); | ||||||
| awarenessInstances.set( | ||||||
| getAwarenessId( objectType, objectId ), | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This allows for the |
||||||
| awareness | ||||||
| ); | ||||||
|
|
||||||
| return awareness; | ||||||
| } | ||||||
|
|
||||||
| return undefined; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Set the current user's connection status in the awareness instance for the given object type and object ID. | ||||||
| * | ||||||
| * TODO: Use this in a generic way with each provider so it doesn't need to be exported externally. | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chriszarate - this was from the previous PR that I didn't add a TODO for. We need this for the connected vs disconnected users within the debug data as well as within the UI. I tried to play around with setting it on a provider but I think due to the generic way we set it up I wasn't able to. I've punted it to a follow up PR instead. |
||||||
| * | ||||||
| * @param objectType Object type. | ||||||
| * @param objectId Object ID. | ||||||
| * @param isConnected Connection status. | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import { Awareness } from 'y-protocols/awareness'; | ||
|
|
||
| import { getRecordValue } from '../utils'; | ||
| import type { SelectionState } from '../selection-utils'; | ||
|
|
||
| /** | ||
| * Extended Awareness class with typed state accessors. | ||
|
|
@@ -37,14 +38,39 @@ export class TypedAwareness< State extends BaseState > extends Awareness { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * This base user info is a subset of the User interface from @wordpress/core-data. | ||
| * | ||
| * In order to avoid circular dependencies, we define it here instead of importing | ||
| * the User interface from @wordpress/core-data. | ||
| * | ||
| * The avatarUrl is an additional field that is not part of the User interface. | ||
| */ | ||
| export interface WordPressUserInfo { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is different from the existing RTC code. I had to re-define it due to the circular dependency it would cause. |
||
| id: number; | ||
| name: string; | ||
| slug: string; | ||
| avatar_urls: Record< string, string >; | ||
| } | ||
|
|
||
| /** | ||
| * The user info interface extends the base user info with additional fields used for presence | ||
| * indicators. | ||
| */ | ||
| export interface UserInfo extends WordPressUserInfo { | ||
| browserType: string; | ||
| color: string; | ||
| enteredAt: number; | ||
| } | ||
|
|
||
| /** | ||
| * This base state represents the presence of the user. We expect it to be | ||
| * extended to include additional state describing the user's current activity. | ||
| * This state must be serializable and compact. | ||
| * | ||
| * TODO: Add in the user information. | ||
| */ | ||
| export interface BaseState {} | ||
| export interface BaseState { | ||
| userInfo: UserInfo; | ||
| } | ||
|
|
||
| /** | ||
| * An enhanced state includes additional metadata about the user's connection | ||
|
|
@@ -56,15 +82,44 @@ export type EnhancedState< State extends BaseState > = State & { | |
| isMe: boolean; | ||
| }; | ||
|
|
||
| /** | ||
| * A block selection object. | ||
| * | ||
| * In order to avoid circular dependencies, we define it here instead of importing | ||
| * the WPBlockSelection interface from @wordpress/editor. | ||
| */ | ||
| export type WPBlockSelection = { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is different from the existing RTC code. I had to re-define it due to the circular dependency it would cause.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re-defining types is something we seem to do a lot. It doesn't seem great but I don't have a solution. Commenting to hear if @chriszarate does. |
||
| /** | ||
| * A block client ID. | ||
| */ | ||
| clientId: string; | ||
| /** | ||
| * A block attribute key. | ||
| */ | ||
| attributeKey: string; | ||
| /** | ||
| * An attribute value offset, based on the rich | ||
| * text value. See `wp.richText.create`. | ||
| */ | ||
| offset: number; | ||
| }; | ||
|
|
||
| export type EqualityFieldCheck< | ||
| State extends BaseState, | ||
| FieldName extends keyof State, | ||
| > = ( value1?: State[ FieldName ], value2?: State[ FieldName ] ) => boolean; | ||
|
|
||
| /** | ||
| * The editor state includes information about the user's current selection. | ||
| */ | ||
| export interface EditorState { | ||
| selection: SelectionState; | ||
| } | ||
|
|
||
| /** | ||
| * The post editor state extends the base state with information used to render | ||
| * presence indicators in the post editor. | ||
| * | ||
| * TODO: Add in the presence indicators. | ||
| */ | ||
| export interface PostEditorState extends BaseState {} | ||
| export interface PostEditorState extends BaseState { | ||
| editorState?: EditorState; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to allow for
core-dataandeditorto not be used withinselection-utils.tsfrom the sync package. It also means that the awareness instance is always aware about what post type and post ID it's meant for. There's no need to fetch it and verify if its set or not.