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

Introduce the "Prefer absolute timestamps" setting for users that want to disable relative time #24342

Closed
wants to merge 12 commits into from

Conversation

yardenshoham
Copy link
Member

@yardenshoham yardenshoham commented Apr 25, 2023

Apparently, some users prefer all timestamps to display the full date instead of relative time. They can do that now by setting it in their appearance preferences.
image
image

This setting is set to false by default, allowing dates to render as "5 hours ago". Here are some screenshots of the UI with this setting set to true:
image
image
image
image
image

Changes

  1. Refactor a bit to get context into TimeSince, pass context in templates that call TimeSince
  2. Allow extra attributes in DateTime so we can pass the time-since class
  3. Add a new setting in the user's appearance page

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 25, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 25, 2023
@yardenshoham yardenshoham added the type/enhancement An improvement of existing functionality label Apr 25, 2023
@yardenshoham yardenshoham added this to the 1.20.0 milestone Apr 25, 2023
@silverwind
Copy link
Member

silverwind commented Apr 25, 2023

How about TIME_FORMAT = relative | absolute? That would leave room for other formats and is more clear of an option imho. Should note that it does not affect all dates, as some are still absolute regardless of setting.

@yardenshoham
Copy link
Member Author

I can't think of other possible values except the ones you mentioned

@silverwind
Copy link
Member

silverwind commented Apr 25, 2023

Hmm, I would invert the setting at least and name it like "Force Absolute Timestamps". Settings which default to false seem better in general.

Also, I think ideally this would be a per-user setting, not per-server because as you said, it's a user preference, not an admin preference.

@silverwind
Copy link
Member

I think it may be worth it to introduce a block in the user appearance settings for options like this, stored server-side. I have a few other settings in mind for this as well, like tab size preference.

@wxiaoguang
Copy link
Contributor

I think we can postpone this setting to per-user setting. And we might have found a perfect context-function solution at that time.

@yardenshoham
Copy link
Member Author

What's the process for adding a setting to the user's appearance page?

@wxiaoguang
Copy link
Contributor

What's the process for adding a setting to the user's appearance page?

"progress": There are already some setting, eg: hide some comments

@yardenshoham
Copy link
Member Author

Do I need to add a migration? Any PR I can look at to model mine?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 26, 2023

Do I need to add a migration? Any PR I can look at to model mine?

No migration is needed. The user setting system is flexible

image

@yardenshoham
Copy link
Member Author

I will try

@silverwind
Copy link
Member

silverwind commented Apr 26, 2023

Thanks, I'm looking forward to it. It'll be helpful to have as a reference for future PRs that add user settings.

@confusedsushi
Copy link
Contributor

I would love to see this soon in Gitea. A per user setting is appreciated, but being able to set this server wide is much more important to me.

@yardenshoham yardenshoham marked this pull request as draft April 26, 2023 15:53
@yardenshoham yardenshoham changed the title Introduce ALLOW_RELATIVE_TIME setting for users that want to disable relative time Introduce the "Force absolute timestamp" setting for users that want to disable relative time Apr 26, 2023
…e setting

Signed-off-by: Yarden Shoham <git@yardenshoham.com>
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 26, 2023
@yardenshoham
Copy link
Member Author

I managed to add the setting to the appearance page but I now need the context user in TimeSince so I can load the setting. How can I get it? I need it in since.go

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 27, 2023

I think we can postpone this setting to per-user setting. And we might have found a perfect context-function solution at that time.

Need "a perfect context-function solution" #24228

@silverwind
Copy link
Member

I would love to see this soon in Gitea. A per user setting is appreciated, but being able to set this server wide is much more important to me.

Not sure we want it. Personal preference is important and I wouldn't want to see a admin forcing one particular style onto users. What can be global is a default style, but users need to be able to change it.

@yardenshoham
Copy link
Member Author

I don't think the problem is context now. I have an import cycle, how can I avoid it?

@confusedsushi
Copy link
Contributor

I would love to see this soon in Gitea. A per user setting is appreciated, but being able to set this server wide is much more important to me.

Not sure we want it. Personal preference is important and I wouldn't want to see a admin forcing one particular style onto users. What can be global is a default style, but users need to be able to change it.

In company environments forcing user settings is often the better choice. For a company it is important that everyone is using the tools the same way. Documentation how tools have to be configured is often not read carefully. If an admin can force certain settings you can avoid misconfiguration.

Nevertheless, I appreciate that setting, even if every user needs to configure it by its own.

@wxiaoguang
Copy link
Contributor

I don't think the problem is context now. I have an import cycle, how can I avoid it?

I would still recommend to wait for a perfect context support.

The last time for such "context passing trick" is the avatar, it causes a lot of 500, not only one PRs were trying to fix these bugs.

@yardenshoham
Copy link
Member Author

Even if I wait for that, I would still have the issue with the import cycle, right?

Co-authored-by: silverwind <me@silverwind.io>
@silverwind
Copy link
Member

