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

Allow users to display local time in profile #27748

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

JakobDev
Copy link
Contributor

Implements https://codeberg.org/forgejo/forgejo/issues/1089

This allows users to set their timezone in the profile settings, so their local time will be displayed on the profile.

Where does the data came from?
From this CSV

Why use the CSV instead of the built-in timezone support of Go?

  1. Go don't provide a function to list all available timeszones. There are workaround to this, but they are just not more than workarounds.
  2. Go uses the timezone information from the system. You can compile timezone iformation in the binary, but Go still prefers the system timezones. by using the CSV we can be sure every Gitea instance uses the same information by default.
  3. Admins can easily update the timezones if needed

Screenshoots:
grafik
grafik

As usual, the migration will be added when everything reviewed

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 23, 2023
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 23, 2023
@github-actions github-actions bot added modifies/translation modifies/api This PR adds API routes or modifies them modifies/internal labels Oct 23, 2023
@puni9869
Copy link
Member

Just a thought and question.

Can't we do it in simple js.
Getting the local time zone and set the UTC to local.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getTimezoneOffset

@JakobDev
Copy link
Contributor Author

If we use JS, we can't return the current time with the API. We also can't be sure every users sees the same time. Maybe a timezone changes e.g daylight saving time. In this case, the users will eventually see different times on different devices, depending how up to date it is. Also some people want to use at least JS as possible.

@puni9869
Copy link
Member

Also some people want to use at least JS as possible.

Include me that,

I understood your solution. Give me today's time to review this change. Will share my findings.

Copy link
Member

Choose a reason for hiding this comment

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

For anyone wondering, this file hardcodes any timezone change from 1893 to 2499.
Hmm… How much extra strain is that on any instance?
I can't imagine it to be small, my current estimate is something like 7MB.
Is my calculation correct?

Copy link
Member

Choose a reason for hiding this comment

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

Do we want that much "bloat"?

Copy link
Member

Choose a reason for hiding this comment

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

Is that .gz and still 7mb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without gzip, the file is currently 6.05 MB, which gzip it's 844 KB. I will rewrite 5the Code to use a gziped file.

Copy link
Member

Choose a reason for hiding this comment

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

IMO gzip is fine. I am okay with that.

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 CSV is now compressed

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @JakobDev I have reviewed the half Pr, Need todays'time more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was able to reduce the size drastically. The CSV now only contains the info from 5 years ago until 30 years in the future. It also only includes the needed columns. We are now at 45KB.

@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them modifies/internal modifies/translation size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants