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

Added a settings window. #662

Merged
merged 15 commits into from
Nov 23, 2023
Merged

Added a settings window. #662

merged 15 commits into from
Nov 23, 2023

Conversation

lilSander
Copy link
Collaborator

Added a settings window, in which the user can:

  1. See the keybindings and its description.
  2. Return back to the game.
  3. Exit the game.

@lilSander lilSander linked an issue Nov 9, 2023 that may be closed by this pull request
Cheat Sheet for the Keybindings #659
Closed
@koschke koschke added the enhancement New feature or request label Nov 9, 2023
@koschke koschke added this to the Qt World Summit milestone Nov 9, 2023
@koschke koschke requested a review from falko17 November 9, 2023 19:25
falko17
falko17 requested changes Nov 9, 2023
View reviewed changes
Copy link
Collaborator

@falko17 falko17 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 your PR, this is a very useful feature. Before we can merge this, please rewrite the SettingsMenu to be a proper UI component within SEE's UI framework (see review comments below for details).

Assets/SEE/Controls/KeyBindings.cs Outdated Show resolved Hide resolved
Assets/SEE/Controls/KeyBindings.cs Outdated Show resolved Hide resolved
Assets/SEE/Controls/SettingsMenue.cs Outdated Show resolved Hide resolved
Assets/SEE/Controls/SettingsMenue.cs Outdated Show resolved Hide resolved
Assets/SEE/Controls/SettingsMenue.cs Outdated Show resolved Hide resolved
falko17
falko17 requested changes Nov 11, 2023
View reviewed changes
Copy link
Collaborator

@falko17 falko17 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 changes, this is now better integrated into our existing UI framework. I have a few additional minor comments, but apart from that this should be ready to merge.

Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/Controls/KeyBindings.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Show resolved Hide resolved
@koschke
Copy link
Collaborator

koschke commented Nov 15, 2023

Please note that you have a conflict with Assets/Resources/Prefabs/Players/DesktopPlayer.prefab. Please check what has been changed in the master branch versus your branch. The textual conflict resolution by Git may not work well for Prefabs files.

@koschke
Copy link
Collaborator

koschke commented Nov 15, 2023

Please note that you have a conflict with Assets/Resources/Prefabs/Players/DesktopPlayer.prefab. Please check what has been changed in the master branch versus your branch. The textual conflict resolution by Git may not work well for Prefabs files.

I will take care of that.

@axivionsee
Merge remote-tracking branch 'origin/master' into 659-cheat-sheet-for…
83b1ffa 
…-the-keybindings-1

# Conflicts:
#	Assets/Resources/Prefabs/Players/DesktopPlayer.prefab
@axivionsee
#659 Re-added component SettingsMenu after merging the master into th…
940cfa7 
…is branch.
koschke
koschke requested changes Nov 15, 2023
View reviewed changes
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot reviewed Nov 15, 2023
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

There are a few bad patterns I found which you should check.

Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
Assets/SEE/UI/SettingsMenu.cs Outdated Show resolved Hide resolved
koschke and others added 3 commits November 15, 2023 10:32
@koschke @github-actions
Applied github action suggestion.
10fe478 
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@koschke @github-actions
Applied github action suggestion.
1b4cebb 
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@koschke @github-actions
Applied github action suggestion.
5f2a5a2 
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@koschke
Copy link
Collaborator

koschke commented Nov 15, 2023 •
edited
Loading

This is a very useful feature. Yet, I think that the list of keybindings is not easy to read. Could you add some markup/structure and also widen the window to make the list more readable?

I added a new issue #673 for that.

koschke
koschke approved these changes Nov 16, 2023
View reviewed changes
Copy link
Collaborator

@koschke koschke left a comment

Choose a reason for hiding this comment

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

All fine now. Can be merged.

@koschke
Copy link
Collaborator

koschke commented Nov 16, 2023

@falko17 I think all your change requests have been resolved. If you are happy with these, you can approve this PR. Or have I overlooked anything?

falko17
falko17 approved these changes Nov 16, 2023
View reviewed changes
Copy link
Collaborator

@falko17 falko17 left a comment

Choose a reason for hiding this comment

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

Yes, everything seems good now.

@falko17
Copy link
Collaborator

falko17 commented Nov 16, 2023

@koschke Apparently, the Axivion Dashboard at stvr2 is down, hence tests are failing.

@koschke
Copy link
Collaborator

koschke commented Nov 17, 2023

@koschke Apparently, the Axivion Dashboard at stvr2 is down, hence tests are failing.

Thanks for letting me know @falko17 I restarted the Axivion service again.

@koschke koschke merged commit f0776d1 into master Nov 23, 2023
1 of 8 checks passed
@koschke koschke deleted the 659-cheat-sheet-for-the-keybindings-1 branch November 23, 2023 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@falko17 falko17 falko17 approved these changes

@koschke koschke koschke approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
Qt World Summit
Development

Successfully merging this pull request may close these issues.

Cheat Sheet for the Keybindings
4 participants
@lilSander @koschke @falko17 @axivionsee