@yardenshoham what do you think about switchting to string option absolute | relative (e.g. dropdown) and adding the ui.DEFAULT_TIMESTAMP_FORMAT to set the default? I think it's just more understandable than a boolean option.

@yardenshoham
Copy link
Member Author

The thing is, if you choose relative, some dates will still display as absolute so it might confuse users

@silverwind
Copy link
Member

silverwind commented Apr 28, 2023

Right, it's not used everywhere. Let's rename the vars and such to "absoluteTimestamps" and if you agree, also add the setting ui.DEFAULT_ABSOLUTE_TIMESTAMPS to obtain the default and satisfy the request above.

@yardenshoham
Copy link
Member Author

Sounds good, I'm on it

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 29, 2023

There are some possible choices:

  1. Do we need a global ini setting?
  2. Do we want to support per-user setting?
  3. Do we want to avoid using $.Context in templates?

For "global setting", I think it's better to have a new inheritable config system (but not ini): global setting -> org/user setting -> repo setting, instead of adding many global ini options (maintainability?), there will be a lot of similar requirements: global-2FA/org-2FA, global merge style / org merge style, etc.

If we agree for choice-2, then choice-1 seems not necessary, and we had better do choice-3.

My personal opinion is: at the moment only do 2+3 (per-user + wait for template context function)

What do you think?

@silverwind
Copy link
Member

silverwind commented Apr 29, 2023

Yes I agree global setting is not needed in case we are planning for a better config system later which also covers per-org settings. I'm still of the opinion that per-user setting is enough as a start here, and admins don't really need "power" over this imho, as it seems like power abuse to force stylistic choices onto users.

So let's just rename and then I'm happy to approve.

@yardenshoham yardenshoham changed the title Introduce the "Force absolute timestamp" setting for users that want to disable relative time Introduce the "Prefer absolute timestamps" setting for users that want to disable relative time Apr 29, 2023
@@ -76,7 +76,7 @@
{{$.locale.Tr "repo.released_this"}}
</span>
{{if .CreatedUnix}}
<span class="time">{{TimeSinceUnix .CreatedUnix $.locale}}</span>
<span class="time">{{TimeSinceUnix $.Context .CreatedUnix $.locale}}</span>
Copy link
Member Author

Choose a reason for hiding this comment

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

Why does this not work when a user is unauthenticated but nil is fine?

Suggested change
<span class="time">{{TimeSinceUnix $.Context .CreatedUnix $.locale}}</span>
<span class="time">{{TimeSinceUnix nil .CreatedUnix $.locale}}</span>

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't, in the worse case, $.Context be nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

The error:

Render failed, failed to render template: repo/release/list, error: template error: builtin(static):repo/release/list:80:30 : executing "repo/release/list" at <TimeSinceUnix $.Context .CreatedUnix $.locale>: error calling TimeSinceUnix: runtime error: invalid memory address or nil pointer dereference
----------------------------------------------------------------------
									<span class="time">{{TimeSinceUnix $.Context .CreatedUnix $.locale}}</span>
									                     ^
----------------------------------------------------------------------

Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't looked into details, just share some my experiences:

  1. Golang template is not friendly to panic text/template: provide full stacktrace when the function call panics golang/go#59400 , you need to write some temp code to do "recover" to see what happens.
  2. According to Golang standard, ctx should never be nil. The Golang standard library functions will panic if you feed nil to them.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 29, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 29, 2023

If we are not in a hurry, I would suggest to leave enough time for this PR.

I wouldn't put Request Changes, but there are indeed some problems which need enough attention:

  1. The "context" problem, it's not safe to fill .Context in templates. It's better to avoid doing so. Comment Introduce the "Prefer absolute timestamps" setting for users that want to disable relative time #24342 (comment)
  2. The performance, think about a case, there are 100 datetime components on a page, then the GetSetting will send 100 database queries, which is very unfriendly to database and slows down the whole system.

@yardenshoham
Copy link
Member Author

I understand and agree

@yardenshoham yardenshoham deleted the issues/22493 branch April 29, 2023 14:09
@silverwind
Copy link
Member

The performance, think about a case, there are 100 datetime components on a page, then the GetSetting will send 100 database queries, which is very unfriendly to database and slows down the whole system.

Right, it should be one database query per request, maybe even with an additional caching mechanism on top.

@wxiaoguang
Copy link
Contributor

I am doing my best to improve the template system now, hopefully we can start the "per-user setting for all templates" work soon.

Thank you! 🙏

@yardenshoham
Copy link
Member Author

If you want we could go back to the server-wide option

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 29, 2023

If I failed to get the context function working before 1.20 frozen, let's go back to the global ini option solution, I also agree that it brings true values for end users. 👍

@silverwind
Copy link
Member

Still I don't quite understand why this was closed as the issues would surely have been fixable, or does it really depend on this upcoming context rewrite?

@GiteaBot GiteaBot removed this from the 1.20.0 milestone Apr 29, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add preference setting for commit time display format viewing dates requires mouse hover
5 participants