Modify the deprecated func #42

ujun · 2019-01-10T11:17:49Z

The document of tcell (https://godoc.org/github.com/gdamore/tcell#Screen) says as following.

SetCell is an older API and will be removed. Please use SetContent instead

This PR alters SetCell into SetContent .

ujun · 2019-01-10T11:26:32Z

As the doc also says that the fourth argument will usually be nil, I set nil to that for the present.

akavel · 2019-01-10T13:24:33Z

I believe the way the new API in tcell is better, is that it allows for supporting combining runes. Would you consider adding full support for those as part of this PR, as part of a comprehensive, "full steam ahead" migration to the new API? I must admit I'd really love, were we to do it, if we did it right...

Without that, I'm currently somewhat hesitant towards just changing the call sites from one signature to the other; especially given the fact, that with the old signature, we kinda implicitly signal, that we are in fact obsolete and "incorrect" (by blindly ignoring the need for special handling of the combining characters). This way, we're hinting between the lines at the fact, that this should indeed be changed to SetContent; but then, ideally by having it done in the "correct" way, with support and recognition of Unicode combining characters. I don't really have any idea what such support would take; I just never tried to do it; it may well be really simple to do (there's the whole unicode package, which may well have some ready-made functions which might make this functionality trivial, or at least easy enough). If you were to try exploring this direction, it would really help me make my mind on this change. Would you consider trying? Or have maybe you already tried, and could share some findings from this exploration, maybe showing how it's too hard to do?

With all that said, please note I am not yet well aware of all the reasons you had to find this change worth doing and submitting. Could you let me know a bit more of the background behind what helped you decide on that? I'd be really interested in learning more, if you don't mind telling me! This could help me understand how to try and approach this change and this conversation appropriately for both of us. TIA! And by all means, please do note I'm really happy and grateful for your attempt at helping me with the project, and really appreciate it, thanks a lot! 😃

ujun · 2019-01-11T09:55:52Z

@akavel

Thanks for your reply.
First of all, I'm sorry I didn't make the purpose of this PR clear enough.

I too think that this PR shouldn't be merged without full support .
As I'm so new about handling Unicode, I hoped somebody to help me through this PR.

Though I thought I should tackle this on my own, wanna do anything better for this repos and carelessly made this PR.

You can close this PR for now.
Sorry for disturbing you.

Best Regards,

akavel · 2019-01-11T12:18:20Z

@ujun -san,

I'm very sorry, I would be really grateful if you could try to not take my words as critique or offensive, I absolutely didn't intend this! I'm really super happy, and really appreciate it, that you found the project so interesting, that you took time and effort to want to help me with it, and submit a PR! It's just that I needed a lot of clarification on various things; that's why I tried to ask, in such a way I was able to. Communication on the Internet is, in my opinion, unfortunately often difficult, and sometimes much more words need to be spilled until a common understanding is reached, than if speaking eye to eye. Please try to be patient with me, I know I may not understand Japanese culture and ways well enough, I have big respect for them and for you, I would never want to offend you, and always try all my best to be polite; I would be really grateful if you could try to always be patient for me, and any faux pas I may have done, especially as a Westerner, when talking with you, and I'm afraid I may do again, alas, not even knowing that I do them! I never in my life had contact with Japanese culture, and way of speaking and communication. So there is nobody, other than you, that can tell me if I did something offending or impolite in your eyes, and teach me your ways. But you are also always free to leave this conversation and relationship at your discretion, if you ever find you are tired and don't have enough patience for me; I already have huge respect for you, and it will stay so, so I will always respect your decision.

I want to also add, that I would be really very happy to try and help you through this PR, if you would be interested in my guidance and further discussion. That's something I especially respect and appreciate in people who decide to submit a PR for projects I tend to. That you said so in your reply, was very important for me and made me really happy for you, and makes me believe there is a good perspective that we could hopefully cooperate to try and lead this project to improve and become better, as a carefully tended garden. I strongly believe, that cooperation on a PR is always a very interesting learning experience for both sides, both on the professional side, and on personal side of things.

