Skip to content

Conversation

@zorkow
Copy link
Member

@zorkow zorkow commented Nov 5, 2017

Upgrades to the latest version of SRE. Adapts the a11y menu as discussed:

  • removes generation options
  • replaces selection of walkers by a binary choice to switch walker on or off.
    Both generation and walker options can still be altered in the default configurations for the explorer and a therefore reachable.

NOTE: PR does not update the walker selection in the labs, though.

@zorkow zorkow assigned zorkow, pkra and dpvc and unassigned zorkow Nov 5, 2017
@zorkow zorkow changed the title Sre v2.1.0 Sre v2.1.1 Nov 5, 2017
Copy link
Member

@dpvc dpvc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code that is no longer accessible should be removed, but other than that, it is ready to go.

var speechOn = Assistive.getOption('speech');
var constructor = Explorer.Walkers[Assistive.getOption('walker')] ||
var constructor = Assistive.getOption('walker') ?
Explorer.Walkers[Assistive.defaults.walker] :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I understand, this will force anyone who has chosen a walker in the past to be moved to the new table walker?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table walker is the new default walker and it subsumes the syntax walker (which can still be set via configurations).
However, if I understand your question correctly, then you are concerned that anyone who has previously chosen syntax walker and has that in their cookie setting, might be "stuck" on the old walker, without realising that there is a new one.
Indeed, I had not considered that possibility.
Cookies are not overwritten with a new version of MathJax or an extension, right?
In that case maybe I should map syntactic -> TableWalker and come up with a new name for the old walker.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The table walker is the new default walker and it subsumes the syntax walker (which can still be set via configurations).

I'm not sure that the value in the configuration is used any longer. As I read line 598 above, the table walker is always used, since you no longer use the walker option to select the which walker is used. You have changed the menu to use a boolean rather than a string, and if the cookie value is truthy, you get the Assistive.defaults.walker (table), and none otherwise. So I don't see how you can get any of the previous walkers.

I'm wondering if you want to either make the menu checkbox return a string (table) rather than the default boolean, and use that as the index to Explorer.Walkers[] as before, or alternatively, check the value of Assistive.getOption('walker') and if it is a boolean and true, use Assistive.defaults.walker, otherwise use the value of Assistive.getOption('walker') if it is a string, or none as the default.

Cookies are not overwritten with a new version of MathJax or an extension, right?

Not automatically, but they will of the user changes the settings. It would also be possible to add code to modify existing cookie settings when the extension is loaded. Alternatively, the addDefaults() method could probably be made to handle that.

{action: Explorer.Regenerate}),
ITEM.RADIO(['lazy', 'Lazy'], 'Assistive-generation',
{action: Explorer.Regenerate})
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that these values are removed, do we need the switch statement at line 414 (in the new version of the file), and the AddSpeechEager() function? I also don't see when AddSpeechLazy() ever gets called.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still set the generation option in the configuration. I could be useful if a page author wants all speech pre-computed at load. But if this is a use case that we don't want to support, I can remove it and get rid of the generation option altogether.
To clarify the cookie issue here: I assume a saved option that no longer exists is not a problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still set the generation option in the configuration.

OK, good point. Then leaving the code is fine.

I assume a saved option that no longer exists is not a problem.

It depends on how the value is used. There is no error in storing the cookie but if the stored value is used in a way that cases an error in the code using it, then that would be a problem. For example, when I was testing these changes, I was using a local copy of the extensions, and forgot to update the mathjax_sre.js file, so the table walker was missing. This caused the Explorer.Walkers[Assistive.defaults.walker] above to return undefined, and that caused later code to fail. It took me a while to figure out what was happening. So bad cookie values can cause problems, under some circumstances. It looks like these ones for Assistive-generation should not be a problem, however.

Again, there could be code added to the extension that would check the cookie and modify any old values to appropriate new ones. That would not me hard.

@zorkow
Copy link
Member Author

zorkow commented Dec 13, 2017

Following f2f: The correct configuration parameters already exists. So we are now using the parameters in hub.config for walker and generation instead of the menu entries.

@dpvc
Copy link
Member

dpvc commented Dec 18, 2017

OK, I was worried that the setDefaults() function, by populating the menu settings with the default values, would cause those values to be saved in the menu cookie if some other menu option was changed by the user. It appears that isn't the case, so I think you are OK at this point.

In hindsight, I think that saving the defaults in the menu settings (SETTINGS[]) is probably the wrong approach, and that getOption() should check the menu settings, the configuration, and the defaults, in that order, and return the first one that is found, and then always use getOption() rather than using SETTINGS[] directly (as it is in a few places). But it is probably not worth it at this point, as all of this will be changing in v3. The menu (if there is one) is probably going to be an extension, not part of the core, and the API for hooking into it will have to be more robust.

In any case, you can merge this.

@zorkow zorkow merged commit f3a7afa into master Dec 20, 2017
@zorkow zorkow deleted the sre_v2.1.0 branch December 20, 2017 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants