Added ability to navigate with browser forward/backward buttons #64

DarkMatterMatt · 2017-11-10T21:49:52Z

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

Added ability to navigate with browser forward/backward buttons 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 which doesn't leave default settings blank

KirkMcDonald · 2017-11-11T05:01:37Z

events.js

+        mouseOnPage = false
+    });
+});
+window.addEventListener("hashchange", function() {


On consideration, "hashchange" is probably the wrong event. This business with "keepHash" and mouseOnPage is fiddly, and it fails when using keyboard shortcuts rather than clicking on the buttons.

The "popstate" event seems like a better fit. It should be less error-prone and should eliminate a lot of the fiddly bits of this code.

I'm also inclined to think that this event handler should only be installed after the dataset is loaded for the first time, though in practice you'd have to be trying pretty hard to make a problem out of this.

popstate is also called when updating the hash, so it would act almost exactly the same as hashchange. How do you think popstate would eliminate the keepHash? (I think keepHash is ugly - I don't like it either).

I'm also inclined to think that this event handler should only be installed after the dataset is loaded for the first time, though in practice you'd have to be trying pretty hard to make a problem out of this.

I don't think it would be possible to make a problem out of this unless you automated something specifically to break it - and even then it would only affect a local javascript that would be fixed on reload.

EDIT: We could bind a click listener to body then disable the hashchange/popstate checks for 100ms or so

Bluh, for some reason I thought popstate wouldn't do that, but okay. hashchange it is.

A workaround, then, would be to define a function that removes the event handler, changes the hash, and then re-adds the event handler. Then this function could be used in the handful of places where it assigns to window.location.hash. This would ensure that the only things that can trigger the event aren't other events.

Instead of removing the handler there could be a check in the handler for a global flag that is set when the handler is triggered and removed when the handler is finished.

i.e.

var navigationInProgress = false window.addEventListener("hashChange", function() { if (navigationInProgress) { return } navigationInProgress = true // do stuff navigationInProgress = false }

This means that extra hashChange events are ignored

This is probably cleaner, sure.

KirkMcDonald · 2017-11-11T05:13:36Z

fragment.js

    return settings
 }
+
+function loadFullSettings(fragment) {


Is this necessary? A "complete" settings map isn't needed when loading the page initially. It should not be necessary when re-reading the fragment.

The alternative is to check for each default and update it when moving back/forward. Not having either would mean the tab won't change when browsing (for example) from tab=settings_tab to tab=totals_tab (which would have no tab in the settings object).

Basically this function is just something that could be easily implemented as a one-off, but might be used in the future by other aspects of the code (I can't think of an example off the top of my head).

Personally I would have a global settings object and update it, then update the UI off the object (instead of referencing the UI when getting/changing settings), but I'll just do what you say because it's your code 😁

EDIT: another alternative is to window.location.reload() but ugh

Yeah, settings are all over the place. Some are global variables, some live in their UI elements, and some live on the FactorySpec. Refactoring them into an object dedicated for the purpose is on my to-do list, but I don't think this will be necessary here.

There's an invariant that is inconsistently applied across the settings. Enforcing it universally would make this simpler:

When rendering a setting, if its value is missing from the 'settings' object, it should be set to its default value.

Some of the render functions do this explicitly, while others do not, instead relying on some sort of global initialization to set the default. The lesson here is that global initialization is insufficient: renderSettings() might get called multiple times, and a settings object should wholly define the settings for the calculator, with the understanding that missing values should be equivalent to using the default values.

This should hold for all of the values in the fragment, actually, and not just those involved under renderSettings(). This is not a problem for "items" or "modules", which do this already, but as you point out, it is a problem for "tab". A solution there may be to have clickTab do e.g.

if (!tabName) { tabName = DEFAULT_TAB }

Glancing through settings.js, I see that these settings do not appear to enforce this invariant:

Display rate

Rate and count precisions

Furnace

Mining productivity bonus

Display format

The following do appear to enforce this invariant:

Dataset

Minimum assembler

Preferred belt

Minimum pipe length

Default module

Default beacon

If the settings in the first list can be corrected, this should remove the need to track the defaults explicitly.

Cool, do you mind if I do that? Should it be included in this PR or get a separate one (I'm inclined to think include it because it's relevant)?

If I'm doing it then it should be done in 24h, maybe 48h if I get busy.

EDIT: oh, and whats the default furnace - it's like the last one loaded in the mod list or something strange... the DEFAULT_FURNACE is fine for rendering after the mod is loaded, but it seems a bit weird

I was going to tweak this tonight. Hopefully it's as easy as I think it will be.

The default furnace is the "largest" furnace, as defined by FactoryDef.less(). Factories are ordered by the tuple of (crafting speed, module slots). In vanilla, this selects the electric furnace.

These criteria may change as part of adding Bob's support (depending on which furnace I want to use by default in Bob's), but I think the important thing is that the default is selected as part of initializing the FactorySpec (which parses through all of the factory entities). This should happen before the settings are rendered, so the default should be available by that point.

EDIT: And done in 7f704cc.

Now I've got to learn how to merge changes 😛 Usually it's just me working on something

changed hashchange to not trigger if it is already being handled moved `clickTab` in `init` to inside `loadDataRunner` callback to make it click the tab after the furnace has been set (otherwise `spec` is undefined when `clickTab` tries to `formatSettings`)

DarkMatterMatt · 2017-11-11T19:53:34Z

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 · 2017-11-12T02:42:09Z

... 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 · 2017-11-13T01:33:10Z

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.

`formatSettings` hash been renamed `updateHash` and directly modifies `window.location.hash`. `updateHash` does not run when the back/forward browser navigation button have been clicked.

Changed for consistency

DarkMatterMatt · 2017-11-13T23:26:19Z

I think these commits have addressed your comment.

KirkMcDonald · 2017-11-13T23:56:15Z

init.js

-        currentTab = settings.tab + "_tab"
-    }
    loadData(currentMod(), settings)
    // We don't need to call clickVisualize here, as we will properly render


Move this comment as well.

KirkMcDonald · 2017-11-14T02:41:58Z

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 · 2017-11-14T07:31:50Z

Two questions:

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).
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 · 2017-11-14T07:40:22Z

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.

Added a flag if it is a `userTriggeredHashUpdate` which makes the `hashchange` handler do nothing. The flag is reset on each hashchange

DarkMatterMatt · 2017-11-14T09:01:49Z

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 · 2017-11-14T09:38:10Z

events.js

+var navigationInProgress = false
+document.addEventListener("DOMContentLoaded", function() {
+    window.addEventListener("hashchange", function() {
+        if (navigationInProgress) {


This check should not be necessary.

KirkMcDonald · 2017-11-14T20:38:32Z

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

DarkMatterMatt · 2017-11-14T20:54:06Z

Are you sure it's required? there are two ways the hash can be changed:

through updateHash, the hashchange handler will not run because the plannedHashUpdate flag will be set
through browser forward/back navigation, the hashchange handler will run because the plannedHashUpdate flag will not be set

I didn't test it before committing (previously)...

KirkMcDonald · 2017-11-14T21:01:00Z

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:

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

plannedHashUpdate gates the first case. navigationInProgress gated the second.

DarkMatterMatt · 2017-11-14T21:09:44Z

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

Also changed `clickTab` to only click the tab if it is different to the current tab

KirkMcDonald · 2017-11-14T21:40:27Z

fragment.js

    }
    var zip = "zip=" + window.btoa(pako.deflateRaw(settings, {to: "string"}))
    if (zip.length < settings.length) {
        return zip


Don't forget to handle this case. This line should be:

settings = zip

KirkMcDonald · 2017-11-14T21:43:26Z

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.

KirkMcDonald

Apologies for the radio silence. I wanted to take some time to put this through some more thorough testing, and honestly just kept procrastinating on that.

KirkMcDonald · 2017-11-26T01:09:11Z

fragment.js

+        return
+    }
+
+    var settings = "#"


Don't add the hashmark here. It messes with the "zip" feature. Add it when assigning to window.location.hash.

KirkMcDonald · 2017-11-26T01:12:46Z

init.js

+    }
+}
+
+function loadModules(settings) {


On further review, I was wrong earlier: 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.

This does mean that the change, as written, won't quite handle modules correctly.

Changed updateSettings to add the hash last, just before assignment to window.location.hash. This fixes unwanted behaviour with the zip'ed hash.

DarkMatterMatt · 2017-11-26T01:40:41Z

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.

KirkMcDonald requested changes Nov 11, 2017

View reviewed changes

DarkMatterMatt added 2 commits November 12, 2017 08:18

Merge branch 'master' into browserNavigation

7224baa

DarkMatterMatt added 2 commits November 14, 2017 12:02

Changed formatSettings to updatehash

639a3d3

`formatSettings` hash been renamed `updateHash` and directly modifies `window.location.hash`. `updateHash` does not run when the back/forward browser navigation button have been clicked.

All tab names in JavaScript do not end in _tab

9f9455c

Changed for consistency

KirkMcDonald reviewed Nov 13, 2017

View reviewed changes

Removed comment

48e1358

Stopped double processing for each action

daa5ceb

Added a flag if it is a `userTriggeredHashUpdate` which makes the `hashchange` handler do nothing. The flag is reset on each hashchange

KirkMcDonald reviewed Nov 14, 2017

View reviewed changes

Removed unnecessary navigationInProgress flag

0f0b330

Fixed bug referencing undefined variable

32edcb0

I didn't test it before committing (previously)...

Reinstated navigationInProgress

2885741

Also changed `clickTab` to only click the tab if it is different to the current tab

KirkMcDonald reviewed Nov 14, 2017

View reviewed changes

plannedHashUpdate flag is only set if the hash actually will be updated

7713712

KirkMcDonald requested changes Nov 26, 2017

View reviewed changes

Changed updateSettings

da93afb

Changed updateSettings to add the hash last, just before assignment to window.location.hash. This fixes unwanted behaviour with the zip'ed hash.

Added ability to navigate with browser forward/backward buttons #64

Are you sure you want to change the base?

Added ability to navigate with browser forward/backward buttons #64

Conversation

DarkMatterMatt commented Nov 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkMatterMatt Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkMatterMatt Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkMatterMatt Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkMatterMatt Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KirkMcDonald Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkMatterMatt commented Nov 11, 2017

Uh oh!

DarkMatterMatt commented Nov 12, 2017

Uh oh!

KirkMcDonald commented Nov 13, 2017

Uh oh!

DarkMatterMatt commented Nov 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KirkMcDonald commented Nov 14, 2017

Uh oh!

DarkMatterMatt commented Nov 14, 2017

Uh oh!

KirkMcDonald commented Nov 14, 2017

Uh oh!

DarkMatterMatt commented Nov 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KirkMcDonald commented Nov 14, 2017

Uh oh!

DarkMatterMatt commented Nov 14, 2017

Uh oh!

KirkMcDonald commented Nov 14, 2017

Uh oh!

DarkMatterMatt commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KirkMcDonald commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KirkMcDonald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkMatterMatt commented Nov 26, 2017

DarkMatterMatt Nov 11, 2017 •

edited

Loading

DarkMatterMatt Nov 11, 2017 •

edited

Loading

DarkMatterMatt Nov 11, 2017 •

edited

Loading

DarkMatterMatt Nov 11, 2017 •

edited

Loading

KirkMcDonald Nov 11, 2017 •

edited

Loading

DarkMatterMatt commented Nov 14, 2017 •

edited

Loading

KirkMcDonald commented Nov 14, 2017 •

edited

Loading