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

replace() doesn't add the replaced screen onto the stack #63

Closed
Jerakin opened this issue May 23, 2020 · 11 comments · Fixed by #64
Closed

replace() doesn't add the replaced screen onto the stack #63

Jerakin opened this issue May 23, 2020 · 11 comments · Fixed by #64
Labels
bug more info More information is needed before any action can be taken

Comments

@Jerakin
Copy link
Contributor

Jerakin commented May 23, 2020

Calling replace doesn't seem to add the replaced screen on the stack

monarch.show("game")  -- "Normal screen"
monarch.show("menu")   -- "Popup screen"
print(monarch.dump_stack())
monarch.replace("score")   -- "Popup screen"
print(monarch.dump_stack())

The print output is this

DEBUG:SCRIPT: 1 = hash: [game]
2 = hash: [menu]
DEBUG:SCRIPT: 1 = hash: [game]

When I call monarch.replace twice in a row the second time the screen doesn't show. By explicitly saying pop=0 I can get the item to show. Though I still do not get the monarch.dump_stack() I expect.

@Jerakin
Copy link
Contributor Author

Jerakin commented May 27, 2020

My Project.zip

Trying to use replace() to "cycle" screens.

Expected outcome:
Screen A is loaded [Stack: A]
Popup B is loaded [Stack: A, B]
Popup C replacing B [Stack: A, C]
Popup D replacing C [Stack: A, D]

Actual outcome:
Screen A is loaded [Stack: A]
Popup B is loaded [Stack: A, B]
Popup C replacing B [Stack: C]
UNLOADS C BUT NEVER LOADS D [Stack:]

@britzl
Copy link
Owner

britzl commented May 28, 2020

Ok, so first of all there was a a problem in the code where an error wasn't caught and handled properly. This has been fixed. BUT the new replace functionality is a bit tricky:

When show() is called the first thing that happens is that any open popups are called, unless the new screen is also a popup and the "popup on popup" option is enabled on the current popup. The replace functionality comes into play first AFTER the popups are closed. This results in:

  • Stack contains A,B
  • Calling monarch.replace("C") (ie pop = 1)
    • B is a popup -> close it!
    • Stack contains A
    • Now replace/pop comes into play and removes A from the stack (but doesn't unload or hide it)
    • C is shown
  • Stack contains C

So the question is what the expected behavior is when popups exist? I mean, say you have a screen and then two popups in the stack: [S1, P1, P2]. What happens if you replace/pop with a normal screen? You can't expect to have [S1, P1, S2]. The only logical thing is for popups to first close and then replace which in the example would mean [S2] in the stack.

But it also means that using replace() with popups is a bit weird:

[S1, P1] - replace(P2) -> [P2]?

Replace is the normal behavior when showing one popup on top of another (unless popup on popup is allowed).

Thoughts on this @dapetcu21 ?

@dapetcu21
Copy link
Contributor

Yes, I agree. In the case where you're replacing with another screen, we should definitely do the replace after all the popups are closed.

As you pointed out, when you're replacing with a popup with popup on popup enabled, the situation is a bit tricky. We could either leave it as is, or I propose we do the following:

Treat the popup and screen sections of the stack separately.

If the screen that is to be shown/replaced is a popup, then do all the pops off the stack (if they're more than the number of popups on the stack, then stop), THEN do the top.popup and screen.popup_on_popup check and pop all the popups if necessary, THEN push the new screen on the stack.

If the screen that is to be shown/replaced is a screen, then pop all the popups, then do all the pops, then push the new screen.

@dapetcu21
Copy link
Contributor

Some examples of that behaviour:
[S1, P1] - replace(P2) -> [S1, P2]
[S1, P1, P2] - replace(P3, popup_on_popup) -> [S1, P1, P3]
[S1, P1, P2] - replace(P3) -> [S1, P3]
[S1] - replace(P1) -> [S1, P2] // There are no popups here, so replace() acts like show()
[S1, P1, P2] - replace(S2) -> [S2]

@britzl
Copy link
Owner

britzl commented May 28, 2020

Yes, this is probably the way forward to properly handle all cases.

@dapetcu21
Copy link
Contributor

I'll do a PR, then

@britzl
Copy link
Owner

britzl commented May 28, 2020

If you have the time that would be great!

@Jerakin
Copy link
Contributor Author

Jerakin commented May 29, 2020

Can confirm that replace now works as I presumed it would! 👍

@Jerakin
Copy link
Contributor Author

Jerakin commented Sep 6, 2020

I noticed today that if I do .replace from within the script that gets replaces the new screen does not get onto the stack (but it still is loaded/shown).

Example: [S1, P1] - replace(P2) -> [S1, P2]
So if I in P2 do replace(P2)

The stack will become
[S1] instead of the expected [S1, P2]

@britzl britzl reopened this Sep 12, 2020
@britzl
Copy link
Owner

britzl commented Sep 12, 2020

I'm trying to reproduce this in the example that ships with Monarch by modifying the two popups "about" and "confirm":

[menu] -> menu.gui_script calls show(about) -> [menu, about] -> about.gui_script calls replace(confirm) -> [menu, confirm] -> confirm.gui_script calls replace(confirm) -> [menu, confirm]

If I understood your bug report it would mean that when the Confirm popup is on the top of the stack and when confirm.gui_script calls replace("confirm") it would get removed but not added back.

Could you please create a small example?

@britzl britzl added bug more info More information is needed before any action can be taken labels Sep 12, 2020
@britzl
Copy link
Owner

britzl commented Sep 19, 2021

Not able to reproduce and no sample project provided. Closing.

@britzl britzl closed this as completed Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug more info More information is needed before any action can be taken
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants