Skip to content

Conversation

@molor
Copy link

@molor molor commented Jun 23, 2021

Information

This PR closes #4198.

Thanks to @DebugOk for making me want to learn git

Details

Now the datetime of first player join can be viewed in /whois (also in /seen with essentials.seen.joined permission).

It also adds a permission essentials.seen.uuid to display UUID in /seen, because I think that this info need only for admins (that have this permission anyway), not the players who don't even know what it is.

Proposed feature:

Add ability to display player first join datetime in /whois and /seen.

Environments tested:

OS: Windows 10 x64; Windows Server 2012 R2 x64

Java version: 16 x64

  • Most recent Paper version (1.16.5, git-Paper-777)
  • CraftBukkit/Spigot/Paper 1.12.2
  • CraftBukkit 1.8.8

Demonstration:

(a little outdated)
seen:
image
whois:
image

@imDaniX
Copy link
Contributor

imDaniX commented Jun 24, 2021

Should it really be displayed in /seen command tho? I think this is not the kind of data that you usually want to check quickly.

@JRoy
Copy link
Member

JRoy commented Jun 24, 2021

Should it really be displayed in /seen command tho? I think this is not the kind of data that you usually want to check quickly.

This is out of the scope of the seen command, should be whois only.

@pop4959
Copy link
Member

pop4959 commented Jun 24, 2021

Maybe "Joined" instead of "First seen" as well.

Not sure about the date format. Some locales use a different date format, and you seem to be hard coding it here. Easiest would probably just be to remove it and just have the datediff (plus this avoids wrapping to a new line which looks cleaner).

Edit: Be sure to update permissions as well, and the UUID permission change should probably be its own PR.

@pop4959 pop4959 added module: main Issues or PRs for the main Essentials module type: enhancement Features and feature requests. labels Jun 24, 2021
@molor
Copy link
Author

molor commented Jun 24, 2021

Should it really be displayed in /seen command tho? I think this is not the kind of data that you usually want to check quickly.

First seen and last seen, why not? :D
I have several times faced the need to find out when the (offline!) player joined for the first time. And there is no way to find out this except by reading the server logs. Adding this information to /seen can solve this problem.

Not sure about the date format. Some locales use a different date format, and you seem to be hard coding it here. Easiest would probably just be to remove it and just have the datediff (plus this avoids wrapping to a new line which looks cleaner).

It is a problem with the date output. For example, my helper/moderator and I need to know the exact date and time when the player first join (without having to look at the server logs), but at the same time it may be enough for others (including you) to know how long ago it was.. How to solve this problem so that it is good for everyone?

@molor
Copy link
Author

molor commented Jun 24, 2021

Updated demonstrations — it looks better now, but need to do something with full date if current implementation suits only me and my helper..

@imDaniX
Copy link
Contributor

imDaniX commented Jun 24, 2021

Not sure about the date format. Some locales use a different date format, and you seem to be hard coding it here.

Btw #3710 uses similar date format. I think it should be configurable through the config and shared across the plugin code.

@pop4959
Copy link
Member

pop4959 commented Jun 25, 2021

Not sure about the date format. Some locales use a different date format, and you seem to be hard coding it here.

Btw #3710 uses similar date format. I think it should be configurable through the config and shared across the plugin code.

Good idea. We could probably expose this in the translations and let translators deal with it. I think this is the right approach at least, if we'd like to keep the date format in here.

@pop4959
Copy link
Member

pop4959 commented Jun 26, 2021

Another idea for @molor, not sure why I hadn't thought of this yesterday. You could include the date format but just not make it the default. This is done with many other messages in Essentials. For example, include {0} as the datediff, and {1} as the date timestamp, but in the basic translation just leave it as {0}. Then, you (or anyone else) can decide to switch the format to {1} if they prefer it that way. 👏

Copy link
Member

@pop4959 pop4959 left a comment

Choose a reason for hiding this comment

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

Aside from the other comments I left, you should still refactor the date format to at minimum remain consistent with what is done in #3710, if not also exposing it in the translations (which I would recommend doing while you're at it). It shouldn't just be copy/pasted all over the place.

@molor molor requested a review from pop4959 July 28, 2021 20:18
@mdcfe
Copy link
Member

mdcfe commented Jul 29, 2021

I'm also iffy on this being in /seen - this feels like it belongs in /whois only.

@molor molor requested a review from JRoy July 31, 2021 20:22
@TNTUP
Copy link

TNTUP commented Aug 1, 2021

What happens with older players that have joined after this PR gets merged? They'll be shown as all "Joined today/yesterday" or it will guess (Based on the playerdata of players in world/playerdata?

@JRoy
Copy link
Member

JRoy commented Aug 1, 2021

What happens with older players that have joined after this PR gets merged? They'll be shown as all "Joined today/yesterday" or it will guess (Based on the playerdata of players in world/playerdata?

It's based on org.bukkit.Player#getFirstPlayed which uses regular mc playerdata

@molor
Copy link
Author

molor commented Aug 7, 2021

So this PR is partially useless [for me] now

need to find out when the (offline!) player joined for the first time. And there is no way to find out this

@molor
Copy link
Author

molor commented Aug 9, 2021

Timed out

@molor molor closed this Aug 9, 2021
@TNTUP
Copy link

TNTUP commented Aug 9, 2021

Rip PR

@imDaniX
Copy link
Contributor

imDaniX commented Aug 12, 2021

What about implementing some kind of /offlinewhois?

@pop4959
Copy link
Member

pop4959 commented Aug 12, 2021

What about implementing some kind of /offlinewhois?

So /seen?

@imDaniX
Copy link
Contributor

imDaniX commented Aug 13, 2021

So /seen?

/whois is a more descriptive version of /seen (or vise versa, idk), but, well, @molor's problem is that it doesn't work with offline players. Surely, we can't get all the /whois info about an offline player without some funky workarounds, but some of it, including first join, can be.

@oskarkk
Copy link

oskarkk commented Aug 14, 2021

What about implementing some kind of /offlinewhois?

So /seen?

As you can see in the comments above, for some reasons you cannot put anything more into the /seen command. I don't understand why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module: main Issues or PRs for the main Essentials module type: enhancement Features and feature requests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants