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

[retry] Replace termbox to tcell #55

Merged
merged 5 commits into from
Mar 17, 2021

Conversation

tjmtmmnk
Copy link
Contributor

@tjmtmmnk tjmtmmnk commented Feb 21, 2021

related: #32

Hello.

Finally, tcell announced the fix of lost key problem officially. gdamore/tcell#194 (comment)

So, we can avoid the workaround which causes the cgo problem!

I could confirm that this program work well in local.

@codecov
Copy link

codecov bot commented Feb 21, 2021

Codecov Report

Merging #55 (5a7ebd0) into master (103f22a) will decrease coverage by 1.55%.
The diff coverage is 85.83%.

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
- Coverage   88.65%   87.10%   -1.56%     
==========================================
  Files           5        5              
  Lines         485      543      +58     
==========================================
+ Hits          430      473      +43     
- Misses         40       57      +17     
+ Partials       15       13       -2     

@ktr0731
Copy link
Owner

ktr0731 commented Feb 25, 2021

@tjmtmmnk
Hi, thanks for reporting the bug is fixed!
I don't have much time to spend on this right now, so can you please wait a little while?
I'll probably be able to review this PR in a few days...

@ktr0731
Copy link
Owner

ktr0731 commented Feb 25, 2021

It looks like there are conflicts, so can you resolve them?

@ktr0731 ktr0731 self-requested a review February 25, 2021 09:18
@tjmtmmnk tjmtmmnk force-pushed the replace-to-tcell-retry branch from 2de347c to 3821bcd Compare February 26, 2021 05:36
@tjmtmmnk
Copy link
Contributor Author

Of course, any time is ok!

Copy link
Owner

@ktr0731 ktr0731 left a comment

Choose a reason for hiding this comment

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

I tried another snippet, but it got strange behavior. The second prompt didn't appear.
#46

It seems that PollEvent is returning nil because the Screen is finalized in the previous session.

@tjmtmmnk
Copy link
Contributor Author

tjmtmmnk commented Mar 2, 2021

I cloud reproduce the same problem...
I'm getting a little busy too so it may take some time to investigate🙇‍♂️

@ktr0731
Copy link
Owner

ktr0731 commented Mar 3, 2021

No problem 👍

@tjmtmmnk tjmtmmnk force-pushed the replace-to-tcell-retry branch from 3821bcd to 5a7ebd0 Compare March 12, 2021 13:44
@tjmtmmnk
Copy link
Contributor Author

Hello, I apologize for the delay.

After checking, this problem seems to be caused by a package variable defaultFinder.

defaultFinder was singleton, so finder.term was nil in the first Find but finder.term was not nil in the second Find.

Therefore, I created newFinder func to recreate finder every time you exec Find.

I could confirm that this problem was fixed in local.


I tried to add test, but finder is recreated in test so it was difficult to check.

Copy link
Owner

@ktr0731 ktr0731 left a comment

Choose a reason for hiding this comment

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

Perfect! Looks good to me, thanks for your continuous contribution!

@@ -68,13 +64,22 @@ type finder struct {
opt *opt
}

func newFinder() *finder {
Copy link
Owner

Choose a reason for hiding this comment

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

👍

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.

2 participants