britzl / monarch

Monarch is a Defold screen manager with transition support and a straight forward API
MIT License
163 stars 23 forks source link

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

Closed Jerakin closed 3 years ago

Jerakin commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

dapetcu21 commented 4 years ago

I'll do a PR, then

britzl commented 4 years ago

If you have the time that would be great!

Jerakin commented 4 years ago

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

Jerakin commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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