Added ability to navigate with browser forward/backward buttons

[DarkMatterMatt]

Added ability to navigate with browser forward/backward buttons as per #48

Moved item/module loading from the mod (data set) loader. Now loadData only gets mod data and stores it appropriately. loadModules and loadItems have been added.

Functions that change the hash now have an optional parameter called keepHash (which stops the function changing the hash)

Added loadFullSettings function which doesn't leave default settings blank

[DarkMatterMatt]

I think it's all good, I'll do more testing later. It would be nice if clickTab accepted a settings object instead of a tabName.

Also, in

function clickVisualize(event, tabName) {
    clickTab(event, tabName)
    renderGraph(globalTotals, spec.ignore)
}

Should the event be there? It doesn't do anything as far as I can tell.

[DarkMatterMatt]

... it doesn't work, gets stuck between two states because:

Given a scenario where state1 is the current state, state2 is the last state.

back button is pressed hashchange is triggered which will load state2 correctly and then update the hash (making the state2 the latest and state1 the last state)

clicking back again will repeat, - hashchange is triggered which will load state1 correctly and then update the hash (making the state1 the latest and state2 the last state)

keepHash stopped this by preventing the hash being updated and stopping the states being reordered

[KirkMcDonald]

clickTab is, first and foremost, an event handler. It is called when a tab is clicked on. It would make no sense for it to receive a settings object. On the other hand, it would make sense for it to consistently receive strings that end in "_tab" or not. The latter (which are the strings stored in the settings object) may end up being simpler.

It annoys me that clickVisualize works in that state, actually. It should not have an 'event' parameter.

As for the hashchange event handler, the goal should be suppressing any modifications to the hash while handling hashchange events. Instead of using the global flag to control whether the event handler does anything, use it to control whether any assignments to window.location.hash happen at all. This implies defining a new function which performs those assignments, gated by the flag, and replacing any existing assignments with that function call.

The hope is that, if you click "back" and get a new URL fragment identifier from the browser history, you can use that to put the calculator into such a state that it would generate that fragment anyway, rendering the assignment of a new hash moot.

[DarkMatterMatt]

I think these commits have addressed your comment.

[KirkMcDonald]

Okay, there's one more detail: Not only do we need to avoid changing the hash as part of processing the hashchange event, but we need to avoid triggering the hashchange event handler when changing the hash.

As your change currently is, the code will compute any change to the solution twice: Once in response to the event handler responsible for handling whatever prompted the new solution, and once again when that causes the hash to change.

While you could use the navigationInProgress flag to gate both cases, it would probably be clearer to use two separate flags for this.

[DarkMatterMatt]

Two questions: 1) Is there a better way to do this than to set/unset the flag in every event handler that calls itemUpdate or display? (this is why I prefer a single function that handles multiple actions (instead of one function per action), your way looks neater though).

2) How did you find this out - did you just read it and realize or do you count the number of times itemUpdate is called per action or what?

[KirkMcDonald]

To answer the second question first, I had a suspicion that it would do this, and I verified that suspicion using Chrome's performance analyzer. A breakpoint in the JS debugger would have revealed this as well.

The solution is trickier, but should only involve changes to the hashchange handler and updateHash(). I suspect that merely setting and unsetting the flag immediately before and after the assignment to window.location.hash won't actually address the problem: Event handlers are executed in sequence, meaning that this approach would unset the flag before the hashchange handler is even executed.

Some experimentation may be required, but my guess is the logic may need to be along the lines of setting the flag when you perform the assignment, and unsetting it in the hashchange event handler, just before you return from it early.

[DarkMatterMatt]

You got it bang on, first I tried setting and unsetting in the hash update function, that didn't fix it. Setting it in the hash update function then unsetting it in the hashchange handler works well :)

[KirkMcDonald]

I only meant to remove that one specific check. The navigationInProgress flag is still required.

[DarkMatterMatt]

Are you sure it's required? there are two ways the hash can be changed: 1) through updateHash, the hashchange handler will not run because the plannedHashUpdate flag will be set 2) through browser forward/back navigation, the hashchange handler will run because the plannedHashUpdate flag will not be set

[KirkMcDonald]

And, as we discussed before, when the hashchange event does run, it needs to not result in the hash being assigned to when it ends up calling updateHash(). You have to gate both cases:

1. updateHash() needs to avoid triggering the hashchange event handler.
2. The hashchange event handler needs to avoid triggering a reassignment of the hash.

plannedHashUpdate gates the first case. navigationInProgress gated the second.

[DarkMatterMatt]

At the moment the hash is assigned to when the event does run, but it does not trigger the event to run a second time (because of plannedHashUpdate), nor does it add an additional state in the browser history because the hash is not changing. Adding the navigationInProgress gate would avoid the hash being assigned to unnecessarily but it wouldn't change the calculator's behaviour or performance in any way.

EDIT: I'm testing it and I think I'm wrong again

[KirkMcDonald]

If assigning an identical string to window.location.hash doesn't trigger the event handler, then you have the separate problem of assigning plannedHashUpdate = true when no hash update is in fact planned, which will cause it to ignore the next legitimate hashchange event.

EDIT: Which, to be clear, the navigationInProgress flag avoids, by preventing plannedHashUpdate from being assigned to true.

[DarkMatterMatt]

Apologies for the radio silence.

Haha no worries, I figure I'm probably being a nuisance wanting to push through changes that you aren't quite ready to make 😛

Don't add the hashmark here. It messes with the "zip" feature.

Fixed/changed

This function does not properly reset things to their defaults from a given settings object. It will need some rewriting, but I'd be okay applying that fix after accepting the rest of this PR.

I have no idea how the modules/beacons sections of your code works, so I'm not going to try fix/muddle around with it.