I would be very sorry to close this PR, if you still have time and will of heart to try and improve it. It made me really sorry that you wrote about it as careless; I don't see your PR as such; on the contrary, I strongly believe that the fact that you made it is in itself an expression that you do care. It's just that I didn't understand initially enough to know more about your plans, ideas, context, background, etc, etc. Without those, a PR with a short message is unfortunately usually too difficult for me to understand and merge without some further questions and discussion.

For your convenience, based on what I managed to find for now on the Internet, for developing this PR, I thought that maybe those links could be of some initial help:

https://godoc.org/golang.org/x/text/unicode/norm#Properties — I would hope this could be in some way used in up, to detect which runes are combining, and with which characters, to properly pass them as combc in SetContent; but this needs more thoughtful exploration, to understand how to possibly use it in a proper way, if at all possible (I hope it is!);
https://stackoverflow.com/a/52007014/98528 — here is one example of real combining characters, that I believe could be useful for testing; this is probably just a simple one, so more advanced ones would be needed in future too, but I believe this could be really good initial case for prototyping.
http://unicode.org/faq/char_combmark.html — this appears to be a comprehensive introductory article for understanding Unicode combining characters.

If you would have any questions, or doubts, or problems, I would be honored, and really glad, and really respect you if you decided to ask me about them, ideally in this conversation; I will do my best to answer them the best I can, or try to lead you in some potentially promising direction. I would also really appreciate if you could share your plans regarding this PR and how you think you might improve it, if it's OK for you that we would then discuss them.

Also, by the way, if we manage to successfully lead this PR to completion, I would be really interested if you liked to help the project to be useful with Japanese characters. I have no idea if it works OK now!

Thank you very much, and sorry for any ways that I may have written in a way not polite enough, or may do so again and again in future, even though I really wish this would never happen.

Best Regards,

akavel · 2019-01-11T12:32:05Z

PS. On the other hand, based on https://github.com/leighmcculloch/go-strrev/blob/master/strrev.go#L79 (via https://stackoverflow.com/a/46903574/98528), it might be possible, that just the unicode.IsMark function might be enough for up? It would be good to try and understand this area somewhat better.

ujun · 2019-01-11T13:19:47Z

@akavel

Thank you for your reply.
And thanks for much information.
I really think this project is wonderful.

You are not making me angry or annoyed.
Rather, I am very pleased to meet such a kind person on the Internet for the first time.

I had no confidence and I was about to give up.
Thank you for encouraging me.
It may take quite some time, but I will try it.

Please give me some time.

…combining characters

…account

ujun · 2019-04-09T11:18:22Z

@akavel

I'm sorry I took time.

It looks like it works well with the fixes I have committed additionally considering combining characters. Also, within my observation range, it seems that Japanese is working well.

The following modules that you have already added to go.mod helped a lot:
https://github.com/mattn/go-runwidth

Also, unicode.IsMark () could be useful returning if rune is a combining character.
https://golang.org/pkg/unicode/#IsMark

There have been a lot of fixes, but would you please review it?

Regards,

akavel

Thank you very much for the extra time and effort you've put into this PR! I like a lot about your work here, how you looked about the different aspects of the program, and how you carefully split the contribution into small commits, in a way that made it much easier to understand and analyze for me. I really appreciate that! I do also believe there are some things that can still be improved here. Please take care to review the comments I've added. Thank you so much!

akavel · 2019-04-10T18:44:49Z

up.go

+	// the primary non-zero width rune
+	var mainc rune
+	// the array that follows is a possible list of combining characters to append
+	combc := make([]rune, 0)


Please use the combc := []rune{} form. Both are equivalent, it's just that this is the one I'm trying to use in all my projects by default.

As we discussed, I modified drawText function like below. Thanks 243eb17

