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 theTimelineItemIdentifier handling #3418

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

stefanceriu
Copy link
Member

  • stop relying on optional EventOrTransactionIds and be explicit when setting composer modes from the draft service.

@stefanceriu stefanceriu added the pr-misc for other changes label Oct 16, 2024
@stefanceriu stefanceriu requested a review from pixlwave October 16, 2024 11:08
@stefanceriu stefanceriu requested a review from a team as a code owner October 16, 2024 11:08
Copy link

github-actions bot commented Oct 16, 2024

Warnings
⚠️ Some of the commits are missing ticket numbers. Please consider squashing all commits that don't have a tracking number.
⚠️ You seem to have made changes to views. Please consider adding screenshots.

Generated by 🚫 Danger Swift against d357fd9

Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

👏

Would be nice to have our own EventOrTransactionID enum so we have ID everywhere instead of Id but no need right now.

@@ -283,9 +284,14 @@ extension FormatType {
}

enum ComposerMode: Equatable {
enum EditSource {
Copy link
Member

Choose a reason for hiding this comment

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

Could call this EditActionSource if you think it makes the timeline case clearer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think ComposerMode.EditActionSource is clear enough

Comment on lines -75 to -85

extension RoomTimelineControllerProtocol {
func sendMessage(_ message: String,
html: String?,
intentionalMentions: IntentionalMentions) async {
await sendMessage(message,
html: html,
inReplyTo: nil,
intentionalMentions: intentionalMentions)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what was wrong with this, seems like a reasonable thing to use when you don't want to reply?

Copy link
Member Author

Choose a reason for hiding this comment

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

I found them extra to be honest. On the highest level they're both called from the same place and each variant evetually trickles down into the same timeline one.

Comment on lines -115 to -125

extension TimelineProxyProtocol {
func sendMessage(_ message: String,
html: String?,
intentionalMentions: IntentionalMentions) async -> Result<Void, TimelineProxyError> {
await sendMessage(message,
html: html,
inReplyTo: nil,
intentionalMentions: intentionalMentions)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Comment on lines 637 to 641
if case let .event(eventTimelineItem) = item {
switch eventTimelineItem.id {
case .event(_, let identifier):
if identifier == eventOrTransactionID {
return eventTimelineItem.item
}
default:
break
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This might be slightly cleaner:

Suggested change
if case let .event(eventTimelineItem) = item {
switch eventTimelineItem.id {
case .event(_, let identifier):
if identifier == eventOrTransactionID {
return eventTimelineItem.item
}
default:
break
}
}
if case let .event(eventTimelineItem) = item,
case let .event(_, identifier) = eventTimelineItem.id,
identifier == eventOrTransactionID {
return eventTimelineItem.item
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup yup, nice, thank you!

Comment on lines -401 to +386
return SeparatorRoomTimelineItem(id: .init(uniqueID: dateString), text: dateString)
return SeparatorRoomTimelineItem(id: .virtual(uniqueID: uniqueID), text: dateString)
Copy link
Member

Choose a reason for hiding this comment

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

Just want to highlight that this changed from dateString to uniqueID.

Copy link
Member Author

Choose a reason for hiding this comment

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

It did yes, but I think that's correct as the uniqueID is what rust actually uses for that timeline item, no?

@stefanceriu stefanceriu force-pushed the stefan/identifierChanges branch 2 times, most recently from 68527d7 to 290db72 Compare October 16, 2024 11:47
Copy link

codecov bot commented Oct 16, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
679 1 678 0
View the top 1 failed tests by shortest run time
MediaUploadingPreprocessorTests testPortraitMp4VideoProcessing()
Stack Traces | 60s run time
Test exceeded execution time allowance of 1 minute (see `-[XCTestCase executionTimeAllowance]`)

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

…al `EventOrTransactionId`s and be explicit when setting composer modes from the draft service.
@stefanceriu stefanceriu force-pushed the stefan/identifierChanges branch from 290db72 to d357fd9 Compare October 16, 2024 12:23
Copy link

@stefanceriu stefanceriu merged commit 35d49c4 into develop Oct 16, 2024
6 of 7 checks passed
@stefanceriu stefanceriu deleted the stefan/identifierChanges branch October 16, 2024 13:37
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