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

progress: Add editMessage to allow atomic updating of tracker.Message #164

Closed
wants to merge 1 commit into from

Conversation

virtuald
Copy link
Contributor

You're going to hate me... 😎

Thanks so much for your work!

@@ -195,13 +195,15 @@ func (p *Progress) moveCursorToTheTop(out *strings.Builder) {
}

func (p *Progress) renderTracker(out *strings.Builder, t *Tracker, hint renderHint) {
message := t.message()
if strings.Contains(message, "\t") {
t.Message = strings.Replace(message, "\t", " ", -1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW: If this PR isn't accepted, this should be switched to UpdateMessage regardless

Copy link
Owner

@jedib0t jedib0t Apr 22, 2021

Choose a reason for hiding this comment

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

I missed this instance in my previous change. 😞

I think the better way to do this would be to get the message value one single time, and then use it for a single render event. Give me some time to see if I can make this work.

out.WriteRune('\n')
}
}

func (p *Progress) renderTrackerMessage(out *strings.Builder, t *Tracker) {
func (p *Progress) renderTrackerMessage(out *strings.Builder, t *Tracker, message string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal with this change is to make sure that if the renderer edits the message, that the edited message is the message that is being displayed.

While not a formal race condition, without this change if UpdateMessage was called by the user between the text.Pad/text.Snip and this point the displayed message + progressbar might (for a very short period of time) not look as expected.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 775025369

  • 32 of 32 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 774792570: 0.0%
Covered Lines: 2954
Relevant Lines: 2954

💛 - Coveralls

@jedib0t
Copy link
Owner

jedib0t commented Apr 22, 2021

You're going to hate me... 😎

Thanks so much for your work!

Definitely not! 😄 I appreciate the contributions!

@jedib0t
Copy link
Owner

jedib0t commented Apr 22, 2021

Hey @virtuald ... let me know if you approve the change in #165 in lieu of this PR.

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.

3 participants