func drawText(region Region, x int, style tcell.Style, text []rune) int { w := 0 // a combining character sequence, see: http://unicode.org/faq/char_combmark.html seq := []rune{} drawSeq := func() { if len(seq) > 0 { region.SetContent(x+w, 0, seq[0], seq[1:], style) w += runewidth.RuneWidth(seq[0]) } } for _, ch := range text { if !unicode.IsMark(ch) { drawSeq() seq = seq[:0] } seq = append(seq, ch) } drawSeq() return w }

akavel · 2019-04-10T18:45:43Z

up.go

+func drawText(region Region, x int, style tcell.Style, text string) {
+	// the primary non-zero width rune
+	var mainc rune
+	// the array that follows is a possible list of combining characters to append


Please keep this comment to only: "a possible list of combining characters"; it's perfectly good and understandable without the rest of the words.

Thanks. As you say, I thinks it's simple and better.

290fae2

akavel · 2019-04-10T18:58:29Z

up.go

+		if unicode.IsMark(ch) {
+			combc = append(combc, ch)
+		} else {
+			region.SetContent(x, 0, mainc, combc, style)


I don't think I understand the logic here correctly. Based on the Unicode Combining Marks FAQ, I assume the runes in a string in case of a combining character sequence are ordered as: a base character, followed by any number of combining characters. For example, å can be encoded as: "A" + "COMBINING RING". This leads me to believe, that for such a character sequence, this loop would first enter the block of l.810, calling SetContent with mainc=0; that doesn't seem right?

Expanding on your proposal, what would you think about changing this function as below?

w := 0 // a combining character sequence, see: http://unicode.org/faq/char_combmark.html seq := []rune{} drawSeq := func() { if len(seq) > 0 { region.SetContent(x+w, 0, seq[0], seq[1:], style) w += runewidth.RuneWidth(seq[0]) } } for _, ch := range text { if !unicode.IsMark(ch) { drawSeq() seq = seq[:0] } seq = append(seq, ch) } drawSeq(seq) return w

@akavel

Thanks for your advice.

In fact, I was just thinking asking for help about this function. As you pointed out, if å appears, a zero-width character are printed. But because it has no width, the variable w is not incremented. As the result, the following loop sequence begin with the first byte of this line.

I think this is a little hard to understand and tend to be a cause of bug. I think your way more suitable.

Modified drawText function as you recommended. Thanks.

Please review 243eb17

akavel · 2019-04-10T19:32:33Z

up.go

 		if e.cursor > 0 {
 			e.cursor--
+			for unicode.IsMark(rune(e.value[e.cursor])) {


Please add a safety check: for e.cursor > 0 && ...

Isn't e.value a []rune? are there reasons why the cast to rune(...) in this line is needed?

Please add a safety check: for e.cursor > 0 && ...

Could you review d674d5f ?

Isn't e.value a []rune? are there reasons why the cast to rune(...) in this line is needed?

As you say, that cast seems to unnecessary...
I modified. d674d5f

akavel · 2019-04-10T19:36:47Z

up.go

 		if e.cursor < len(e.value) {
 			e.cursor++
+			for e.cursor < len(e.value) && unicode.IsMark(rune(e.value[e.cursor])) {


Similar as above: is there a reason for the cast to rune(...) here?

That cast seems to unnecessary...
I modified 093d762

akavel · 2019-04-10T20:04:37Z

up.go

 		if x <= v.X && v.X != 0 {
-			x, ch = 0, '«'
+			x, mainc = 0, '«'


shouldn't we also set combc = nil here?

I consider again and think that it's better way to use a simple sequence of characters instead of mainc and combc like drawText function as you recommended. So, I made similar changes to (v *BufView) DrawTo as below.

Sorry for a little bit large, could you review?

9bb3ce5

akavel · 2019-04-10T20:05:03Z

up.go

 			lclip = true
 		} else {
 			x -= v.X
 		}
 		if x >= region.W {
-			x, ch = region.W-1, '»'
+			x, mainc = region.W-1, '»'


similar as above — what about combc?

similar as above — Cloud you review ?

akavel · 2019-04-10T21:05:01Z

up.go

-	for i, ch := range e.value {
-		region.SetCell(len(e.prompt)+i, 0, style, ch)
-	}
+	drawText(region, 0, style, string(e.prompt))


If you could modify drawText to return the width of the string passed as the last parameter, we could save the widths of this and the next line in variables, and reuse them later, replacing some of the StringWidth calls:

wprompt := drawText(region, 0, style, string(e.prompt)) wvalue := drawText(region, wprompt, style, string(e.value)) ...

I also think your way better. Thanks.

Could you review 243eb17 ?
I modified drawText function and use the return value.

akavel · 2019-04-10T21:10:31Z

up.go

-func drawText(region Region, style tcell.Style, text string) {
-	for x, ch := range text {
-		region.SetCell(x, 0, style, ch)
+func drawText(region Region, x int, style tcell.Style, text string) {


Please try changing the last parameter from string type to []rune. I think this may simplify most of the calls to drawText in the new code; if I see correctly, there might be only one place where an explicit []rune(...) conversion will be needed when calling drawText.

Could you review 243eb17 ?

I modified drawText function as you pointed out.

type of the last parameter

return value as a lengh of text

use a seq variable to hold a sequence of characters

akavel · 2019-04-10T21:27:50Z

up.go

 		}
 	}

 	x, y := 0, 0
-	// TODO: handle runes properly, including their visual width (mattn/go-runewidth)
+	// the primary non-zero width rune


Please try coming back to this function after reviewing the comments I made on the drawText function.

and modified the type of the last argument for drawText function to []rune

Modify the deprecated func

107468b

Uchino Jun added 7 commits April 8, 2019 23:14

Modified drawText function to support combining characters

7eb8334

Modified drawText function to support the offset of text

950747f

Modified (*Editor)DrawTo method to support combining characters

6813342

Enable moving the cursor on Editor properly for strings that contain …

a55e206

…combining characters

Count the actual delta of e.cursor, taking combining characters into …

df224fa

…account

Modified drawch to support combining characters

8c56ee2

Modified (*BufView)DrawTo method to support combining characters

1ee1b22

akavel requested changes Apr 10, 2019

View reviewed changes

Uchino Jun added 10 commits April 26, 2019 09:46

Added combc = nil

7ed4fa5

Modified the type of the last argument of drawText function

b2e6c71

Deleted the unnecessary cast

093d762

Modified drawText function to return the rune width of the text

243eb17

and modified the type of the last argument for drawText function to []rune

Added a safety check and removed the unnecessary cast

d674d5f

Modified delete method to a simple way

5600338

Substitute drawSeq function for drawch function

9bb3ce5

Modified some comments

290fae2

go fmt

ca16ea7

Added some tests for delete method

fa748c5

akavel mentioned this pull request Jan 15, 2021

Chinese support got an issue #51

Open

akavel mentioned this pull request Apr 2, 2021

keys: Implement unix-word-rubout (Ctrl-W) #54

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify the deprecated func #42

Modify the deprecated func #42

ujun commented Jan 10, 2019 •

edited

Loading

ujun commented Jan 10, 2019

akavel commented Jan 10, 2019

ujun commented Jan 11, 2019

akavel commented Jan 11, 2019 •

edited

Loading

akavel commented Jan 11, 2019 •

edited

Loading

ujun commented Jan 11, 2019

ujun commented Apr 9, 2019 •

edited

Loading

akavel left a comment

akavel Apr 10, 2019

ujun Apr 28, 2019

akavel Apr 10, 2019

ujun Apr 28, 2019

akavel Apr 10, 2019

ujun Apr 11, 2019

ujun Apr 28, 2019

akavel Apr 10, 2019

ujun Apr 28, 2019

akavel Apr 10, 2019

ujun Apr 28, 2019

akavel Apr 10, 2019

ujun Apr 28, 2019

akavel Apr 10, 2019

ujun Apr 28, 2019

akavel Apr 10, 2019

ujun Apr 28, 2019

akavel Apr 10, 2019

ujun Apr 28, 2019

akavel Apr 10, 2019

Modify the deprecated func #42

Are you sure you want to change the base?

Modify the deprecated func #42

Conversation

ujun commented Jan 10, 2019 • edited Loading

ujun commented Jan 10, 2019

akavel commented Jan 10, 2019

ujun commented Jan 11, 2019

akavel commented Jan 11, 2019 • edited Loading

akavel commented Jan 11, 2019 • edited Loading

ujun commented Jan 11, 2019

ujun commented Apr 9, 2019 • edited Loading

akavel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujun commented Jan 10, 2019 •

edited

Loading

akavel commented Jan 11, 2019 •

edited

Loading

akavel commented Jan 11, 2019 •

edited

Loading

ujun commented Apr 9, 2019 •

edited

Loading