Skip to content

Conversation

@Riksu9000
Copy link
Contributor

@Riksu9000 Riksu9000 commented Aug 10, 2022

Previously the minimum number of digits was fixed at two, padded with leading zeros if necessary. Now it will be determined from the max value.

Current:
00, 01, 02 ... 99, 100, 101 ... 999, 1000 1001

With this change:
0000, 0001, 0002 ... 0099, 0100, 0101 ... 0999, 1000, 1001

This issue doesn't happen with the current uses of the Counter.

@Riksu9000 Riksu9000 added this to the 1.11.0 milestone Aug 10, 2022
@lman0

This comment was marked as off-topic.

@Riksu9000
Copy link
Contributor Author

@lman0 I think you're misunderstanding what this PR is about. Please familiarize yourself with the code.

@lman0

This comment was marked as off-topic.

@Riksu9000
Copy link
Contributor Author

@lman0 I've updated the description, but as this is an entirely technical change and essentially a bugfix, I would rather have feedback from devs.

@JF002
Copy link
Collaborator

JF002 commented Aug 14, 2022

@lman0 Github is a developer tool, and more especially the pull request functionality : developers write changes or additions and request core-developers/maintainers to pull those changes into the main code base.
We do this in the public space, anyone is free to read and interact in the comment, but we do not expect everyone to understand everything that is being done or discussed in those comments.

As a developer, I can easily see that this PR only changes the formatting of the leading 0s based on the maximum value the widget is supposed to display. The changes are quite simple and documented enough in the description. I do not need more information.

You are of course free to ask questions and speak your opinion on any pull-requests or issues in the project, but remember that we are under no obligation to answer all of them. Answering those questions and debating endlessly on insignificant details or on topics that you seem to not understand is very time consuming for us and counter productive, and we hide comments when we feel like we are loosing our time.

If you want to contribute to the project (and I think you really want to), please try to be a bit more organized : comment on topics where you really think you can provide additional value, do not question each and every change done by developers, think a bit more before publishing your comments to avoid multiple edits afterwards, and, if you think you have a good/better idea, try to contribute it yourself.

@lman0
Copy link

lman0 commented Aug 14, 2022

@lman0 I've updated the description, but as this is an entirely technical change and essentially a bugfix, I would rather have feedback from devs.

thanks for the update !

@JF002 @Riksu9000 i've seen on other projects that tag "technical" were added to prs to say it's an entirely technical ( not visual or app ) change so user like me not pry.
that could be helpful

@Riksu9000 Riksu9000 merged commit 62c4ff9 into InfiniTimeOrg:develop Aug 21, 2022
@Riksu9000 Riksu9000 deleted the counter-leading-zeros branch August 21, 2022 11:52
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.

4 participants