Skip to content

Stop Progress ElapsedTimeRemaining from updating - #1672

Closed
jbennink wants to merge 1 commit into
spectreconsole:mainfrom
jbennink:main
Closed

jbennink wants to merge 1 commit into
spectreconsole:mainfrom
jbennink:main

Conversation

@jbennink

Copy link
Copy Markdown

This fixes issue #1472. I read the issue and thought it should not be too hard and perfrect for a first PR. It turns out it is caused due to StopTime not being set when the maxValue is reached. I simply added one line to fix this which fixed the issue.

  • I have read the Contribution Guidelines
  • I have commented on the issue above and discussed the intended changes
  • A maintainer has signed off on the changes and the issue was assigned to me
  • All newly added code is adequately covered by tests
  • All existing tests are still running without errors
  • No documentation changes are required.

Changes

The issue occurs due to StopTime not being set when the MaxValue is readed. The Isfinished bool checks both StopTime and Value > MaxValue, but since the ElapsedTime simple checks whether StopTime is set, and that is never set except when StopTask is called this keeps on counting. By adding the assignment to StopTime in the Update method when _value > _maxValue this fixes the issue.

P.S. I did not comment on the issue since it is not a behaviour change but simply a bugfix.


Please upvote 👍 this pull request if you are interested in it.

@FrankRay78

Copy link
Copy Markdown
Contributor

Hello @jbennink, thanks for taking the time to submit this, and write up the PR description. I have only taken a cursory look, and whilst it's a one-line change (not to diminish the significance of such things), we need all changes to have unit test coverage. Have you ensured there is before/after coverage of the change, and/or should we still be expecting tests to follow for this PR?

@FrankRay78
FrankRay78 self-requested a review November 5, 2024 15:50
@FrankRay78 FrankRay78 added the bug label Nov 5, 2024
@FrankRay78 FrankRay78 added this to the 0.50 milestone Nov 5, 2024
@FrankRay78 FrankRay78 assigned FrankRay78 and jbennink and unassigned FrankRay78 Nov 5, 2024
@FrankRay78 FrankRay78 linked an issue Nov 5, 2024 that may be closed by this pull request
@FrankRay78 FrankRay78 changed the title This fixes issue #1472 to stop ElapsedTimeRemaining from updating Stop Progress ElapsedTimeRemaining from updating Nov 13, 2024
@patriksvensson patriksvensson modified the milestones: 0.50, 0.51 Nov 13, 2024
@patriksvensson

Copy link
Copy Markdown
Contributor

Hi!

Thanks so much for taking the time to submit this PR.
I really appreciate the contribution!

Unfortunately, I haven’t had the capacity to review this in a timely manner, and since it was opened, the repository has changed significantly. Because of that, this PR is no longer in a state where it can be reviewed or merged as-is. This is not your fault, but mine, and I'm sorry about this.

I’m going to close it for now to help keep things tidy. That said, if you’re still interested in contributing, please feel free to open a new pull request that’s up to date with the current state of the repository, and I’ll be happy to take a look when I can. I'll promise I'll do better this time around.

Thanks again for your work and for your interest in the project!

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.

ElapsedTimeRemaining Keeps updating

3 participants