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 Rust timeline identifiers into our own. #3731

Merged
merged 2 commits into from
Feb 4, 2025

Conversation

pixlwave
Copy link
Member

@pixlwave pixlwave commented Feb 3, 2025

In preparation for swiping media in the room screen, this PR replaces all the uses of Rust's EventOrTransactionId and TimelineUniqueId with our own local versions as it seems silly to keep importing the SDK into UI layers just to identify things.

Broken into 2 commits, one for each refactor so it should be easier to review.

@pixlwave pixlwave requested a review from a team as a code owner February 3, 2025 17:01
@pixlwave pixlwave requested review from Velin92 and removed request for a team February 3, 2025 17:01
@pixlwave pixlwave added the pr-misc for other changes label Feb 3, 2025
@pixlwave pixlwave force-pushed the doug/item-identifiers branch from 4b48291 to e4f1329 Compare February 3, 2025 17:02
Copy link

github-actions bot commented Feb 3, 2025

Warnings
⚠️ You seem to have made changes to views. Please consider adding screenshots.

Generated by 🚫 Danger Swift against e4f1329

Copy link

sonarqubecloud bot commented Feb 3, 2025

Copy link

codecov bot commented Feb 3, 2025

Codecov Report

Attention: Patch coverage is 76.52174% with 27 lines in your changes missing coverage. Please review.

Project coverage is 77.53%. Comparing base (10d3925) to head (e4f1329).
Report is 3 commits behind head on develop.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ces/Services/Timeline/TimelineItemIdentifier.swift 48.57% 18 Missing ⚠️
...meline/TimelineController/TimelineController.swift 0.00% 3 Missing ⚠️
...w/TimelineViews/ImageMediaEventsTimelineView.swift 50.00% 1 Missing ⚠️
...Screens/Timeline/TimelineTableViewController.swift 85.71% 1 Missing ⚠️
...View/TimelineItemViews/ImageRoomTimelineView.swift 50.00% 1 Missing ⚠️
...ervices/Room/RoomSummary/RoomSummaryProvider.swift 0.00% 1 Missing ⚠️
...s/Timeline/Fixtures/RoomTimelineItemFixtures.swift 50.00% 1 Missing ⚠️
.../Items/Virtual/TimelineStartRoomTimelineItem.swift 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3731      +/-   ##
===========================================
- Coverage    77.58%   77.53%   -0.05%     
===========================================
  Files          797      797              
  Lines        69319    69329      +10     
===========================================
- Hits         53782    53757      -25     
- Misses       15537    15572      +35     
Flag Coverage Δ
unittests 70.30% <76.52%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Velin92 Velin92 left a comment

Choose a reason for hiding this comment

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

Couldn't we use a typealias of String with some methods on top, instead of defining a struct with a single value?

@pixlwave pixlwave merged commit bad4a8f into develop Feb 4, 2025
13 checks passed
@pixlwave pixlwave deleted the doug/item-identifiers branch February 4, 2025 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-misc for other changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants