Skip to content

Client: Fix wrong truncation of unicode aliases #2275

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.

Sign up for GitHub

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

Jump to bottom
Merged
hoffie merged 1 commit into from
Jan 25, 2022
Merged

Client: Fix wrong truncation of unicode aliases #2275

hoffie merged 1 commit into from
Jan 25, 2022

Conversation

@hoffie
Copy link
Member

@hoffie hoffie commented Jan 25, 2022 •
edited
Loading

Short description of changes

If the 16th char of the user-selected alias was a multi-byte unicode char, it would be incorrectly split in half, rendering it invalid. This commit makes the truncation unicode-aware.

The fix was initially developed in ce370e9. It got lost during PR merge in commit a8853b6, most likely during conflict resolution due to code reorganizations in d94fd69.

This commit re-applies the fix to the new place (CClientSettingsDlg::OnAliasTextChanged instead of the no longer
existing CMusProfDlg::OnAliasTextChanged). It also adds a function declaration to the header to make inclusion by other files possible in the first place. This also required dropping the static and inline keywords.

Co-authored-by: @djfun

See #2274 for more context.

Context: Fixes an issue?

Fixes: #1994 (for real)
Fixes: #2274

Does this change need documentation? What needs to be documented and how?

No. ChangeLog has already been slightly adjusted to include this issue number.
CHANGELOG: SKIP

Status of this Pull Request

Ready, should go into 3.8.2.

What is missing until this pull request can be merged?

Needs review.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want (Linux)
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@hoffie hoffie added this to the Release 3.8.2 milestone Jan 25, 2022
@hoffie hoffie mentioned this pull request Jan 25, 2022
Closed
3 tasks
@softins
Copy link
Member

softins commented Jan 25, 2022

It also moves the related inline function to the header to make it possible to include in the first place.

Did you mean "inline" rather than "include"?

As a minor thing, I'm not a fan of non-trivial code being in a .h rather than a .cpp. Does this function really need to be inline?

@hoffie
Copy link
Member Author

hoffie commented Jan 25, 2022

It also moves the related inline function to the header to make it possible to include in the first place.

Did you mean "inline" rather than "include"?

What I meant is that without moving the file to the (included) uti.h header, it is not usable outside of util.cpp. I have adjusted the PR and the commit text in the hope that it is clearer now.

As a minor thing, I'm not a fan of non-trivial code being in a .h rather than a .cpp. Does this function really need to be inline?

I agree, but I initially tried to keep the code as-unmodified as possible. Keeping the static definition required me to move the whole function to the header. When dropping static, one would still get compiler warnings due to inline (probably because inlineonly makes sense in header files for reusable code?).
I don't see a reason for inlining though. This is not a performance-critical function and even if it was, compilers are usually better at deciding this, I guess...

So, I've dropped both static and inline now, kept the function in util.cpp and added the declaration to util.h.

@hoffie @djfun
Client: Fix wrong truncation of unicode aliases
3fc13f8 
If the 16th char of the user-selected alias was a multi-byte unicode
char, it would be incorrectly split in half, rendering it invalid.

This commit makes the truncation unicode-aware.

The fix was initially developed in ce370e9.
It got lost during PR merge in commit a8853b6,
most likely during conflict resolution due to code reorganizations in
d94fd69.

This commit re-applies the fix to the new place
(CClientSettingsDlg::OnAliasTextChanged instead of the no longer
existing CMusProfDlg::OnAliasTextChanged).

It also adds a function declaration to the header to make
inclusion by other files possible in the first place. This also required
dropping the `static` and `inline` keywords.

Fixes jamulussoftware#1994 (for real).
Fixes jamulussoftware#2274.

Co-authored-by: Martin Kaistra <admin@djfun.de>
softins
softins approved these changes Jan 25, 2022
View reviewed changes
Copy link
Member

@softins softins left a comment

Choose a reason for hiding this comment

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

I was unable to demonstrate the problem in a version prior to this fix, so couldn't do a before and after comparison. But the code looks fine, and alias entry and display works correctly with an accented character at the end,so happy to approve.

@djfun
Copy link
Contributor

djfun commented Jan 25, 2022 •
edited
Loading

the original issue mentions the test string "Mr. Hu 🍽", where the last character is problematic

@softins
Copy link
Member

softins commented Jan 25, 2022

the original issue mentions the test string "Mr. Hu 🍽", where the last character is problematic

Yes, that was the one that showed it up when trying to line-wrap the name on the mixer display, and was already fixed. But this PR is addressing truncation after 16 characters in the settings window, which I couldn't make go wrong in a prior version, with a multi-byte char at the end.

@djfun
Copy link
Contributor

djfun commented Jan 25, 2022

the original issue mentions the test string "Mr. Hu 🍽", where the last character is problematic

Yes, that was the one that showed it up when trying to line-wrap the name on the mixer display, and was already fixed. But this PR is addressing truncation after 16 characters in the settings window, which I couldn't make go wrong in a prior version, with a multi-byte char at the end.

Try putting this string in the settings window alias field and then insert characters before the multi-byte character until you get a question mark character at the end.

@hoffie
Copy link
Member Author

hoffie commented Jan 25, 2022

I was unable to demonstrate the problem in a version prior to this fix, so couldn't do a before and after comparison.

Sorry, should have documented my test case as it also took me some time to understand and reproduce the problem.

Before this fix: 12345678901234😍 works, adding 5 makes Jamulus display an invalid character (?)
After this fix: 12345678901234😍 works, adding 5 properly discards the whole Unicode character at the end (no ?).

pljones
pljones approved these changes Jan 25, 2022
View reviewed changes
Copy link
Collaborator

@pljones pljones left a comment

Choose a reason for hiding this comment

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

Thanks!

@hoffie hoffie merged commit 6df834c into jamulussoftware:master Jan 25, 2022
@hoffie hoffie deleted the fix-pr1451 branch March 19, 2022 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pljones pljones pljones approved these changes

@softins softins softins approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Release 3.8.2

Development

Successfully merging this pull request may close these issues.

Parts of PR #1994 (wrong display of Unicode characters at line wrap) got lost

4 participants

@hoffie @softins @djfun @pljones