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.

Sign up for GitHub

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

Jump to bottom

Update core bindings to Yrs v0.18 #94

Merged
merged 75 commits into from
Jun 13, 2024
Merged

Update core bindings to Yrs v0.18 #94

merged 75 commits into from
Jun 13, 2024

Conversation

LSViana
Copy link
Collaborator

@LSViana LSViana commented Mar 23, 2024

Closes #91.

This PR is a WIP. There are some TODO comments in the code to be checked.

@LSViana
feat: introduce SubscriptionChannel to bind yunobserve()
e3ed5e9
@LSViana
feat: update EventSubscriber to match `SubscriptionChannel.Unobserv…
117872e 
…e()`
@LSViana
feat: update Array to use SubscriptionChannel.Unobserve
c375712
@LSViana
feat: update Branch to use SubscriptionChannel.Unobserve
aa521ee
@LSViana
feat: update Map to use SubscriptionChannel.Unobserve
1ff4382 
feat: update `Map` to use `SubscriptionChannel.Unobserve`
@LSViana
feat: update Text to use SubscriptionChannel.Unobserve
f67d5ca
@LSViana
feat: update XmlElement to use SubscriptionChannel.Unobserve
1f70710
@LSViana
feat: update XmlText to use SubscriptionChannel.Unobserve
81baa39
@LSViana
feat: update UndoManager to use SubscriptionChannel.Unobserve
8efa594
@LSViana
feat: update Doc to use SubscriptionChannel.Unobserve
9efa7a6
@LSViana
test: dispose subscription on tests for Text.ObserveDeep()
1f9415d
@LSViana
style: fix access modifier order in Branch
e99603c
@LSViana
fix: remove unnecessary null conditional operator from `XmlText.Forma…
fbc9c06 
…t()`
@LSViana
feat: add the OutputTag.XmlFragment constant
5567037
@LSViana
feat: update EventBranchTag to include XmlFragment and synchroniz…
27e516c 
…e with `BranchKind`
@LSViana
feat: synchronize BranchKind with OutputTag
55c6481
@LSViana
feat: introduce XmlFragment and XmlFragmentEvent
83aae2d
@LSViana
feat: update XmlElement and XmlElementEvent to inherit from their…
84a04fb 
… `XmlFragment*` counterparts
@LSViana
feat: update Doc and DocChannel to only allow getting root-level …
90a9a2e 
…`XmlFragment`
@LSViana
feat: update Transaction to only allow getting root-level `XmlFragm…
1be876a 
…ent`
@LSViana
feat: update EventBranch to expose XmlFragmentEvent
7ef9c0d
@LSViana
test: update existing unit tests to use root-level XmlFragment when…
fa0c563 
… needed
@LSViana
test: introduce unit tests for XmlFragment
fee50a1
@LSViana
test: introduce unit tests for ObserveDeep() in XmlFragment
0c4fee3
@LSViana
feat: use exact type comparison to prevent inheritance from allowing …
b3ebc36 
…multiple events to be exposed in `EventBranch`
@LSViana
feat: drop BranchChannel.WriteTransaction()
77173f7
@LSViana
feat: update Branch.WriteTransaction() to use Doc.WriteTransaction()
88c8a9e
@LSViana
chore: fix typos in Branch and Doc
a7a70fd
@LSViana
feat: drop BranchChannel.ReadTransaction()
8d4299f
@LSViana
feat: update Branch.ReadTransaction() to use Doc.ReadTransaction()
98ac727
@LSViana
Copy link
Collaborator Author

LSViana commented May 1, 2024

@SebastianStehle That's great! Given that, I propagated those changes to all other Branch types.

Can you check these latest changes in the classes of the YDotNet project, please?

@LSViana LSViana force-pushed the 91-update-core-bindings-to-yrs-v018 branch from 722c8be to 33a37b5 Compare May 1, 2024 04:16
@LSViana
Copy link
Collaborator Author

LSViana commented May 1, 2024

I just pushed some extra commits to fix all unit tests (including some API changes to UndoEvent to mirror changes from yffi).

The macOS tests are failing for some reason but I can check it later.

@LSViana
Copy link
Collaborator Author

LSViana commented May 6, 2024

@SebastianStehle If you have time, can you please check the latest changes mentioned in my previous comment?

SebastianStehle
SebastianStehle reviewed May 6, 2024
View reviewed changes
Copy link
Collaborator

@SebastianStehle SebastianStehle left a comment

Choose a reason for hiding this comment

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

Only some minor things.

YDotNet/Document/Types/Branches/Branch.cs Outdated Show resolved Hide resolved
YDotNet/Document/Types/Branches/Branch.cs Outdated Show resolved Hide resolved
YDotNet/Document/Types/Branches/Branch.cs Show resolved Hide resolved
YDotNet/Document/Types/Branches/Branch.cs Outdated Show resolved Hide resolved
YDotNet/Document/Types/Events/EventBranch.cs Show resolved Hide resolved
@LSViana
Copy link
Collaborator Author

LSViana commented May 13, 2024

@SebastianStehle I checked and replied to your latest comments.

I'm not sure I fully understood all of your questions so, please, feel free to push code changes for details that you see are obvious to improve.

@LSViana
Copy link
Collaborator Author

LSViana commented May 25, 2024 •
edited
Loading

@SebastianStehle I just resolved all the latest comments that were pending. I'd just like to confirm with you if everything here is good to be merged and released, what do you think?

I'll also fix the failing macOS tests before merging the PR.

By the way, I've not been very active in the repository lately but whenever I find time, I'll try to contribute with the library's core as we agreed on before. 🙂

@SebastianStehle
Copy link
Collaborator

Yes, it is. I would like to know whether it is still needed to keep a reference to the document everywhere. But we can solve this after the PR.

@LSViana LSViana reopened this May 26, 2024
@SebastianStehle SebastianStehle mentioned this pull request May 31, 2024
Closed
@LSViana
Copy link
Collaborator Author

LSViana commented Jun 7, 2024 •
edited
Loading

@SebastianStehle I just realized a few unit tests have been failing because the GitHub Action runner images use .NET 8 by default.

We have two options from here:

  1. Manually configure them to use .NET 7 (like this)
  2. Update the projects of the solution to .NET 8

Bumping a whole .NET version looks like a major change, so I'm leaning towards option #1 but option #2 is something we'll need to do at some point anyway.

This is the last detail before I can merge this PR. What do you think?

@LSViana
Copy link
Collaborator Author

LSViana commented Jun 7, 2024

@SebastianStehle Also, can I get your approval for the PR? It's still marked as "requested changes". 🙂

@LSViana LSViana self-assigned this Jun 7, 2024
@LSViana
Copy link
Collaborator Author

LSViana commented Jun 13, 2024

I'm going to merge this PR regardless of the pending approval.

@LSViana LSViana merged commit 3904b23 into main Jun 13, 2024
4 checks passed
@LSViana LSViana deleted the 91-update-core-bindings-to-yrs-v018 branch June 13, 2024 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SebastianStehle SebastianStehle SebastianStehle requested changes

Assignees

@LSViana LSViana

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update core bindings to Yrs v0.18
2 participants
@LSViana @SebastianStehle