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

Add confirmation on new player registration #6849

Merged
merged 4 commits into from Jan 13, 2018

Conversation

Projects
None yet
@srifqi
Contributor

srifqi commented Dec 27, 2017

It will show a confirmation dialog for new player not in singleplayer mode.

Using SRP auth mechanism, if server sent AUTH_MECHANISM_FIRST_SRP that means the player isn't exist. This is not supported by older network protocol.

Fixes #258 and closes #3167.

TODO:

  • How to display the confirmation dialog.
  • Ways to convert from std::wstring to std::string and vice versa (for comparing and swprintf()).
  • Use template to also show the server (IP address) and the name which the player chose.

confirmation dialog for new player

(Old screenshot) https://user-images.githubusercontent.com/4017302/34456483-e3f3479a-edc9-11e7-9d23-d3d155b2d872.png
(Older screenshot) https://user-images.githubusercontent.com/4017302/34455891-70557dac-edbb-11e7-9003-ab7f7359d545.png

@lisacvuk

This comment has been minimized.

Show comment
Hide comment
@lisacvuk

lisacvuk Dec 27, 2017

Contributor

Great idea, sometimes I start typing my password hoping that password box has focus, and that way selecting random server, and logging in to that one instead.

Contributor

lisacvuk commented Dec 27, 2017

Great idea, sometimes I start typing my password hoping that password box has focus, and that way selecting random server, and logging in to that one instead.

@srifqi

This comment has been minimized.

Show comment
Hide comment
@srifqi

srifqi Dec 30, 2017

Contributor

Thanks to @red-001 for the idea of using code from guiKeyChangeMenu.cpp/.h, but I use code from guiPasswordChange.cpp/.h. I've figured out how to display the form.

Now it's the problem about converting std::string to std::wstring and vice versa.

Contributor

srifqi commented Dec 30, 2017

Thanks to @red-001 for the idea of using code from guiKeyChangeMenu.cpp/.h, but I use code from guiPasswordChange.cpp/.h. I've figured out how to display the form.

Now it's the problem about converting std::string to std::wstring and vice versa.

@srifqi

This comment has been minimized.

Show comment
Hide comment
@srifqi

srifqi Dec 30, 2017

Contributor

Thanks to @sfan5 for pointing out the functions I need to do std::string to std::wstring conversion and vice versa.

Contributor

srifqi commented Dec 30, 2017

Thanks to @sfan5 for pointing out the functions I need to do std::string to std::wstring conversion and vice versa.

@srifqi srifqi changed the title from [WIP] Add confirmation on new player registration to Add confirmation on new player registration Dec 30, 2017

@srifqi

This comment has been minimized.

Show comment
Hide comment
@srifqi

srifqi Jan 2, 2018

Contributor

Ready for review. Please remove the WIP tag.

Contributor

srifqi commented Jan 2, 2018

Ready for review. Please remove the WIP tag.

@SmallJoker SmallJoker removed the WIP label Jan 2, 2018

@DS-Minetest

This comment has been minimized.

Show comment
Hide comment
@DS-Minetest

DS-Minetest Jan 2, 2018

Contributor

Imo the message could be a bit better, it's so long.

Contributor

DS-Minetest commented Jan 2, 2018

Imo the message could be a bit better, it's so long.

@benrob0329

This comment has been minimized.

Show comment
Hide comment
@benrob0329

benrob0329 Jan 2, 2018

"Please confirm your password for player registration on <foo>"

benrob0329 commented Jan 2, 2018

"Please confirm your password for player registration on <foo>"

@Ezhh

This comment has been minimized.

Show comment
Hide comment
@Ezhh

Ezhh Jan 2, 2018

Member

Tiny thing: in any documentation like this it would be expected to use "please type" instead of "please enter". I also feel the text is a bit long in general; young player's simply will not read it, so it should be shortened if possible.

With that said - love the idea.

Member

Ezhh commented Jan 2, 2018

