further refinements to navigation interception

[bcolloran]

along the lines of what I described in #2403 (comment) , i think there is more room to refine how and when we intercept navigation.

I think the desired behavior is that we should always make sure a user really intends to navigate away if doing so would cause them to lose work, either edits or novel computed state.

• if you arrive at the page in Report view and never enter explore view, don't alert on nav away (since no novel state can be lost)
• if you make any edits and then eval and then attempt to navigate away, alert since novel state may be lost

are there other rules or cases we want to consider? @wlach @Algentile what do you guys think?

Moreover, i think we probably don't need to specifically handle delete/backspace keys since the nav actions they trigger are a subset of all nav actions, and we always want to confirm a users intent during a nav action, no matter how it is triggered. In this case, we can probably factor out the key handling code.

[Algentile]

@bcolloran I think the one remaining edge case to consider is if we enter report mode after a change has been made alert on nav away as novel state still might be lost if edits are made, eval is run, then user enter's report mode.

I think one user brought up that their desired intention when using back button was to go from report mode to explore mode as a secondary option, this would require us to push the state from before the user enters report mode.

[bcolloran]

enter report mode after a change has been made alert on nav away as novel state still might be lost if edits are made, eval is run, then user enter's report mode.

100% agree -- i was thinking of "make edits, eval code, enter report view" a subset of "make edits, eval code" -- whether the user enters report view or stays in explore view, they might lose evaluation state. and i think that one code path can cover both cases. do you think they need to be handled as separate cases?

in fact, i think that "make edits then eval code" is the set of steps needed to mark a session as having "novel state", and that we should double check about nav away if and only if a session has novel state.

I think one user brought up that their desired intention when using back button was to go from report mode to explore mode as a secondary option, this would require us to push the state from before the user enters report mode.

this sounds good to me! i guess we could push these states, and then pop them when a user pushes the back button until they run out of state and the back button attempts to navigate away, at which point we could confirm their action if the session has novel eval state, or just let them nav away if there is no novel eval state.

and nav away via other mechanisms (url bar entry, reload, etc) would be unaffected by the back/forward stack, and would just check the novel-eval-state flag to see if we need to confirm the action

[wlach]

The current behaviour is a bit frustrating -- while it's harder to lose your work than it used to be (good), I'm finding I'm getting a lot of warnings about losing work with notebooks I haven't ever modified when interacting with iodide (bad).

This plan sounds totally reasonable to me, it should be easy enough to hook into the redux actions that modify iomd and run code in order to track this "novel state" condition.

[Algentile]

Hey @wlach I will follow up with a PR that should close these issues. In the interim should we revert the change with the previous PR since it is providing warnings on notebooks that were not modified by the current working user?

[wlach]

I think it's ok to leave things as is for now, the current behaviour is a little annoying, but arguably still better than what we had before since it's now harder to lose data

…

On Mon, Dec 23, 2019, 18:43 Anthony Gentile ***@***.***> wrote: Hey @wlach <https://github.com/wlach> I will follow up with a PR that should close these issues. In the interim should we revert the change with the previous PR since it is providing warnings on notebooks that were not modified by the current working user? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2537?email_source=notifications&email_token=AAAFAWLK7FTCEKNFWHGZSZ3Q2FELDA5CNFSM4JZCZ6OKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHSEF7Y#issuecomment-568607487>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAFAWIBUH4ANQFWMWNUVBLQ2FELDANCNFSM4JZCZ6OA> .

[wlach]

This drives me absolutely crazy whenever I have to work on iodide, so I decided to just sit down and spend a couple hours fixing it.