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: text color of the last event description was incorrect. #7545

Merged
merged 1 commit into from
May 15, 2023

Conversation

nimau
Copy link
Contributor

@nimau nimau commented May 12, 2023

This PR fixes a problem where the font color of lastEventDescription is not updated when the theme changes.

In the RecentTableViewCell, the lastEventDescription label has a foreground color but most of the time it displays the roomCellData.lastEventAttributedTextMessage which is an attributed string containing its own set of text color attributes.

By default, this attributed string uses only the secondaryTextColor, but in some scenarios (such as a live broadcast), the text may have a different color.

This attributed string is calculated by the EventFormatter when updating the room summary. The solution chosen for this PR was add two custom attributes for this attributed string:

  • themeIdentifierAttributeName: to store which theme was active when the attributed string was created
  • themeColorNameAttributeName: to store the name for the color

Before rendering the attributed string, if it contains the themeIdentifierAttributeName attribute and the active theme doesn't match, then the colors for the string will be updated to the current theme using the themeColorNameAttributeName attribute.

@nimau nimau force-pushed the nimau/PSB307_last_event_description_color branch from 4975739 to b558d4c Compare May 12, 2023 12:36
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

❗ No coverage uploaded for pull request base (develop@cc9bc24). Click here to learn what that means.
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #7545   +/-   ##
==========================================
  Coverage           ?   12.36%           
==========================================
  Files              ?     1648           
  Lines              ?   163576           
  Branches           ?    67167           
==========================================
  Hits               ?    20231           
  Misses             ?   142680           
  Partials           ?      665           
Flag Coverage Δ
uitests 55.06% <0.00%> (?)
unittests 6.21% <0.00%> (?)

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

Impacted Files Coverage Δ
Riot/Categories/NSAttributedString+Theme.swift 0.00% <0.00%> (ø)
Riot/Managers/Theme/ThemeService.swift 0.00% <0.00%> (ø)
...Modules/Common/Recents/Views/RecentTableViewCell.m 0.00% <0.00%> (ø)
Riot/Utils/EventFormatter.m 7.46% <0.00%> (ø)
Riot/Utils/ThemeColorResolver.swift 0.00% <0.00%> (ø)

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

@nimau nimau force-pushed the nimau/PSB307_last_event_description_color branch from b558d4c to d628a03 Compare May 12, 2023 15:16
@sonarcloud
Copy link

sonarcloud bot commented May 12, 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

@nimau nimau marked this pull request as ready for review May 12, 2023 15:26
@nimau nimau requested a review from pixlwave May 12, 2023 15:26
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.

Nice, looks sane to me. Maybe now that there's a colour name it could use theme.performSelector(name) instead of a Mirror and reflection, but off the top of my head I'm not sure if Theme is coming directly from objc to allow that.

@nimau
Copy link
Contributor Author

nimau commented May 15, 2023

Nice, looks sane to me. Maybe now that there's a colour name it could use theme.performSelector(name) instead of a Mirror and reflection, but off the top of my head I'm not sure if Theme is coming directly from objc to allow that.

Hi @pixlwave, I tried but it doesn't seem possible on theme

@nimau nimau merged commit ae407fa into develop May 15, 2023
@nimau nimau deleted the nimau/PSB307_last_event_description_color branch May 15, 2023 07:03
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