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 calculating button absolute path length #184

Merged
merged 5 commits into from
Jul 13, 2022
Merged

Conversation

harimambura
Copy link
Contributor

Fix calculating button absolute path length with non-Latin symbols according issue #183

@EdJoPaTo
Copy link
Owner

I added a test case which interestingly also works when using the old logic with absolutePath.length. Am I overlooking something? Did that already work before?

@harimambura
Copy link
Contributor Author

harimambura commented Jun 25, 2022

I added a test case which interestingly also works when using the old logic with absolutePath.length. Am I overlooking something? Did that already work before?

this could not work before because in utf-8 encoding Russian characters start from 1040, which is 2 bytes
I'll take a look at test case

@harimambura
Copy link
Contributor Author

Done

@harimambura
Copy link
Contributor Author

As I can see, there is another one problem with Buffer with typescript? but I never used typescript and don't know how to fix it, please take a look :)

@EdJoPaTo
Copy link
Owner

As I can see, there is another one problem with Buffer with typescript? but I never used typescript and don't know how to fix it, please take a look :)

I already fixed them in my first commit (570930e)


I think I understand the thought behind your change in 6aa40e6 but the Keyboard class takes a relative path (without the /). In this case it still works as the leading / means to go back to the root level where it currently is. That way the resulting path is the same in both cases.


Looks like JavaScript String.length uses UTF-16 which is why it also correctly identified the cyrillic letters to use 2 characters.

So for everything within the UTF-8 range String.length should work fine which is the current behaviour. Not sure what the issue in #183 is when the .length seems to work in the case of cyrillic letters? 🤔

@harimambura
Copy link
Contributor Author

harimambura commented Jun 25, 2022

ok, could you just run next code in node?
'пример'.length === Buffer.byteLength('пример', 'utf-8')
and next code in browser devtools:
'пример'.length === (new Blob(['пример'])).length

I'm getting false for both cases

So String.length returns the number of characters, not the length of the bytes, but the TG API calculates the length of the bytes, that's the problem

The testcase was done misleading which resulted in to many characters.
Therefore it did not test what it was supposed to test.
@EdJoPaTo
Copy link
Owner

Sorry for the long delay…

I found out my confusion why String.length seemed to work. The testcase I wrote used your 48 character string twice which ended up being >64 anyway. Both characters and bytes were too long so String.length also seemed to work for that case. I fixed the testcase in f240b9f. Using String.length with the fixed testcase makes a difference which resolves my confusion about it.

Thank you for reporting that problem and providing the fix! Sorry again for my long delay in handling this.

@EdJoPaTo EdJoPaTo merged commit b95e62c into EdJoPaTo:main Jul 13, 2022
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