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

FormatTime() and time correcting #875 #917

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

FormatTime() and time correcting #875 #917

wants to merge 3 commits into from

Conversation

komashchenko
Copy link
Contributor

Implementing time formatting without taking into account time zone changes
compatibility old plugins not be broken

@sm9cc
Copy link
Contributor

sm9cc commented Oct 31, 2018

Implementing time formatting without taking into account time zone changes
compatibility old plugins not be broken

I would also suggest changing the commit description to something like this:

Implement the ability to use local time formatting without breaking compatibility with existing plugins.

And the commit title to something like this:

Added localtime parameter to FormatTime();

Other than that, I see no reason why we can't merge this.

@peace-maker
Copy link
Member

What's your use case?

@komashchenko
Copy link
Contributor Author

What's your use case?

Necessary to record time UTC in database.

@Kenzzer
Copy link
Member

Kenzzer commented Nov 4, 2018

I would have a use for this one too

@Headline Headline added the Feature Request user requested feature label Nov 8, 2018
@asherkin
Copy link
Member

It looks we don't have any way to bypass sm_time_adjustment, which makes this not-very-useful for people using that for timezone correction.

@KyleSanderson
Copy link
Member

I'm still struggling to see the usefulness of sm_time_adjustment. I'm not sure I've seen any other application out there have a similar mechanism that's actively in-use by people.

@asherkin
Copy link
Member

It’s so people who rent severs from a GSP can set the timezone correctly.

sm9cc and others added 2 commits November 13, 2018 19:00
Co-Authored-By: komashchenko <komashchenko@users.noreply.github.com>
@DarkDeviL
Copy link

What's your use case?

Necessary to record time UTC in database.

MySQL/MariaDB UTC_TIMESTAMP(), maybe ?

It’s so people who rent severs from a GSP can set the timezone correctly.

If things should change, it would make more sense to make an entry in addons/sourcemod/configs/core.cfg for the server's timezone, similar to the Linux system's /etc/timezone, e.g. "America/New_York" or "Europe/London".

A similar per-player setting could then be introduced, so it can also be adjusted for each individual player.

The latest "localtime" bool variable of FormatTime could then be the client id, of which it should be changed according to, e.g. "0" (which would be default) would imply the server, and be relative to the setting from core.cfg, where anything else would point towards a specific client id.

Or, maybe for simplicity, the latest "localtime" variable could simply just be a text string, e.g. "America/New_York" or "Europe/London".

@Headline
Copy link
Member

Headline commented Nov 27, 2018

I think it'd be more useful to have this option in GetTime, rather than in FormatTime.

@KyleSanderson
Copy link
Member

@asherkin before I ping komashchenko on this one - how do you feel about it?

I can see it being useful, but additionally with FormatTime I always included the TZ as end-consumers already had a hint about where things were hosted (per latency to the box). You're right for GSPs this scenario is useful.

Anyway, I'm conflicted because it's legitimately changing time underneath people. In a totally amateur plugin I wrote in 2009 I have FormatTime(Clock, sizeof(Clock), "%I:%M:%S%p %Z", TimeCache); which to me shows this is beyond user error.

@komashchenko
Copy link
Contributor Author

@KyleSanderson At the time of creation, I wanted to use it with TZD, since I was getting the player's local time, then when formatting via FormatTime it turned out to be wrong, so I just added myself the TZD_FormatTime function

@awillinger
Copy link
Contributor

To reactivate this old PR: I would also have a use case for this: Using SourceTV Manager, I've written a plugin which automatically starts/stops recording of server-side demos and uploads them to a FTP server so they can be downloaded.

To avoid people having to know which time zone the server is in, I would like to store the recording start time as a UTC string in the filename, which currently is not possible, as FormatTime uses the server's timezone, which can either be EST or CE(S)T in my case. Moreover, I've also written a script which automatically links said demos to bans in Sourcebans for evidence purposes and which currently requires extra (and imo unnecessary) code for timezone adjustment per server.

Not sure what else is preventing this PR from being merged, doesn't seem like it breaks bcompat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request user requested feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants