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

I think the variable about time should not be set to time.Now() #195

Closed
chengxilo opened this issue Aug 29, 2024 · 2 comments · Fixed by #196
Closed

I think the variable about time should not be set to time.Now() #195

chengxilo opened this issue Aug 29, 2024 · 2 comments · Fixed by #196

Comments

@chengxilo
Copy link
Contributor

chengxilo commented Aug 29, 2024

I realized that we have this method

progressbar/progressbar.go

Lines 361 to 368 in 03fc4e9

func getBasicState() state {
now := time.Now()
return state{
startTime: now,
lastShown: now,
counterTime: now,
}
}

it was called by NewOption64

progressbar/progressbar.go

Lines 319 to 334 in 03fc4e9

func NewOptions64(max int64, options ...Option) *ProgressBar {
b := ProgressBar{
state: getBasicState(),
config: config{
writer: os.Stdout,
theme: defaultTheme,
iterationString: "it",
width: 40,
max: max,
throttleDuration: 0 * time.Nanosecond,
elapsedTime: max == -1,
predictTime: true,
spinnerType: 9,
invisible: false,
},
}

So even we didn't render it, the lastShown time was set to time.Now() which is not so good for futher development. I think this variable shouldn't be set when the progress was render instead of called when the progress bar was just initialized.

We can use IsZero() to check whether the time is default value. And in my opinion, this change will make it possible for us to check whether the bar has been rendered before. If we could do that, we can add some goroutine for rendering the progress bar. Such as fix #141 , if we can check whether the progress bar start rendering, we can start a goroutine for it and re-render the progress bar according to the time interval.

var n time.Time
fmt.Println(n.IsZero()) // output: true

And I think these arguments apply equally to lastShown and counterTime

I would like to ask your opinion on whether it needs to be changed and if it is ok, I'd like to pr for it.

@chengxilo chengxilo changed the title I think the variable lastShown,startTime should not be set to time.Now() I think the variable about time should not be set to time.Now() Aug 29, 2024
@schollz
Copy link
Owner

schollz commented Aug 31, 2024

Yeah! That is a good idea, please feel free to make a pr

@chengxilo
Copy link
Contributor Author

chengxilo commented Aug 31, 2024

Yeah! That is a good idea, please feel free to make a pr

Thank you for you reply.I will handle it next week.🐭

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 a pull request may close this issue.

2 participants