Some users might not want to save the password at all, so please add a tick box (maybe off by default, so an opt-out) "Remember my password".
This is also somewhat security-critical. E.g. taking the following example as a thread model: On a shared, public computer (internet cafe) the admin is so nice to install 0ad (don't ask me whether this is realistic). There, obviously I do not want the next user of the system to login as me. This is only an example! There may be other reasons and situations (multiuser setup, no disk encryption, paranoia, whatever…) why users do not want to have their passwords saved.
Also another UX aspect: Currently I do not even know that my password is saved. So probably, I think I have to use an easy one or have to save it somewhere else, so that I do not lose access. However, when I see that checkbox, I see "Ah, right, it is saving my password. How nice.".
Details
- Reviewers
elexis - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Owners Package (Owns No Changed Paths) - Commits
- rP21311: Adds remember password checkbox
- Trac Tickets
- #4708
- Run the game
- Turn on the checkbox
- Check that the password was saved
- Turn off the checkbox
- Check that the password wasn't saved
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Rest looks good.
binaries/data/mods/public/gui/lobby/prelobby.js | ||
---|---|---|
195 ↗ | (On Diff #3266) | The implementation regulates how many reports for forgotton passwords I get, so I'd prefer a confirmation message box before deleting user passwords while not having a forgotton-pw function. (In fact I'd prefer three messsage boxes before deletion, maybe a 5 minute consideration timer xD) Agree that a "delete-password" button isn't better, because people who dont want the PW saved, don't have their use case addressed. |
binaries/data/mods/public/gui/lobby/prelobby.js | ||
---|---|---|
283 ↗ | (On Diff #3269) | I'm not sure about function names, I'm open to suggestions. |
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1897/ for more details.
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1898/ for more details.
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jw:8080/job/phabricator/1899/ for more details.
binaries/data/mods/public/gui/lobby/prelobby.js | ||
---|---|---|
193 ↗ | (On Diff #3269) | (Some prefer to have the most common case first, others prefer to avoid the double negation by checking for positives in if-else statements) |
283 ↗ | (On Diff #3269) | Name functions according to what they do. So maybe toggleRemeberPassword. |
binaries/data/mods/public/gui/lobby/prelobby.xml | ||
55 ↗ | (On Diff #3269) | Either remove the unused name property, or probably better do not pass this as an argument, since the function will never be called for a different checkbox and instead use let checkbox = Engine.GetGUIObjectByName("rememberPassword")` |
57 ↗ | (On Diff #3269) | XML is a markup language and in our case deals with formatting the GUI layout. XML by definition is not supposed to contain code, while JS files should. There have been GUI files that accumulated a lot of JS code, it should be avoided as much as possible. So move it to init IMO. |
64 ↗ | (On Diff #3269) | Not sure if Password woldn't suit better. |
Thank you for the overview!
binaries/data/mods/public/gui/lobby/prelobby.xml | ||
---|---|---|
64 ↗ | (On Diff #3269) | I've met both cases on different sites, so I'm not sure too. |
Build is green
Updating workspaces... FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’: FCollada/FCDocument/FCDLibrary.cpp:149:30: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0); ^ FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’: FCollada/FCDocument/FCDLibrary.cpp:150:34: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’: FCollada/FCDocument/FCDLibrary.cpp:151:27: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’: FCollada/FCDocument/FCDLibrary.cpp:152:31: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’: FCollada/FCDocument/FCDLibrary.cpp:153:27: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’: FCollada/FCDocument/FCDLibrary.cpp:154:28: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’: FCollada/FCDocument/FCDLibrary.cpp:155:31: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’: FCollada/FCDocument/FCDLibrary.cpp:156:29: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’: FCollada/FCDocument/FCDLibrary.cpp:157:26: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’: FCollada/FCDocument/FCDLibrary.cpp:158:26: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’: FCollada/FCDocument/FCDLibrary.cpp:159:29: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’: FCollada/FCDocument/FCDLibrary.cpp:160:30: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’: FCollada/FCDocument/FCDLibrary.cpp:161:33: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’: FCollada/FCDocument/FCDLibrary.cpp:162:36: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsScene]’: FCollada/FCDocument/FCDLibrary.cpp:163:33: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] In file included from FCollada/FUtils/FUSemaphore.cpp:10:0: FCollada/FUtils/FUSemaphore.h:36:2: warning: #warning "FUSemaphore: Semaphore not implemented for non Windows" [-Wcpp] #warning "FUSemaphore: Semaphore not implemented for non Windows" ^ FCollada/FUtils/FUStringConversion.cpp: In function ‘void TrickLinkerFUStringConversion()’: FCollada/FUtils/FUStringConversion.cpp:281:8: warning: variable ‘f’ set but not used [-Wunused-but-set-variable] float f = FUStringConversion::ToFloat(&c); ^ FCollada/FUtils/FUStringConversion.cpp:283:7: warning: variable ‘b’ set but not used [-Wunused-but-set-variable] bool b = FUStringConversion::ToBoolean(c); ^ FCollada/FUtils/FUStringConversion.cpp:285:8: warning: variable ‘i32’ set but not used [-Wunused-but-set-variable] int32 i32 = FUStringConversion::ToInt32(&c); ^ FCollada/FUtils/FUStringConversion.cpp:287:9: warning: variable ‘u32’ set but not used [-Wunused-but-set-variable] uint32 u32 = FUStringConversion::ToUInt32(&c); ^ In file included from FCollada/FUtils/FUThread.cpp:10:0: FCollada/FUtils/FUThread.h:30:2: warning: #warning "Threads not yet implemented for non Windows." [-Wcpp] #warning "Threads not yet implemented for non Windows." ^ FCollada/FCDocument/FCDAnimationCurve.cpp: In member function ‘float FCDAnimationCurve::Evaluate(float) const’: FCollada/FCDocument/FCDAnimationCurve.cpp:396:13: warning: ‘inTangent.FMVector2::<anonymous>.FMVector2::<anonymous union>::x’ may be used uninitialized in this function [-Wmaybe-uninitialized] FMVector2 inTangent; ^ FCollada/FCDocument/FCDAnimationCurve.cpp:396:13: warning: ‘inTangent.FMVector2::<anonymous>.FMVector2::<anonymous union>::y’ may be used uninitialized in this function [-Wmaybe-uninitialized] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’: FCollada/FCDocument/FCDLibrary.cpp:149:30: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0); ^ FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’: FCollada/FCDocument/FCDLibrary.cpp:150:34: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’: FCollada/FCDocument/FCDLibrary.cpp:151:27: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’: FCollada/FCDocument/FCDLibrary.cpp:152:31: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’: FCollada/FCDocument/FCDLibrary.cpp:153:27: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’: FCollada/FCDocument/FCDLibrary.cpp:154:28: required from here FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable] FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’: FCollada/FCDocument/FC
http://jenkins-master:8080/job/phabricator/2153/ for more details.
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/gui/prelobby/prelobby.js | 159| » while·((message·=·Engine.LobbyGuiPollNewMessage())·!=·undefined) | | [NORMAL] JSHintBear: | | Use '!==' to compare with 'undefined'. Executing section XML GUI... | | [INFO] XMLBear: | | XML can be formatted better. |----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/prelobby/prelobby.xml | |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/prelobby/prelobby.xml | 1| 1| <?xml version="1.0" encoding="utf-8"?> | 2| |- | 3| 2| <objects> | 4| 3| | 5| 4| <script file="gui/common/functions_global_object.js"/>
http://jenkins-master:8080/job/phabricator_lint/621/ for more details.
Build is green
Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jenkins-master:8080/job/phabricator/2154/ for more details.
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/gui/prelobby/prelobby.js | 159| » while·((message·=·Engine.LobbyGuiPollNewMessage())·!=·undefined) | | [NORMAL] JSHintBear: | | Use '!==' to compare with 'undefined'. Executing section XML GUI... | | [INFO] XMLBear: | | XML can be formatted better. |----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/prelobby/prelobby.xml | |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/prelobby/prelobby.xml | 1| 1| <?xml version="1.0" encoding="utf-8"?> | 2| |- | 3| 2| <objects> | 4| 3| | 5| 4| <script file="gui/common/functions_global_object.js"/>
http://jenkins-master:8080/job/phabricator_lint/622/ for more details.
binaries/data/config/default.cfg | ||
---|---|---|
384 ↗ | (On Diff #3952) | Shouldn't that be true by default? |
binaries/data/config/default.cfg | ||
---|---|---|
384 ↗ | (On Diff #3952) | I agree, that it's good for usual players. But pedantic users can say: "Aha! You want to steal my password!". If it's ok, then no problem - I can change it. |
binaries/data/config/default.cfg | ||
---|---|---|
384 ↗ | (On Diff #3952) | We had one of these players that complained about the password being saved. So if you really want it to be false, then there should be a dialog upon registration that asks after the registration whether the user wants to store the encrypted password in the config IMO |
binaries/data/mods/public/gui/lobby/prelobby.xml | ||
64 ↗ | (On Diff #3269) | rP20000 changed a lot to Title Case. |
Build is green
Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK! Checking XML files...
http://jenkins-master:8080/job/phabricator/2167/ for more details.
Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/public/gui/prelobby/prelobby.js | 159| » while·((message·=·Engine.LobbyGuiPollNewMessage())·!=·undefined) | | [NORMAL] JSHintBear: | | Use '!==' to compare with 'undefined'. Executing section XML GUI... | | [INFO] XMLBear: | | XML can be formatted better. |----| | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/prelobby/prelobby.xml | |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/prelobby/prelobby.xml | 1| 1| <?xml version="1.0" encoding="utf-8"?> | 2| |- | 3| 2| <objects> | 4| 3| | 5| 4| <script file="gui/common/functions_global_object.js"/>
http://jenkins-master:8080/job/phabricator_lint/632/ for more details.
Tested, works so far, code looks correct.
We have a lobby tab in the options dialog, so it's cheap to add this as a setting there.
binaries/data/config/default.cfg | ||
---|---|---|
384 ↗ | (On Diff #3952) | "Whether to store the encrypted password in the user config"? |
binaries/data/mods/public/gui/prelobby/prelobby.js | ||
209 ↗ | (On Diff #3979) | (One might also think about keeping the password still in the config after login, just not writing it to the config again. But then we would have to ensure that whenever saving a new config, i.e. nope.) |
300 ↗ | (On Diff #3979) | after connecting |
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/61/display/redirect