Firstly, thanks for picking up this module and giving it some love, it really needed it.

We've maintained a set of patches for some time to implement various settings:

window.rsConf = {
  ui: {
    disableDetachment: true,
    tools: { translation: false },
  },
};

The post mode setting, the only one available through the UI, is only one of a number of other settings that users may want to adjust. One of the issues we've had with with post_mode.js is that it overwrites the settings object completely making it hard to add extra settings:

window.rsConf = {general: {usePost: true}};

What are you thoughts on how to allow users to customise other settings? I see two obvious options:

1: Add UI options to implement more of the setttings - that said, there are some that are seemingly not documented anywhere: https://wrdev.readspeaker.com/adaptation/configurations

2: Allow the user to enter a JS object (like the one above) into a textarea so it's open season - that will make it harder for the average user to add custom settings however, unless you took the common ones (like post mode) and made them checkboxes and then you could use a custom JS object to extend that.

I'd also like to see the introduction of a region selector, the default region for URLs seems to be Europe (eu) but we use Oceancia (oc) - would be great it this was an additional option - it could be added into the URL for the remote script as another token (as per the recent addition of the account ID), and also into the template file.

I'm happy to provide patches for the above if you have a preferred approcah.

Cheers

Issue fork open_readspeaker-3156251

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Feng-Shui created an issue. See original summary.

sunlix’s picture

sunlix
Primary language English
Location Wesel
commented

@Feng-Shui

thank you for your request. You are right, currently the rsConf will be overrided by the use_post library if enabled.
That crossed my mind during some refactoring work in the last weeks. But I did not record any changes on that due to lack of use cases for the further settings.

And yeah mostly the "general" settings are very undocumented. Even the post: true setting is undocumented.

So based on your suggestions I would go with topic 1, because I don't like any sort of "textfield" to enter JSON or another developer oriented config markup. It is not easy to understand on the first sight and need a deep knowledge of the possible configurations to put in there.
I think it would be easier for site builders and administrators to use a well formed configuration form. :)

Can you explain where are the different region (oc or eu) are applied in your cases?
We have two URL's that point to the ReadSpeaker services, the JS asset URL on the Open ReadSpeaker config page and in the template for the ReadSpeaker button.

In my case the JS asset URL is: //cdn1.readspeaker.com/script/[account_id]/webReader/webReader.js?pids=wr
No region specific anymore. Maybe you can try out if your asset library can be requested the same way?
In case the button the default is: //app-eu.readspeaker.com/cgi-bin/rsent
That could be change due to template override in your theme. Or would you prefer here any token replacement?

feng-shui’s picture

feng-shui commented

Will check out the cdn link for sure, didn't know that existed thanks for that.

If you're happy with approach 1, then I'm happy to put up a patch that adds a couple more settings to the admin form if you're ok with that?

sunlix’s picture

sunlix
Primary language English
Location Wesel
commented

@Feng-Shui

Yes I would like to aim for approach 1. :)
Sure, take the ticket, I will review the incoming patch(es) and support you as well as I can.

Thank you.

feng-shui’s picture

feng-shui commented

So I've had a good look through the documentation and I think I've found everything that's configurable.

Couple of questions/comments though.

1: Settings forms

Settings are split between the module's admin form and block's configuration form. If you can assume there would only ever be one ReadSpeaker block on the site, I think all config should be moved into the admin form so it's all in the one spot. If more than one block should be supported, then I think it makes more sense to move more configuration options to the block.

