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

Fix the diff visualizer of connected sessions #674

Merged
merged 5 commits into from
Jun 4, 2023

Conversation

boatbomber
Copy link
Member

@boatbomber boatbomber commented May 11, 2023

Clicking on the "X changes X ago" message opens up a handy diff visualizer to see what those changes were. However, it had quite a few issues that needed fixing.

  • Disconnecting a session with it expanded caused an error as it tried to read the serveSession that no longer exists during the page fade transition. (Disconnecting with synced changes expanded breaks the plugin #671)
    • Resolved by converting to stateful component and holding the serveSession during the lifetime to ensure it can render the last known changes during the fade transition
  • Leaving it open while new changes are synced did not update the visualizer
    • The patch data was piggybacking on an existing binding, which meant that new patches did not trigger rerender.
      • Resolved by converting to state
      • Also made some improvements to that old binding
        • Moved from app to connected page for better organization and separation of duties
        • No more useless updates causing rerenders with no real change
    • Scroll window child component wouldn't actually display the updated visuals
      • Resolved by making major improvements to VirtualScroller
        • Made more robust against edge case states
        • Made smarter about knowing when it needs to refresh

As you can see in this slow motion GIF, it works now.
slowedDemo

@boatbomber
Copy link
Member Author

boatbomber commented May 11, 2023

Whoops, somehow the serveSession is never getting stopped correctly and it continues listening on the port. A memory leak is one thing, but this one is weird.

Edit: Actually, this issue occurs even on master and is unrelated to this PR.

@boatbomber boatbomber marked this pull request as draft May 11, 2023 18:14
@boatbomber boatbomber marked this pull request as ready for review May 11, 2023 18:22
@Kampfkarren Kampfkarren enabled auto-merge (squash) June 4, 2023 05:46
@Kampfkarren Kampfkarren disabled auto-merge June 4, 2023 05:46
@Kampfkarren Kampfkarren merged commit 6542304 into rojo-rbx:master Jun 4, 2023
@boatbomber boatbomber deleted the fix-connected-index-nil branch June 4, 2023 05:46
Dekkonot pushed a commit to UpliftGames/rojo that referenced this pull request Jan 11, 2024
Clicking on the "X changes X ago" message opens up a handy diff
visualizer to see what those changes were. However, it had quite a few
issues that needed fixing.

- Disconnecting a session with it expanded caused an error as it tried
to read the serveSession that no longer exists during the page fade
transition. (rojo-rbx#671)
- Resolved by converting to stateful component and holding the
serveSession during the lifetime to ensure it can render the last known
changes during the fade transition
- Leaving it open while new changes are synced did not update the
visualizer
- The patch data was piggybacking on an existing binding, which meant
that new patches did not trigger rerender.
    - Resolved by converting to state
    - Also made some improvements to that old binding
- Moved from app to connected page for better organization and
separation of duties
      - No more useless updates causing rerenders with no real change
- Scroll window child component wouldn't actually display the updated
visuals
    - Resolved by making major improvements to VirtualScroller
      - Made more robust against edge case states
      - Made smarter about knowing when it needs to refresh

As you can see in this slow motion GIF, it works now.

![slowedDemo](https://github.com/rojo-rbx/rojo/assets/40185666/c9fc8489-72a9-47be-ae45-9c420e1535d4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants