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 issue on timeline bubbles not showing proper content after decrypt #7397

Merged
merged 4 commits into from
Mar 2, 2023

Conversation

flescio
Copy link
Contributor

@flescio flescio commented Mar 1, 2023

Fixes https://github.com/vector-im/customer-retainer/issues/12

When an event is received but not decrypted properly, a bubble is create in the timeline with a missing content tag.
The event is lately decrypted correctly but the bubble still has a missing tag.
Resetting the Datasource for that room solve the problem.

Copy link
Contributor

@alfogrillo alfogrillo left a comment

Choose a reason for hiding this comment

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

Left minor comments.
LGTM overall!

Comment on lines +155 to +158
mxEventDidDecryptNotificationObserver = [[NSNotificationCenter defaultCenter] addObserverForName:kMXEventDidDecryptNotification object:nil queue:[NSOperationQueue mainQueue] usingBlock:^(NSNotification *notif) {
MXEvent *decryptedEvent = notif.object;
[self->roomDataSourcesToDestroy addObject:decryptedEvent.roomId];
}];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mxEventDidDecryptNotificationObserver = [[NSNotificationCenter defaultCenter] addObserverForName:kMXEventDidDecryptNotification object:nil queue:[NSOperationQueue mainQueue] usingBlock:^(NSNotification *notif) {
MXEvent *decryptedEvent = notif.object;
[self->roomDataSourcesToDestroy addObject:decryptedEvent.roomId];
}];
mxEventDidDecryptNotificationObserver = [NSNotificationCenter.defaultCenter addObserverForName:kMXEventDidDecryptNotification object:nil queue:NSOperationQueue.mainQueue usingBlock:^(NSNotification *notif) {
MXEvent *decryptedEvent = notif.object;
[self->roomDataSourcesToDestroy addObject:decryptedEvent.roomId];
}];

Minor: dot notation should work on static vars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to remain consistent with similar denotation in the same file

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't care much about that.
Writing more readable code is better to me.
I'll leave this up to you in the end. ;)

@@ -156,6 +174,11 @@ - (void)destroy
[[NSNotificationCenter defaultCenter] removeObserver:UIApplicationDidReceiveMemoryWarningNotificationObserver];
UIApplicationDidReceiveMemoryWarningNotificationObserver = nil;
}
if (mxEventDidDecryptNotificationObserver)
{
[[NSNotificationCenter defaultCenter] removeObserver:mxEventDidDecryptNotificationObserver];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[[NSNotificationCenter defaultCenter] removeObserver:mxEventDidDecryptNotificationObserver];
[NSNotificationCenter.defaultCenter removeObserver:mxEventDidDecryptNotificationObserver];

changelog.d/7397.bugfix Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Patch coverage: 11.76% and project coverage change: -0.01 ⚠️

Comparison is base (081615f) 12.15% compared to head (9a93e72) 12.15%.

❗ Current head 9a93e72 differs from pull request most recent head b628db3. Consider uploading reports for the commit b628db3 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7397      +/-   ##
===========================================
- Coverage    12.15%   12.15%   -0.01%     
===========================================
  Files         1643     1643              
  Lines       162265   162282      +17     
  Branches     66630    66638       +8     
===========================================
+ Hits         19729    19731       +2     
- Misses      141890   141905      +15     
  Partials       646      646              
Flag Coverage Δ
uitests 55.32% <ø> (ø)
unittests 6.02% <11.76%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...s/MatrixKit/Models/Room/MXKRoomDataSourceManager.m 28.24% <11.76%> (-1.76%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Flavio Alescio and others added 2 commits March 1, 2023 14:58
@sonarcloud
Copy link

sonarcloud bot commented Mar 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

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.

Seems like a decent enough workaround if its not possible to update the incorrect tag.

I tested it out and there's one edge case, not sure if it matters:

  • Open a room with a location shared in.
  • Send a verification request from web so that the verification occurs on a modal above the room.
  • When the verification completes and the events start to decrypt that location shows as a geo: text.

Comment on lines +233 to +234
[roomDataSource destroy];
roomDataSources[roomId] = nil;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be reusing closeRoomDataSourceWithRoomId in case that ever gets tweaked and this is missed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So basically I found that the best spot to do it is before opening a room. Calling it on closing it won't cover a case when you receive the key once closed.
Other than that, I did not destroy everything when the user is inside the room.
Also I did found out the destroy is really necessary otherwise it won't work properly

@flescio flescio merged commit 9441f69 into develop Mar 2, 2023
@flescio flescio deleted the flescio/timeline_bubbles_issue_after_decrypt branch March 2, 2023 10:30
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.

3 participants