-
Notifications
You must be signed in to change notification settings - Fork 498
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
Only use device type name as fallback for session display name #6820
Only use device type name as fallback for session display name #6820
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but could you revert the changes to the translations please? We should only edit the en
ones in the project otherwise we risk merge conflicts when updating Weblate.
Argh, sorry I wasn't aware. 🤦♂️ Have backed them out now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🙏
Hm, actually this is now making locheck produce these warnings
And since we call it with I suppose the warning stems from the English string being gone. So maybe we need to keep all strings around for now? |
Codecov ReportBase: 11.41% // Head: 53.58% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #6820 +/- ##
============================================
+ Coverage 11.41% 53.58% +42.16%
============================================
Files 1544 401 -1143
Lines 154237 15594 -138643
Branches 62254 6305 -55949
============================================
- Hits 17606 8356 -9250
+ Misses 136027 7048 -128979
+ Partials 604 190 -414
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. |
📱 Scan the QR code below to install the build for this PR. If you can't scan the QR code you can install the build via this link: https://i.diawi.com/gH2mrh |
Kudos, SonarCloud Quality Gate passed! |
@@ -38,17 +38,15 @@ enum DeviceType { | |||
} | |||
|
|||
var name: String { | |||
let appName = AppInfo.current.displayName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pixlwave sorry just pinging to inform you that I've decided to include this in the current PR as it's related. We shouldn't use the current app name here because the device type name is used for all sessions, so there's no guarantee the others are Element, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The session display name should usually be sufficient and we'll indicate the device type via the icon already.