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

feat: made spent time format configurable #1341

Closed
wants to merge 8 commits into from

Conversation

Trial97
Copy link
Member

@Trial97 Trial97 commented Jul 10, 2023

fixes #554

Small changes that differ from the requirement:

  • Days:
    • %d full amount of days (rounded down). Example: "3"
    • %d.1: full amount of days, with 1 digit of precision. Example: "3.2"
    • %d.2: full amount of days, with 2 digits of precision. Example: "3.18"
  • Hours:
    • %h: amount of hours (full days deducted) (rounded down). Example: "4"
    • %h.1: amount of hours (full days deducted), with 1 digit of precision. Example: "4.4"
    • %h.2: amount of hours (full days deducted), with 2 digits of precision. Example: "4.43"
    • %H: full amount of hours (rounded down). Example: "76"
    • %H.1: full amount of hours, with 1 digit of precision. Example: "76.4"
    • %H.2: full amount of hours, with 2 digits of precision. Example: "76.43"
  • Minutes:
    • %m: amount of minutes (full days and full hours deducted) Example: "26"
    • %M: full amount of minutes. Example: "4586"
  • Seconds:
    • %s: amount of seconds (full days, full hours deducted,and full minutes deducted) Example: "26"
    • %S: full amount of seconds. Example: "4586"

The precision can be set exactly by suffixing the option with a dot and the wanted precision, e.g. %d.4 => days with digits of precision.

Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@getchoo getchoo added the enhancement New feature or request label Jul 10, 2023
launcher/MMCTime.cpp Fixed Show fixed Hide fixed
launcher/MMCTime.cpp Fixed Show fixed Hide fixed
launcher/MMCTime.cpp Fixed Show fixed Hide fixed
launcher/MMCTime.cpp Fixed Show fixed Hide fixed
Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

i really like this! a few suggestions though:

  • validate GameTimeFormat
    • this prevents issues where a user may have input improper or invalid formatting and having weird things appear in the ui. i think it would be best to revert back to the defaults in this case
  • maybe ignore the GameTimeFormat when it doesn't apply to the given instance
    • in cases where i only specify %d%d, the total played: line has no characters following it. i think it would be best to revert back to the default here as well or try to convert the playtime to that format; another solution could be to allow for per instance configuration. imo reverting to the default or converting to the measurement specified by the formatting string would be the best option here, while still being pretty simple
  • simply the syntax
    • similar to date in linux, i think just specifying %d for day, %h for hour, %m for minute, and %s for seconds would make more sense. using the same character twice in most cases (while also having two different formatting strings only for minutes) is a little odd to me and i don't believe it's something users would do on purpose a lot of the time

launcher/MMCTime.cpp Outdated Show resolved Hide resolved
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
Signed-off-by: Trial97 <alexandru.tripon97@gmail.com>
@Trial97
Copy link
Member Author

Trial97 commented Jul 11, 2023

Made most of the suggested changes. Here are some small notes:

  • I kept the extra options(%H,%M, and %S) as they are used when you want to display the duration in that respective unit only, they are meant to be used together with precision (also they are a requirement from the issue)
  • this format should be similar to the date in linux with the additions of the extra options and controllable precision
  • now if the format is empty it fallbacks to the default format
  • if the result is empty because of the trimmed zero values the function is recalled but keeps the zero everywhere(
    { 30, "%dd %hh %mmin", "0d 0h 0min" },
    )
  • I did not do validation as I think is unnecessary, if the format is not empty and doesn't contain any known options the text will be printed as is, missing only the group of options that are not valid( the character '%' and the next one)
    { 90, "custom text", "custom text" },
    { 90, "custom text =%p=", "custom text ==" },
    { 90, "custom text =%p.12=", "custom text ==" },
    { 90, "custom text =%p.=", "custom text =.=" },

@Trial97 Trial97 added the changelog:added A PR that appears under "Added" in the changelog label Aug 16, 2023
@Scrumplex
Copy link
Member

I don't think we need this much granularity for this, though? I think a simple checkbox to show the playtime in hours is fine.

@Trial97
Copy link
Member Author

Trial97 commented Aug 17, 2023

fair: I will close this after I open the other one

@Trial97
Copy link
Member Author

Trial97 commented Aug 18, 2023

closing this in favor of:#1536

@Trial97 Trial97 closed this Aug 18, 2023
@victorleblais
Copy link

I still think this would be neat as an advanced/optional feature that could be hidden behind a dropdown (with three options: days/hours, just hours, or "advanced", which shows the text input below the dropdown to type a custom format).

I always like having more options, but it's fine if they're hidden behind more clicks in order to keep the "basic behaviors" quick and accessible.

@Scrumplex
Copy link
Member

I still think this would be neat as an advanced/optional feature that could be hidden behind a dropdown (with three options: days/hours, just hours, or "advanced", which shows the text input below the dropdown to type a custom format).

I always like having more options, but it's fine if they're hidden behind more clicks in order to keep the "basic behaviors" quick and accessible.

Advanced users can just go and extract the play time in seconds from the instance.cfg file and do whatever they want with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:added A PR that appears under "Added" in the changelog enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to show playtime in hours instead of days (or even custom format)
4 participants