improvement: added warning before unloading edited unfinished practice if it's the current screen

[mathmods]

Please check if the PR fulfills these requirements:

• The commit message follows our guidelines.
• For bug fixes and features:
  • You tested the changes.
  • You updated the docs or changelog.

Related issue (if applicable): #9

What kind of change does this PR introduce?
Adds a few checks in NavigationManager to see if it is about to unload resources. If it is, proceed to ask the screen holding the resource if it is safe to unload. It then waits for a response (confirm or deny).

A new class UIResourceView has been added which both UILesson and UIPractice extends, it contains 1 virtual function and a few interface functions towards NavigationManager.

• Might move the functions into UILesson and UIPractice and remove the base class if this is not ideal.

Does this PR introduce a breaking change?
No

New feature or change

What is the current behavior?
Immediately unload screen with active data.

What is the new behavior?
Warn students before they unload the screen with active data.

Other information
Currently does not check earlier screens for information loss. It does however contain the bare minimum to at least tell when it's supposed to do this kind of check. (Separate signals and storage of unload type)

Navigating forward will always work as this part hasn't been changed at all.

I have currently implemented a base class for UILesson and UIPractice called UIResourceView. This class holds helper functions for NavigatorManager interactions

Denial of navigation isn't particularly difficult as there seems to be nothing wrong with just ignoring calls, but I haven't checked the web side of things. Navigation using browser buttons might require additional actions. (There is a denied function that will be called if denied by user, it just doesn't do anything yet.)

There seems to be a non-breaking bug somewhere, haven't managed to replicate it.
Received error-message from NavigationManager of unsupported request. (Seems it tried to unload post-denial? Or twice?)

[NathanLovato]

is this PR ready to test and review? As you made it as a draft, I assumed the implementation was unfinished, but your post suggests we should review it now.

[mathmods]

@NathanLovato Sorry, I was originally testing if it worked. As far as I am aware, there seems to be a bug somewhere. I managed to receive an error mesage during my PoC tests. I haven't managed to replicate it however, so I think it's ready for review.

[NathanLovato]

It's working well so far. Just when clicking a breadcrumb, like a lesson name, the popup doesn't appear - it currently only appears when clicking the back button (or triggering it with Alt Left).

I haven't reviewed the code itself yet, I'm just testing the features. I'll review the code a bit later

[mathmods]

Breadcrumbs doesn't actually unload the page (just pushes a new screen over it). See #274
So the data isn't technically lost. It is however possible to give a warning of adding a new screen if this is a more natural response.

[NathanLovato]

I think it's fine as it is, and if people feel like they lost stuff when using breadcrumbs, then we can consider adding a warning. But if they don't lose anything, then the warning isn't necessary, it'd be more of a UX thing.

[mathmods]

From my perspective there are currently 3 things that needs some input:

• The new UIResourceView isn't exactly aptly named, it is more or less just a base class holding NavigationManager interface functions for UILesson and UIPractice. There are some ways to address this;
  1. Change the name to UINavigationInterface / UIInterfaceForNavigation / ScreenInterfaceForNavigation / something similar
  2. Remove it and move it's functions into UIPractice and UILesson
  3. Move functions that UILesson and UIPractice share into this class. Would also remove a few ignore unsafe access comments from UINavigator.
  4. Keep it as it is.
• It currently does not react to a page in history being unloaded with data (say a practice unloaded by a lesson going to outliner). This might just be an edge case UX thing however. The required functions for dealing with it exists, just hasn't been properly configured to actually do everything.
• How does the browser back button work? I'm imagining this current PR does not satisfy it completely. Should a new state be re-pushed if the user denies pop_state (this should be easy to implement), or does it not matter?

[NathanLovato]

It currently does not react to a page in history being unloaded with data (say a practice unloaded by a lesson going to outliner)

That's all good, edge case

How does the browser back button work?

You don't have to worry about this for this task. While we do push URLs to the browser's history stack, we can't block browser navigation from the app, as it's sandboxed in a WebGL context. This task is only for navigation from inside the Godot app.

Regarding refactoring the base class, I was reading the code earlier, I need to think about it a little more.

[mathmods]

This task is only for navigation from inside the Godot app.

In that case this change is required in NavigationManager.gd to make browser navigation ignore waiting for user input:
In func _on_js_popstate(_args: Array) -> void:
change navigate_back() to _navigate_back()

[mathmods]

Regarding refactoring the base class, I was reading the code earlier, I need to think about it a little more.

Ok. Just give me a heads up if you need me to do anything with the code.

[NathanLovato]

I found a regression with this PR: in the browser, browser back navigation doesn't work like before. If in the history stack you go through the outline, the screen will get stuck there even when the URL/history should go to a lesson, practice, or whatnot.

Regarding the code choices, what I see is that you designed the code to take into account UILesson or UIPractice, and so you made a base class for both, but this popup and back nav on confirmation is only for UIPractice. As a result, I think the code could be simpler: there's no need for a base class, and all we need to do is:

• In UINavigator's _navigate_back()
• When the user wants to go back from within the app
• If the current screen is a UIPractice, show a popup
• Yield until the user closes the popup
• If they confirmed the back navigation, proceed
• Otherwise, return early from the function

That way, no need for the extra base class or extra code in NavigationManager, I think. Please let me know if I missed something important

[mathmods]

If in the history stack you go through the outline, the screen will get stuck there even when the URL/history should go to a lesson, practice, or whatnot.

If I'm not wrong, this is how it currently works on staging.

[mathmods]

• In UINavigator's _navigate_back()
• When the user wants to go back from within the app
• If the current screen is a UIPractice, show a popup
• Yield until the user closes the popup
• If they confirmed the back navigation, proceed
• Otherwise, return early from the function

This was how I implemented this in #274, but @pycbouh mentioned it didn't catch any browser back calls, thus I reworked it to this.

[NathanLovato]

If I'm not wrong, this is how it currently works on staging.

Yes, indeed. I'll open a ticket for that.

This was how I implemented this in #274, but @pycbouh mentioned it didn't catch any browser back calls

I see. It produces a little bug: the browser's URL and navigation state can desync with the app at the moment. I don't know if that's a serious concern though - if you feel it's not, it's good to keep things that way.

Here's an example where I pressed back twice in the browser. I tried to navigate back to the outliner.

After confirming I want to leave the practice, I ended up on the lesson instead of the main/outline screen. The only thing is the URL doesn't match.

Now, it's not that big of a deal considering the improvement: warning the user about data loss. So if you think it's okay too, we can merge your PR.

[mathmods]

A similar bug exists on staging. Using the browser back button will make the history walk back twice in browser history, but only back once in the actual screen (thus desynced). Should be fixed by adding _push_javascript_state(get_current_url()) in _on_js_popstate. (Effectively telling the browser we'll handle back calls?) I can't test it on my machine sadly.

[mathmods]

After reading on the javascript popstate event I'm realizing that the browser forward button also "pops the state", and the code navigates back on any popstate event. Thus if you want to go forward, you will instead be sent back.
Should probably create an entire issue for this.

In other words, desyncs happen all the time if browser interaction is used. So I'm fine with this merge if you are @NathanLovato

[NathanLovato]

And merged! Thank you very much for your help, once more. We really appreciate it.