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

Display wide characters correctly (Part I) #94

Closed
wants to merge 1 commit into from
Closed

Display wide characters correctly (Part I) #94

wants to merge 1 commit into from

Conversation

yan12125
Copy link
Member

The ancient konsole_wcwidth can't recognize some double-width characters. For example, copy and paste the string 《別來無恙》 into bash, and move cursors around with left arrow, right arrow, Home and End keys. The current line becomes a mess. My patch fixes this problem.

The story doesn't end here. For example, if I paste 𝕐 into the shell, things are even worse. That's because QTerminal assume characters to be UCS-2, so it cannot handle characters outside the Basic Multilingual Plane correctly. I'd like to leave this problem into the next pull request.

By the way, is git submodule a good idea for external dependencies? This version looks the most promising, as it declares to be "Unicode 9-conformant", and is used in Termux, one of the most popular terminal emulators on Android.

@yan12125 yan12125 changed the title Replace the ancient wcwidth impl. with termux/wcwidth Display wide characters correctly (Part I) Oct 21, 2016
@agaida
Copy link
Member

agaida commented Oct 21, 2016

@yan12125 - hmm, i would prevent submodules wherever i could in the real development process (bitten one time to much) - but submodules are generally a nice way to get the current code of something if one know about the pitfalls - if not one will learn them very fast.

@yan12125
Copy link
Member Author

Yes there are indeed tricky edges for submodules. How about copy termux/wcwidth.c into lib/? The first problem I can imagine is licensing.

@agaida
Copy link
Member

agaida commented Oct 21, 2016

@yan12125 - can you please provide the Header or the file as a gist - so i could have a look at - if it is the same as this:

 * Permission to use, copy, modify, and distribute this software
 * for any purpose and without fee is hereby granted. The author
 * disclaims all warranties with regard to this software.
 *
 * Latest version: http://www.cl.cam.ac.uk/~mgk25/ucs/wcwidth.c

we should take the original one - the license header look like a short form of an ancient MIT-License, which would be compatible with the LGPL :)

@agaida agaida closed this Oct 21, 2016
@agaida agaida reopened this Oct 21, 2016
@yan12125 yan12125 mentioned this pull request Oct 21, 2016
@yan12125
Copy link
Member Author

I guess it's MIT-like, too, but I didn't find any explicit license declaration in termux/wcwidth, so I ask them there: termux/wcwidth#2

@agaida
Copy link
Member

agaida commented Oct 21, 2016

a big no - we don't want one or two files without license headers which are copied from an obscure project without any license hint - i can only guess that these guys cutted the stuff they needed out from the original file - in this case we should use the original file and port needed things back into this file or simply write a wrapper - but please no git submodule in this case, it's a no go.

@yan12125
Copy link
Member Author

If the termux variant adds a license header later, can we use it?

@agaida
Copy link
Member

agaida commented Oct 21, 2016

the problem is a little bit deeper i guess - if they have copied it they should have leave the original license intact - or at least credit the orginal author - but not strip the useless header away :) - if they add the header back we can use it - i would prefer the original file and get the needed parts from them. And make a copy not including their repo - ucs isn't known to change often these days.

Hmm, maybe i'm to much influenced by debian - but the problem is that i as a packager must be able to redistribute this file later - so it should have a proper and valid license. License violations are not cool when the main project is under LGPL - bah, couln't belive that i write such things :D

@yan12125
Copy link
Member Author

From what I can see termux/wcwidth (written in C) is based on jquast/wcwidth (written in Python) [1], and the latter is based on Markus Kuhn's version (written in C). Termux's version is not an expansion of Markus Kuhn's version. I choose Termux's version as it targets Unicode 9.0, while Markus Kuhn's one is for Unicode 5.0. I'm just too lazy to update the table again.

For why termux lacks a LICENSE file - I guess they don't have the habit to keep things tidy :)

[1] It's mentioned in termux/wcwidth's README: https://github.com/termux/wcwidth/blob/master/README.md

@agaida
Copy link
Member

agaida commented Oct 21, 2016

thats not sufficient

@yan12125
Copy link
Member Author

I'll wait for some to see whether people at Termux are serious to their fork or not. If I get no response I'll try to update Markus Kuhn's version to Unicode 9.0.

@yan12125
Copy link
Member Author

Hello @agaida, a Termux member has added a MIT license declaration in termux/wcwidth@aae3753. Is it good to move on?

@agaida
Copy link
Member

agaida commented Oct 22, 2016

nearly ok i think - to be honest i would only copy both files ( w*.c and .h) and integrate it. However, the *.c and *.h files should contain a license header - you could help them with that, a PR is no rocket science. With the headers in place no objections. But pleae leave out the sub-modules.

@yan12125
Copy link
Member Author

Another upstream update: termux/wcwidth@d48cb1e. I guess that looks good now?

@agaida
Copy link
Member

agaida commented Oct 22, 2016

yes - but i'm still not ok with the git submodule. Another point - we should/must give the distributions the chance to use their own wcwitdh package - so the usage of the third party code should be optional.

@yan12125
Copy link
Member Author

Force pushed. I have some modifications to termux's version. Changes can be found in my fork: https://github.com/yan12125/wcwidth/commits/master

@yan12125
Copy link
Member Author

yan12125 commented Nov 3, 2016

I decide to use a better maintained library instead of termux/wcwidth => #99. @agaida Sorry for bothering you with an awful project

@yan12125 yan12125 closed this Nov 3, 2016
@agaida
Copy link
Member

agaida commented Nov 3, 2016

exactly the opposite - sorry for being pain in the ass. Glad you found a better solution. Thank you very much.

@agaida agaida added this to Rejected/Declined in Pull Requests Apr 1, 2018
@agaida agaida added this to Archive in Pull Requests Aug 11, 2018
@agaida agaida added this to Archive in PRs Nov 5, 2018
@yan12125 yan12125 deleted the new-wcwidth branch February 21, 2021 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PRs
  
Changelog
Pull Requests
  
Old Archive (fucked up)
Development

Successfully merging this pull request may close these issues.

None yet

2 participants