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.

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

Notification popups #2027

Merged
merged 14 commits into from
Jul 14, 2024
Merged

Notification popups #2027

merged 14 commits into from
Jul 14, 2024

Conversation

byorgey
Copy link
Member

@byorgey byorgey commented Jul 13, 2024

Closes #916.

Adds animated popup boxes at the top of the screen to alert the user when they (1) attain a new achievement, (2) unlock new recipes, or (3) unlock new commands.

I've tried hard to make sure that the UI always gets redrawn when there is a popup box that needs to be animated, but it was a bit fiddly and it's possible I've missed a case. Note that we used to use continueWithoutRedraw extensively while displaying menus, but in case a notification popup occurs we don't want that. Rather than plumb around a boolean so we can decide whether to continueWithoutRedraw or not, I decided to just get rid of them. This means we now redraw the menu every frame but that really shouldn't be an issue.

Open to suggestions for making the popups look nicer or including more information.

To test, try e.g.

  • Opening the About menu (make sure to delete ~/.local/share/swarm/achievement/LookedAtAboutScreen first)
  • Completing a tutorial (delete ~/.local/share/swarm/achievement/CompletedSingleTutorial first)
  • Start a Classic game and get a tree (should notify of new recipes)
  • make "branch"; make "branch predictor" (should notify of new commands)

@byorgey byorgey changed the title Notification system Notification popups Jul 13, 2024
@byorgey byorgey marked this pull request as ready for review July 13, 2024 13:09
@byorgey byorgey mentioned this pull request Jul 13, 2024
22 tasks
@noahyor
Copy link
Member

noahyor commented Jul 13, 2024

I was playing around with this system (it's great, by the way) and I think I found a bug.

To reproduce:

  1. Delete the Single Tutorial achievement
  2. Win the first tutorial

The goal modal appears.

However, the "Congratulations" Menu should be shown.

Doing the above steps without deleting the achievement produces the desired effect.

@byorgey
Copy link
Member Author

byorgey commented Jul 13, 2024

OK, my current theory is that in a sense the game has always "wanted" to display the goals dialog right after displaying the win dialog... but the UI was not updated while the game was implicitly paused by the win dialog, so the goals dialog did not get displayed unless you chose "keep playing". However, when there's a popup to display we must keep updating the UI so it will be properly animated. This also causes the goals dialog to immediately be shown after the win dialog.

In a sense this is sort of a bug which has existed for a long time, but it just happened to work because of the way we did not update the UI while displaying certain modals.

I'll have to think about the best way to resolve this.

@byorgey
Copy link
Member Author

byorgey commented Jul 14, 2024

The fix was very easy in the end --- when there was any popup to animate I was forcing the entire game to run rather than just forcing the UI to be redrawn! In other words when there was a popup, it was ignoring the "pause" setting. The solution is simple --- if the game is paused, only use the "force redraw" boolean to decide whether to call continueWithoutRedraw or not, not whether to call runFrameUI.

@byorgey
Copy link
Member Author

byorgey commented Jul 14, 2024

I think this is ready now! Of course, still very open to suggestions for improvement.

Copy link
Collaborator

@nitinprakash96 nitinprakash96 left a comment

Choose a reason for hiding this comment

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

Works like a charm!

Copy link
Member

@xsebek xsebek left a comment

Choose a reason for hiding this comment

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

This is awesome! 🤩

It's a great improvement on a highlighted shortcut in UI. 😆

src/swarm-tui/Swarm/TUI/Controller/UpdateUI.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Model/Popup.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Model/Popup.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/View.hs Outdated Show resolved Hide resolved
src/swarm-tui/Swarm/TUI/Controller.hs Show resolved Hide resolved
Copy link
Member

@noahyor noahyor left a comment

Choose a reason for hiding this comment

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

This is amazing! I would love for this to be in 0.6!

src/swarm-tui/Swarm/TUI/Model/Popup.hs Outdated Show resolved Hide resolved
@byorgey byorgey added merge me Trigger the merge process of the Pull request. CHANGELOG Once merged, this PR should be highlighted in the changelog for the next release. labels Jul 14, 2024
@mergify mergify bot merged commit 02c7933 into main Jul 14, 2024
12 checks passed
@mergify mergify bot deleted the notifications branch July 14, 2024 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CHANGELOG Once merged, this PR should be highlighted in the changelog for the next release. merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A toplevel notification system (for achievements, etc.)
4 participants