-
-
Notifications
You must be signed in to change notification settings - Fork 83
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
feat: Remove subject prefix from ad-hoc group names (#5385) #5693
Conversation
d475d23
to
5ce1d48
Compare
src/stock_str.rs
Outdated
@@ -443,6 +443,9 @@ pub enum StockMessage { | |||
fallback = "Could not yet establish guaranteed end-to-end encryption, but you may already send a message." | |||
))] | |||
SecurejoinWaitTimeout = 191, | |||
|
|||
#[strum(props(fallback = "Unnamed group"))] |
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.
Btw, i'd suggest to make it "👥📧" or even "..." not to translate it. Not a frequent case anyway
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.
Sounds good to me, the less stock strings we need to "carry around" the better
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.
Done. I think 👥📧 is the better option. I'd even say it is nicer than any black-on-white text
@iequidoo Thank you for your engagement! Unfortunately I am no programmer, therefore i cannot review the code. Please let someone else review it, thanks! :) |
You can review the behaviour described in the commit messages :) So, in the #5385 scenario after merging this you will have two chats with the same name. Then we can think how to merge them into one (i'd suggest to merge chats with the same names and participants, but it's outside of this PR's scope) |
@iequidoo I can do that, of course, as soon as I have an APK to test with. 👍 |
src/receive_imf.rs
Outdated
// See create_or_lookup_group() for explanation | ||
// See create_group() for explanation. | ||
.map(|s| s.trim()) | ||
{ | ||
if let Some(grpname) = mime_parser | ||
.get_header(HeaderDef::ChatGroupName) | ||
// See create_or_lookup_group() for explanation | ||
// See create_group() for explanation. |
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.
Not totally related to this PR, but: Explanation what for, anyway? For why we trim() the group name?
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.
Exactly. There is a comment there that is longer than the corresponding code:
// W/a for "Space added before long group names after MIME serialization/deserialization
// #3650" issue. DC itself never creates group names with leading/trailing whitespace.
I'd prefer not to duplicate it
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.
Do you know what "W/a" means?
I'd prefer not to duplicate it
Me too; we could remove it (or just the two lines here) if we don't think it adds value. For me it wouldn't add value while reading at least because I neither would have wondered why we trim() - we trim() all the time - nor would I have understood it. But we can also just keep it if other people exist who did find it understandable.
Also, do you know what "W/a" means?
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.
Also, do you know what "W/a" means?
I guess it is an abbreviation for "workaround"
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.
Removed these two referencing comments. Not that important indeed
5ce1d48
to
a90cd5e
Compare
I think we will merge this (or not), then you can test a new nightly build against your scenario. EDIT: Anyway this is not a fix yet, only some improvement, so it's up to you. |
Delta Chat -style groups have names w/o prefixes like "Re: " even if the user is added to an already existing group, so let's remove prefixes from ad-hoc group names too. Usually it's not very important that the group is a classic email thread existed before, this info just eats up screen space. Also this way a group name is likely to preserve if the first message was missed.
a90cd5e
to
f66b5ab
Compare
Just a small improvement while looking at #5385. See commit messages.