-
Notifications
You must be signed in to change notification settings - Fork 14
feat: Replace Shared Space Manager #6576
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
base: dev
Are you sure you want to change the base?
Conversation
davidejensen
left a comment
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.
A MA ZING! Left a couple of minor comments/questions
Explorer/Assets/DCL/Chat/_Refactor/ChatCommands/OpenConversationCommand.cs
Show resolved
Hide resolved
Explorer/Assets/DCL/Communities/CommunitiesBrowser/Commands/GoToStreamCommand.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/DCL/Friends/UI/FriendPanel/FriendsPanelController.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/DCL/Friends/UI/FriendPanel/Sections/Friends/FriendSectionController.cs
Outdated
Show resolved
Hide resolved
Explorer/Assets/DCL/Infrastructure/MVC/WindowsStackManager/IWindowsStackManager.cs
Outdated
Show resolved
Hide resolved
DafGreco
left a comment
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.
✔️ PR reviewed and approved by QA on Macos following instructions playing both happy and un-happy path
Regressions for this ticket had been performed in order to verify that the normal flow is working as expected:
- [ ✔️] Backpack and wearables in world
- [ ✔️] Emotes in world and in backpack
- [ ✔️] Teleport with map/coordinates/Jump In
- [ ✔️] Chat and multiplayer
- [ ✔️] Profile card
- [✔️ ] Skybox
- [ ✔️] Settings
PR:
- All points from the PR have been checked on macos and they are working properly on this side :
Sidebar Buttons should open all corresponding windows and update the icon properly (selected/unselected) ✅
The Friends Panel should Hide the Chat ✅
Opening any Fullscreen window (for example Explore Panel or Communities Browser) should disable the chat ✅
Hotkeys should open all the corresponding windows and the icon on the sidebar should also reflect that. ✅
Enter key should focus and unfocus the chat (if there is no text in the input box) ✅
Opening private conversations from any menu (for example, Friends panel, Profile view, Members view on Communities) should open the chat, open the corresponding conversation and focus the input box if possible ✅
Starting a call from any menu should open the chat, open the conversation with the user, focus the input box (if possible) and try to start the call. ✅
Opening Communities conversations through any Communities window should open the chat, open the corresponding conversation and focus the input box (if possible) ✅
Opening any Community Stream (either starting it or joining) should open the chat and show the voice chat interface. If its a community we dont belong to, it wont open the community conversation, as we cant see it. ✅
No new issues have been found during this checks on this branch
…bled or disabled state
Pull Request Description
What does this PR change?
What it DOESNT change should I say?
This PR handles a deep wound on our pride which was the complexity introduced by the SharedSpaceManager. After the refactor of the chat it became obvious that the next step was removing this dependency that caused so much pain when adding any new panel to the sidebar or any logic related to the chat in general.
I also took the opportunity to fix many warnings in the files I edited, as we discussed in dev meetings, reducing the warnings amount and sticking to the code style was a yes even if it didnt relate directly to our PR.
The main change is obviously, the removal of all logic on the
SharedSpaceManager, to do this, I converted every sidebar panel into POPUPs views. Why? because they should be, they are managed just using the MVC Manager and its predictably and easy to follow.This meant adding controllers to many views that didn't have them and making sure their initialization and disposal was cleanly done instead of the mishmash of things we had there.
I also streamlined how the sidebar icons get selected/deselected, using specifically the awaiter of the mvcManager to know when the view was closed. This made it SO easy to track.
The only exception to these changes is the Friends Panel which became a FULLSCREEN view. Why? because it needs to hide the Chat and ALL Fullscreen views hide the chat. The rest of the logic is the same as before.
Other changes included removing a lot of asmdefs and replacing them by asmrefs pointing to (for now) the Social asmdef, as we are trying to coalesce all UI player facing features into one Assembly Def. This saved me from several circular refs I faced during the long gestation of this feature.
Another big change that was added was a way to close the whole hierarchy of windows to go back to the chat. This meant the creation of a new method in the
MVCManagercalledCloseAllNonPersistentwhich does as it names say, close all non persistent views, including all POPUPs, any FULLSCREEN and OVERLAY that might be open. In this way we return to have only persistent views open and we can then send events to the chat to do magics.The communication with the chat remains now using events which I also streamlined, removing the duplicated buses we had for chatEvents. So for example, we are in a popup on top of the communities browser fullscreen view, when we click on the chat button, we close all views, focus the chat and then open the community conversation.
Other than this, I made a big cleanup of params sent from the DynamicWorldContainer to the plugins, removing most of those that can be accessed through singletons particularly for FeatureFlags.
Test Instructions
This feature introduces changes to how the Chat is opened from any other window, as well as how the Sidebar works.
This means we need to test the following: