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

#6449 Extended file name support to include characters from multiple languages, including Cyrillic and Han scripts #8925

Conversation

christianrowlands
Copy link
Contributor

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

I fixed an issue when sharing files that had non-Latin characters would have the file name replaced with underscores. For example, here is a screenshot that shows up when "forwarding" a file that was send in Element Android.

image

Motivation and context

Here is a link to the issue: #6449

Worthy of note is that I thought about a couple different approaches to fixing this problem. My first regex approach was to use the existing "inclusion" approach, and add Cyrillic and Han scripts. However, after realizing that it could get messy to add support for all the different scripts, I switched to an "exclusion" approach where I remove any known invalid characters.

For reference, here was the first approach

.replace("[^\\p{sc=Cyrillic}\\p{sc=Han}a-z A-Z0-9\\\\.\\-]".toRegex(), "_")

And version 2

.replace("[\\\\?%*:|\"<>\\s]".toRegex(), "_")

Tests

  1. I sent a file containing Cyrillic characters in Element Web.
  2. I viewed that message in Element Android
  3. I clicked the share button for that file.
  4. I verified that the file name in the share UI was not all underscores.

I also wrote unit tests to verify the new regex works as expected (see the code diff).

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 15 and Android 5.1

Checklist

Signed-off-by: Christian Rowlands <craxiomdev [at] gmail.com>

@christianrowlands
Copy link
Contributor Author

@bmarty , I have another simple PR for you if you can take a look at it. If you have any objections to the new RegEx, I am happy to update it as necessary, or add more tests to verify different scenarios.

@christianrowlands christianrowlands changed the title #6449 #6449 Extended file name support to include characters from multiple languages, including Cyrillic and Han scripts Oct 16, 2024
@element-hq element-hq deleted a comment from logman12oge Oct 25, 2024
@CLAassistant
Copy link

CLAassistant commented Nov 1, 2024

CLA assistant check
All committers have signed the CLA.

@bmarty bmarty self-requested a review November 8, 2024 10:10
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for this change!

@bmarty
Copy link
Member

bmarty commented Nov 12, 2024

@christianrowlands can you handle the errors reported by the CI please? Let me know if you need some help.

@christianrowlands
Copy link
Contributor Author

Thanks for reviewing!

I will look into the CI failures.

@christianrowlands
Copy link
Contributor Author

@bmarty , give it another run and see if that resolves some or all of the issues.

@bmarty
Copy link
Member

bmarty commented Nov 12, 2024

Can you run ./gradlew ktlintFormat first? I think it will remove some unused imports.

@christianrowlands
Copy link
Contributor Author

Whoops, I thought I already removed that unused Timber import. I will run it again now.

@bmarty
Copy link
Member

bmarty commented Nov 12, 2024

I think you need to either rebase your PR's branch, or merge develop into your branch so that the code can be compiled. We are now using Java 21.

@christianrowlands
Copy link
Contributor Author

Ok, I merged develop into my branch. Hopefully we are good now 🤞

@bmarty bmarty merged commit 93962d0 into element-hq:develop Nov 12, 2024
11 of 13 checks passed
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