Tiny thing: in any documentation like this it would be expected to use "please type" instead of "please enter". I also feel the text is a bit long in general; young player's simply will not read it, so it should be shortened if possible.

With that said - love the idea.

@rubenwardy

Nice feature. Here is a first pass over. I'll have to have a look through the protocol to make sure I fully understand the handshake.

Show outdated Hide outdated src/client.cpp Outdated
Show outdated Hide outdated src/client.h Outdated
Show outdated Hide outdated src/game.cpp Outdated
Show outdated Hide outdated src/game.cpp Outdated
Show outdated Hide outdated src/game.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
@rubenwardy

This comment has been minimized.

Show comment
Hide comment
@rubenwardy

rubenwardy Jan 2, 2018

Member

I agree that the text is a bit long and could be improved, not feeling particularly wordy today however

Member

rubenwardy commented Jan 2, 2018

I agree that the text is a bit long and could be improved, not feeling particularly wordy today however

Show outdated Hide outdated src/client/renderingengine.cpp Outdated
Show outdated Hide outdated src/client.cpp Outdated
Show outdated Hide outdated src/game.cpp Outdated
Show outdated Hide outdated src/game.cpp Outdated
Show outdated Hide outdated src/game.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
Show outdated Hide outdated src/network/clientpackethandler.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.h Outdated
Show outdated Hide outdated src/game.cpp Outdated
@red-001

This comment has been minimized.

Show comment
Hide comment
@red-001

red-001 Jan 2, 2018

Contributor

pretty sure you don't need all the boiler plate code, most of that is in GUIModalMenu

Contributor

red-001 commented Jan 2, 2018

pretty sure you don't need all the boiler plate code, most of that is in GUIModalMenu

@srifqi

This comment has been minimized.

Show comment
Hide comment
@srifqi

srifqi Jan 3, 2018

Contributor

Fix "unable to do translation with different format position" by using %1$s instead of just %s.


Imo the message could be a bit better, it's so long.

I agree that the text is a bit long and could be improved, not feeling particularly wordy today however

I agree. I just copied from what @Wuzzy2 wrote on another issue.


pretty sure you don't need all the boiler plate code, most of that is in GUIModalMenu

How?

Contributor

srifqi commented Jan 3, 2018

Fix "unable to do translation with different format position" by using %1$s instead of just %s.


Imo the message could be a bit better, it's so long.

I agree that the text is a bit long and could be improved, not feeling particularly wordy today however

I agree. I just copied from what @Wuzzy2 wrote on another issue.


pretty sure you don't need all the boiler plate code, most of that is in GUIModalMenu

How?

Show outdated Hide outdated src/game.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
Show outdated Hide outdated src/client.cpp Outdated
Show outdated Hide outdated src/client.h Outdated
Show outdated Hide outdated src/game.cpp Outdated
Show outdated Hide outdated src/game.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
@srifqi

This comment has been minimized.

Show comment
Hide comment
@srifqi

srifqi Jan 8, 2018

Contributor

Ah, branch conflict. I'll rebase.

BTW, do we agree for this for information text?

You are about to join this server (%1$s) with the name "%2$s" the first time. If you proceed, a new account using your credentials will be created on this server.
Please enter your password once again to confirm account creation or cancel to abort.

Contributor

srifqi commented Jan 8, 2018

Ah, branch conflict. I'll rebase.

BTW, do we agree for this for information text?

You are about to join this server (%1$s) with the name "%2$s" the first time. If you proceed, a new account using your credentials will be created on this server.
Please enter your password once again to confirm account creation or cancel to abort.

srifqi added some commits Dec 27, 2017

Attempt to add registration confirmation
Using SRP auth mechanism, if server sent AUTH_MECHANISM_FIRST_SRP that means the player isn't exist.
Also tell player about the server and chosen username.
Local game has localhost as IP address of the server.
Add RenderingEngine::draw_menu_scene() to draw GUI and clouds background.
aborted -> connection_aborted
Rewrite information message text
Client::promptConfirmRegister() -> Client::promptConfirmRegistration()
@srifqi

