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.

Sign up for GitHub

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

Jump to bottom

Add GUI screen to name a recording #3548

Merged
merged 7 commits into from Nov 18, 2018
Merged

Add GUI screen to name a recording #3548

merged 7 commits into from Nov 18, 2018

Conversation

AndyTechGuy
Copy link
Contributor

@AndyTechGuy AndyTechGuy commented Nov 16, 2018 •
edited

Contains

Adds the screen nameRecordingScreen after the selection of a game to record, prompting the player to make a unique name for their new recording. The screen prevents naming a recording with the same name as an existing recording, resolving #3465. If the player enters an invalid file name (blank, entirely invalid characters, existing recording), the player is prompted to re-enter a name.

The structure of RecordScreen has been changed, now primary file operations and loading for the recording are now handled in NameRecordingScreen.

How to test

  • Create a new recording, and set the name to whatever you want!
  • Erronous recording names should be handled as follows:
    • If the text field is left blank, the file name should be reported invalid.
    • If the recording name is made of entirely invalid characters, the file name should be reported invalid.
    • If the recording name already exists, the file name should be reported as a duplicate.
    • If the recording name has some invalid characters, the manifest will have the full name while the directory name will have these characters removed.

Translation

  • Four new translation strings have been added for nameRecordingScreen
  • I have added an English and French translation for all four strings.
  • I'm not a native French speaker, so if somebody can confirm/fix my French translation that would be greatly appreciated! (it has been a long time since french class.....)

@iaronaraujo

@AndyTechGuy
Add a helper function for renaming recordings
2bfcc3b 
Adds a helper function to RecordScreen that renames the recording to a provided title. This commit does not integrate this function into the UI.
@AndyTechGuy
Add popup to RecordScreen to rename conflicting recordings
44544d6 
Adds a NUI popup to RecordScreen to implement the helper function to rename a recording. Checks for if a recording with the same name already exists, and if so, will prompt the user to enter a different name for the recording.
@AndyTechGuy
Make recording naming mandatory, and turn rename prompt into a Screen.
a6f2247 
This commit changes the behavior of recording renaming, from requesting only when there is a name conflict, to requesting upon every new recording. Because the rename prompt occurs on every recording, the popup has been changed to a separate screen altogether. Many functions from RecordScreen have been transplanted to accomodate this change.
@AndyTechGuy
Add TranslationSystem statements to NameRecordingScreen, and slightly…
898bbe9 
… change isNameValid

Adds translation strings to the GUI for the rename screen. Includes an (attempted) French translation. Also adds a step to the name validity check, checking for names made of invalid characters.
@AndyTechGuy
Documentation, checkstyle, and other minor changes for clarity.
e29399f
@AndyTechGuy
Delete ShortEntryPopup.java
8cb34d5
@GooeyHub
Copy link
Member

Can one of the admins please verify this patch?

@llvieira llvieira added GCI Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience labels Nov 17, 2018
@syntaxi
Copy link

syntaxi commented Nov 17, 2018

@GooeyHub: ok to test

Will review shortly

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

iaronaraujo
iaronaraujo suggested changes Nov 17, 2018
View reviewed changes
Copy link
Contributor

@iaronaraujo iaronaraujo left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, these changes look pretty solid! Besides the changes I requested directly on the code, I noticed a small bug: If you try to give an already existing name to a new recording, the message "A recording with this name already exists! Enter a different name." appears, and if I click on "cancel" and try to name another recording, this message is still there instead of "Enter a unique name for this recording."

engine/src/main/java/org/terasology/rendering/nui/layers/mainMenu/RecordScreen.java Outdated Show resolved Hide resolved
engine/src/main/java/org/terasology/rendering/nui/layers/mainMenu/NameRecordingScreen.java Outdated Show resolved Hide resolved
llvieira
llvieira suggested changes Nov 17, 2018
View reviewed changes
Copy link
Member

@llvieira llvieira left a comment

Choose a reason for hiding this comment

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

Good work, @AndyTechGuy! Minor tweaks are required.

engine/src/main/java/org/terasology/rendering/nui/layers/mainMenu/NameRecordingScreen.java Outdated Show resolved Hide resolved
@AndyTechGuy
Implement reviews from llvieira and iaronaraujo
d63e273 
Fixes issues brought up in PR reviews, including:
- Screen error messages now reset when player opens the screen
- File path comparison now performed properly
- Renames screen launching function in RecordScreen
- Removes unused 'title' field in NameRecordingScreen
@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@AndyTechGuy
Copy link
Contributor Author

The issues brought up in both reviews have (hopefully) been addressed, this should be ready for review again!

@iaronaraujo
Copy link
Contributor

The last commit solved the problems I mentioned!

@llvieira llvieira added this to the v2.2.0 milestone Nov 18, 2018
@llvieira llvieira merged commit 20485dd into MovingBlocks:develop Nov 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iaronaraujo iaronaraujo iaronaraujo approved these changes

@llvieira llvieira llvieira approved these changes

@syntaxi syntaxi Awaiting requested review from syntaxi

Assignees
No one assigned
Labels
Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience
Projects
None yet
Milestone
v2.2.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@AndyTechGuy @GooeyHub @syntaxi @iaronaraujo @llvieira