For example, settings like language (although I'm yet to confirm that query param actually makes a difference, still need to look into that) would seem to make more sense being configurable at the block level if more than one block was supported. While settings like "post mode" and "account id" could be "global" settings regardless.

2: Add token as a dependency

Given that token is required to get a functioning URL by default (as the default URL contains "[open-readspeaker:account-id]"), I think it should be added as a dependency.

3: Default URL

The default config that's enabled on installation uses the old Readspeaker.js file, suggest this should be updated to be WebReader.js

4: Settings schema

The settings schema was designed to be flat, with keys like: "open_readspeaker_post_mode". The rsConf object has quite a lot of depth and I think it makes sense to model the settings schema around it. So "open_readspeaker_post_mode" would become

open_readspeaker:
  general:
    usePost: true/false

I think this is more important when you have configuration like this:

open_readspeaker:
  ui:
    tools:
      translation: true/false

Out of interest, this is what the entire object looks like so far (based on the documentation I can find):

window.rsConf = {
  // https://wrdev.readspeaker.com/configuration-api
  general: {
    confirmPolicy: '',
    cookieLifetime: '',
    cookieName: '',
    defaultSpeedValue: '',
    domain: '',
    popupCloseTime: '',
    subdomain: '',
    syncContainer: '',
    saveLangVoice: '',
    usePost: '',
  },
  // https://wrdev.readspeaker.com/configuration-api
  settings: {
    hl: '',
    hlicon: '',
    hlscroll: '',
    hlsent: '',
    hlspeed: '',
    hltext: '',
    hltoggle: '',
    hlword: '',
  },
  ui: {
    mobileVertPos: '',
    // https://wrdev.readspeaker.com/adaptation/configurations
    tools: {
      settings: '',
      voicesettings: '',
      readhover: '',
      enlarge: '',
      textmode: '',
      pagemask: '',
      download: '',
      help: '',
      dictionary: '',
      translation: '',
    },
  },
  phrases: {},
  cb: {},
};

My thoughts here is that if the config schema can be made to match the rsConf object, then it can be converted to from the Drupal configuration object to the rsConf object with minimal processing.

--------------------

All of these things would result in a pretty major change to the module, especially the config schema changes. The more I look at the module's code, the more I think that a new major version would be the place to tackle these issues. The module languished for a long time, and I sense a lot of legacy code here.

Would you be happy to consider at a re-write of the module for an 8.x-2.x version? I'd be happy to provide an initial commit.

sunlix’s picture

sunlix
Primary language English
Location Wesel
commented

@Feng-Shui,

thank you very much for your great elaboration.

Here are my thought on that Topic.

2: Add token as a dependency
Not needed, tokens are supported by Drupal core since 8.0.
The Token contrib module provides only a handy dialog to get the tokens by click.
It is good for explorations, which tokens could be used in current context.
So the contrib module can stay as suggested.

3: Default URL
Sure, we can update the default URL.
My informations are, that the Enterprise Highlight product (ReadSpeaker.js) in not sold anymore, but still supported.
But I am fine with redeclaring the default.

4: Settings schema
yeah, well researched. :)
Currently we only have some small configs there. I think we can change the schema and re-map the old configs to the new more hierarchcally schema.

I will open a 2.0 Issue and add a issue-fork so we can elaborate together on this efford.
I would be very happy to get your support here. :)

sunlix’s picture

sunlix
Primary language English
Location Wesel
commented
Status: Active » Postponed

Postponed by #3225300: Plan for Open ReadSpeaker 2.0 release
All the topics discussed here will be implemented in 2.0.

sunlix’s picture

sunlix
Primary language English
Location Wesel
commented
Version: 8.x-1.x-dev » 2.x-dev

Okay I will commit here my first approach to solve the possible ReadSpeaker configuration at all.
If anyone has some motivation to support this it would be kick ass. Any help is really appreciated.

sunlix’s picture

sunlix
Primary language English
Location Wesel
commented
Status: Postponed » Active
sunlix’s picture

sunlix
Primary language English
Location Wesel
commented

Opinion to the cb (javascript callbacks) config:

I think there is no usefull case to support that from config perspective.
This are raw javascript callbacks that should be handled in the javascript world itself. (library)

You can define a new library like this to attach your custom javascript to the open_readspeaker/conf library.

# my-custom-module-or-theme.libraries.yml
open_readspeaker_callbacks:
  version: VERSION
  js:
    js/cb.js: { }
  dependencies:
    - open_readspeaker/conf

// cb.js
(function (rsConf) {
  'use strict';
  const callbacks = { cb: { ui: { open: function() { console.log('Open Callback!') }}}}
  window.rsConf = { ...rsConf, ...callbacks};
})(rsConf);

I think a paragraph in the README.md should be enough to inform the Drupal-Developers / Site-Builders.

sunlix opened merge request !26

sunlix’s picture

sunlix
Primary language English
Location Wesel
commented

I think this is ready for now.

I left a TODO for the phrases config-map.
Currently I have no use case for this so I tend to spin off a follow-up to track the use case from the community.

sunlix’s picture

sunlix
Primary language English
Location Wesel
commented
Status: Active » Needs review
sunlix’s picture

sunlix
Primary language English
Location Wesel
commented

  • sunlix committed 098ed433 on 2.x 
    Issue #3156251 by sunlix, Feng-Shui: Support for more settings
sunlix’s picture

sunlix
Primary language English
Location Wesel
commented
Status: Needs review » Fixed

Merged.
I will update the update-hook later on, if all config changes are in place, so we get one update-hook for the full config migration.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.