This comment has been minimized.

Show comment
Hide comment
@srifqi

srifqi Jan 11, 2018

Contributor

Rebased and squashed.

New commit rewriting information message text and renaming function.

Contributor

srifqi commented Jan 11, 2018

Rebased and squashed.

New commit rewriting information message text and renaming function.

@SmallJoker

Works

@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jan 12, 2018

Member

i restarted failed build. Lint should be fixed here https://travis-ci.org/minetest/minetest/jobs/327520632

Member

nerzhul commented Jan 12, 2018

i restarted failed build. Lint should be fixed here https://travis-ci.org/minetest/minetest/jobs/327520632

@srifqi

This comment has been minimized.

Show comment
Hide comment
@srifqi

srifqi Jan 12, 2018

Contributor

Build run successfully. Travis check passed.

Contributor

srifqi commented Jan 12, 2018

Build run successfully. Travis check passed.

Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
Show outdated Hide outdated src/gui/guiConfirmRegistration.cpp Outdated
Code style fix (2)
"enter" -> "type"
Increase info_text_buf size.

@nerzhul nerzhul merged commit 7927529 into minetest:master Jan 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nerzhul

This comment has been minimized.

Show comment
Hide comment
@nerzhul

nerzhul Jan 13, 2018

Member

Merged, thanks for your work

Member

nerzhul commented Jan 13, 2018

Merged, thanks for your work

@srifqi

This comment has been minimized.

Show comment
Hide comment
@srifqi

srifqi Jan 13, 2018

Contributor

Thanks for the merge and you're welcome!

Contributor

srifqi commented Jan 13, 2018

Thanks for the merge and you're welcome!

@srifqi srifqi deleted the srifqi:newloginregister branch Jan 13, 2018

@jastevenson303

This comment has been minimized.

Show comment
Hide comment
@jastevenson303

jastevenson303 Jan 22, 2018

Contributor

Wow. I went to go test something real quick and was presented with this block of text and a form, and I became confused and upset. The form is the worst part, because it's asking me to retype my password, but I never typed one in the first place!

Contributor

jastevenson303 commented Jan 22, 2018

Wow. I went to go test something real quick and was presented with this block of text and a form, and I became confused and upset. The form is the worst part, because it's asking me to retype my password, but I never typed one in the first place!

@srifqi

This comment has been minimized.

Show comment
Hide comment
@srifqi

srifqi Jan 22, 2018

Contributor

You didn't use password? I thought Minetest already forbid that.

Maybe we can make password to be required in order to create account?

Yes, the information text is verbose.

Contributor

srifqi commented Jan 22, 2018

You didn't use password? I thought Minetest already forbid that.

Maybe we can make password to be required in order to create account?

Yes, the information text is verbose.

@Ezhh

This comment has been minimized.

Show comment
Hide comment
@Ezhh

Ezhh Jan 22, 2018

Member

There is a "disallow empty password" setting. I think it's good to have this as an option for testing purposes and the screen should probably be skipped when this setting is set to true.

Member

Ezhh commented Jan 22, 2018

There is a "disallow empty password" setting. I think it's good to have this as an option for testing purposes and the screen should probably be skipped when this setting is set to true.

t0ny2 pushed a commit to t0ny2/minetest that referenced this pull request Jan 23, 2018

Add confirmation on new player registration (minetest#6849)
* Attempt to add registration confirmation

Using SRP auth mechanism, if server sent AUTH_MECHANISM_FIRST_SRP that means the player isn't exist.
Also tell player about the server and chosen username.
Local game has localhost as IP address of the server.
Add RenderingEngine::draw_menu_scene() to draw GUI and clouds background.
aborted -> connection_aborted

* Rewrite information message text

Client::promptConfirmRegister() -> Client::promptConfirmRegistration()
@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat
Member

paramat commented Apr 13, 2018

See #7237

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