Replace to tcell from termbox #32

tjmtmmnk · 2020-12-01T14:07:16Z

Related Issue: #15

Thanks great library!
This PR is intended to replace completely from termbox to tcell.

I think this migration has two merits.

It is stated that termbox will not be maintained anymore (ref). So, migrate to maintained tcell, make this product more stable.
tcell has a lot of useful features. For example, Ansi color in preview mode #29 can solve easily by using code used in tview etc (ref).

The problem reported in #17, can solve by using workaround(ref).

I could confirm the code(reproduce the problem) work well in local.

codecov · 2020-12-01T14:08:17Z

Codecov Report

Merging #32 (159aa65) into master (6cc406b) will decrease coverage by 13.15%.
The diff coverage is 84.30%.

@@             Coverage Diff             @@
##           master      #32       +/-   ##
===========================================
- Coverage   89.61%   76.45%   -13.16%     
===========================================
  Files           6        6               
  Lines         597      756      +159     
===========================================
+ Hits          535      578       +43     
- Misses         44      159      +115     
- Partials       18       19        +1

ktr0731 · 2020-12-02T04:50:36Z

Hi, @tjmtmmnk. Thanks a lot for your great work!
I'll review this Pull Request later, so please take a little...

ktr0731

Thanks for your contribution! I commented on some points.

ktr0731 · 2020-12-03T12:49:32Z

.github/workflows/main.yml

@@ -30,7 +30,7 @@ jobs:
      run: go build

    - name: Test
-      run: go test -v -coverpkg ./... -covermode atomic -coverprofile coverage.txt -tags fuzz -numCases 3000 -numEvents 300 ./...


Is this not a part of the changes PR's title describes?

This change related to #32 (comment)

ktr0731 · 2020-12-03T13:07:46Z

mock.go

