fix issue with double output on windows#458
Conversation
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
Hi, sorry for the late review and thanks for helping fix this. I think what this PR would need is some kind of regression test that fails without this fix being applied and will fail in the future if this regresses.
|
|
||
| lineStr := i.answer | ||
|
|
||
| i.AppendRenderedText(lineStr) |
There was a problem hiding this comment.
This code gets called on all platforms and its implementation doesn't differ between platforms. Do you know how it could have caused problems only on Windows but not elsewhere?
There was a problem hiding this comment.
I am not entirely sure what happens on windows to cause this problem, I can try and dig deeper but my only guess right now has to do with the carriage return, as it seems to render the same text twice and this was to remove the second one, which is probably totally wrong. I did do testing on Ubuntu as well and found there was no difference in outputs of my test between before and after my change. Obviously I could be missing some manual test cases so if there are more I can do let me know.
I was trying to add one of these but the functional tests do not run on windows (as far as I can tell from these comments: https://github.com/go-survey/survey/blob/b54ce3849e4770c86edea7108c23e8e2c89fea2b/renderer_posix_test.go#L1-L2) to begin with so it is kinda hard. If you have a suggestion for any tests let me know and I will get on it. |
Ah, you are right. I'll investigate this further with your changes as a starting point. Thank you! |
No problem, let me know if I can be of any help. |
|
@miniscruff Could you please try #474? |
That seems to have fixed the problem for me in windows terminal yes. |
|
@mislav your PR will take care of the issue so we can probably close this PR correct? |
|
Yes. Thanks for the reminder! |
2 participants
I was able to fix the double output issues on windows by removing this line, did a manual test on Ubuntu as well. My guess is this is not the best solution so let me know.
Fixes #406
Fixes #368