-
-// SetSize changes the pseudo-size of the window.
-// Note that SetSize resets added cells.
-func (m *TerminalMock) SetSize(w, h int) {


This change breaks TerminalMock's backward-compatibility. Could you define TerminalMockV2 instead of modifying TerminalMock?
Or, we may be able to put a new method SetTcellEvents into TerminalMock without breaking the current interface (maybe this is better if possible).

~~ok. I will define TerminalMockV2.~~
It wasn't such a simple problem, so I'll think about it for a moment.

To me TerminalMock seemed to be used only for testing purposes.
I didn't understand why backwards compatibility was needed so please let me know...

Yes, it is used only for testing. However, according to the import compatibility rule of Go modules, public interfaces should be compatible, regardless of whether they are for testing or not.

Please let me know if there seems to be no way to keep backward compatibility; I will consider making a v2 release...

Thanks for introducing useful article!
I tried to make the public interface backwards compatible.

mock.go

ktr0731 · 2020-12-03T13:57:08Z

tcell.go

+type termImpl struct {
+	screen tcell.Screen
 }


I think it's clearer to simply use embedding.

Suggested change

type termImpl struct {

screen tcell.Screen

}

type termImpl struct {

tcell.Screen

}

ktr0731 · 2020-12-03T14:05:45Z

fuzzyfinder.go

@@ -553,7 +618,6 @@ func (f *finder) find(slice interface{}, itemFunc func(i int) string, opts []Opt
 	if err := f.initFinder(items, matched, opt); err != nil {
 		return nil, errors.Wrap(err, "failed to initialize the fuzzy finder")
 	}
-	defer f.term.close()


Should we call f.term.Screen().Fini() at the end?

I call Fini() here and here

Oh, I misunderstood...

However, is there any reason to call f.term.Fini() in Find and FindMulti instead of find? I thought it would work fine if we left it here.

I was misunderstanding, as you say, I can call Fini() in find

ktr0731 · 2020-12-03T14:07:32Z

fuzzyfinder.go

+func sendExtraEventFix() {
+	kb, err := keybd_event.NewKeyBonding()
+	if err != nil {
+		panic(err)


Do not use panic, return the error instead.

ktr0731 · 2020-12-03T14:07:47Z

fuzzyfinder.go

@@ -604,6 +672,20 @@ func (f *finder) find(slice interface{}, itemFunc func(i int) string, opts []Opt
 	}
 }

+// This method avoid tcell bug https://github.com/gdamore/tcell/issues/194
+// Aditional EOL event is sent to ensure, consequent events, are correctly handled
+func sendExtraEventFix() {


Great hack 👍

fuzzyfinder.go

fuzzing_test.go

Co-authored-by: ktr <ktr@syfm.me>

tjmtmmnk · 2020-12-13T12:36:12Z

tried to make the public interface backwards compatible by creating v2
embedded struct implementing Screen or SimulationScreen interface
call Fini in find

I fixed 3 points.
Please re-review when you have a time🙏

ktr0731

@tjmtmmnk
I'm really sorry I'm always late...

It's almost perfect, but I'd like you to fix a few minor things before we merge it 🙏

ktr0731 · 2020-12-19T07:12:27Z

helper_test.go

@@ -11,3 +11,11 @@ func NewWithMockedTerminal() (*finder, *TerminalMock) {
 	m.SetSize(w, h)
 	return f, m
 }
+
+func NewWithMockedTerminalV2() (*finder, *TerminalMock) {


There is no need to define a v2 function because helper_test.go is only for testing and it is referenced only from fuzzyfinder_test.go. So you can rename it to NewWithMockedTerminal and remove the old one.

Suggested change

func NewWithMockedTerminalV2() (*finder, *TerminalMock) {

func NewWithMockedTerminal() (*finder, *TerminalMock) {

ktr0731 · 2020-12-19T07:16:58Z

mock.go

+func (m *TerminalMock) close() {}
+
+// UseMockedTerminal switches the terminal, which is used from
+// this package to a mocked one.


Could you add a comment for deprecation? (ref. https://github.com/golang/go/wiki/Deprecated)

Suggested change

// this package to a mocked one.

// this package to a mocked one.

//

// Deprecated: Use UseMockedTerminalV2.

mock.go

ktr0731 · 2020-12-19T07:18:28Z

mock.go

+	return buf.String()
+}
+
+// parseAttrV2 parses color and attribute for test


[super nits]

Suggested change

// parseAttrV2 parses color and attribute for test

// parseAttrV2 parses color and attribute for testing.

ktr0731 · 2020-12-19T07:35:33Z

mock.go

@@ -45,12 +49,30 @@ func (m *TerminalMock) SetSize(w, h int) {
 	m.cells = make([]*cell, w*h)
 }

+func (m *TerminalMock) SetSizeV2(w, h int) {


Could you switch the implementation of SetSize based on a bool field such as m.v2 instead of defining SetSizeV2? (It looks like the variable v2 can be set inside of UseTerminalMockV2)

For example:

// SetSize changes the pseudo-size of the window. // Note that SetSize resets added cells. func (m *TerminalMock) SetSize(w, h int) { if m.v2 { m.simScreen.SetSize(w, h) return } m.sizeMu.Lock() defer m.sizeMu.Unlock() m.cellsMu.Lock() defer m.cellsMu.Unlock() m.width = w m.height = h m.cells = make([]*cell, w*h) }

ktr0731 · 2020-12-19T07:36:24Z

mock.go

 // SetEvents sets all events, which are fetched by pollEvent.
 // A user of this must set the EscKey event at the end.
-func (m *TerminalMock) SetEvents(e ...termbox.Event) {
+func (m *TerminalMock) SetEvents(events ...termbox.Event) {


mock.go

ktr0731 · 2020-12-19T07:38:48Z

mock.go

@@ -61,6 +83,63 @@ func (m *TerminalMock) GetResult() string {
 	return m.result
 }

+func (m *TerminalMock) GetResultV2() string {


Same as the review comment on SetSizeV2.

tjmtmmnk · 2020-12-24T09:22:37Z

Sorry for the delay, and thank you for review!
I fixed problems you indicated, please check when you have a time🙏

ktr0731

LGTM!
Thanks for your contribution. I'm sure that this change will be very helpful for go-fuzzyfinder's future.
Have a happy new year holiday 🍺

tjmtmmnk · 2020-12-25T09:46:44Z

Thank you!!

tjmtmmnk added 14 commits November 28, 2020 23:44

replace with tcell function

efa7b49

mix color logic was wrong

6ae4881

misuse style

c1de08d

migrate to tcell in test

87172be

update golden files

825a74e

call Fini if not in test

7e24714

make more stable

f6c262a

go test parallel one

703ab69

call Fini when find done

fd4b878

apply workaround for tcell bug

7b394be

mis flag position

ae89753

take lock

7fa1592

sleep to filter

0e1592c

remove comment about termbox

db0b770

ktr0731 self-requested a review December 2, 2020 04:48

ktr0731 assigned tjmtmmnk Dec 2, 2020

ktr0731 added the enhancement New feature or request label Dec 2, 2020

ktr0731 reviewed Dec 3, 2020

View reviewed changes

tjmtmmnk and others added 10 commits December 4, 2020 02:56

embed screen

f095d03

mis-remove build tag

a302584

don't use panic, return error

56c7830

follow comment style

a432d84

numCases doesn't interfere tcell limit

a19c30d

call Fini in find

e37529d

backward compatible for public interface

f371c18

use v2

0b0c964

Update fuzzyfinder.go

bedd404

Co-authored-by: ktr <ktr@syfm.me>

clarify v2

ae3ab9f

ktr0731 reviewed Dec 19, 2020

View reviewed changes

tjmtmmnk added 2 commits December 24, 2020 17:48

use v2 flag

eb34660

replace no longer needed functions

159aa65

ktr0731 approved these changes Dec 25, 2020

View reviewed changes

ktr0731 merged commit 821eba4 into ktr0731:master Dec 25, 2020

This was referenced Jan 11, 2021

cross compile is no longer possible #42

Closed

cross compile is no longer possible #43

Closed

tjmtmmnk mentioned this pull request Feb 21, 2021

[retry] Replace termbox to tcell #55

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace to tcell from termbox #32

Replace to tcell from termbox #32

tjmtmmnk commented Dec 1, 2020

codecov bot commented Dec 1, 2020 •

edited

Loading

ktr0731 commented Dec 2, 2020

ktr0731 left a comment

ktr0731 Dec 3, 2020

tjmtmmnk Dec 3, 2020

ktr0731 Dec 3, 2020

tjmtmmnk Dec 3, 2020 •

edited

Loading

tjmtmmnk Dec 7, 2020

ktr0731 Dec 9, 2020

tjmtmmnk Dec 13, 2020 •

edited

Loading

ktr0731 Dec 3, 2020

ktr0731 Dec 3, 2020

tjmtmmnk Dec 3, 2020

ktr0731 Dec 9, 2020

ktr0731 Dec 9, 2020

tjmtmmnk Dec 13, 2020

ktr0731 Dec 3, 2020

ktr0731 Dec 3, 2020

tjmtmmnk commented Dec 13, 2020

ktr0731 left a comment

ktr0731 Dec 19, 2020

ktr0731 Dec 19, 2020

ktr0731 Dec 19, 2020

ktr0731 Dec 19, 2020 •

edited

Loading

ktr0731 Dec 19, 2020

ktr0731 Dec 19, 2020

tjmtmmnk commented Dec 24, 2020

ktr0731 left a comment

tjmtmmnk commented Dec 25, 2020

	func NewWithMockedTerminalV2() (finder, TerminalMock) {
	func NewWithMockedTerminal() (finder, TerminalMock) {

	// parseAttrV2 parses color and attribute for test
	// parseAttrV2 parses color and attribute for testing.

Replace to tcell from termbox #32

Replace to tcell from termbox #32

Conversation

tjmtmmnk commented Dec 1, 2020

codecov bot commented Dec 1, 2020 • edited Loading

Codecov Report

ktr0731 commented Dec 2, 2020

ktr0731 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

tjmtmmnk Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjmtmmnk Dec 13, 2020 • edited Loading

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

tjmtmmnk commented Dec 13, 2020

ktr0731 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

ktr0731 Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjmtmmnk commented Dec 24, 2020

ktr0731 left a comment

Choose a reason for hiding this comment

tjmtmmnk commented Dec 25, 2020

codecov bot commented Dec 1, 2020 •

edited

Loading

tjmtmmnk Dec 3, 2020 •

edited

Loading

tjmtmmnk Dec 13, 2020 •

edited

Loading

ktr0731 Dec 19, 2020 •

edited

Loading