Improvement: config registration form settings

[math-GH]

Closes #981

Changes proposed in this pull request:

• improved UI for set the registration formular

Before:

After:
Value "1": No registration form, because minimum 1 account exists

Value "0": unlimited accounts

Value "x": set the number

by default it will give a number of amount of existing accounts + 1, so that the registration form will be enabled

If the set number is bigger than the number of existing account, than the text "form enabled" will be shown.
Else: "form disabled".
The text is calculated "live"

Pull request checklist:

• clear commit messages
• code manually tested

[math-GH]

published as "Draft" PR to collect your feedback.

Open todo:

• i18n translations. Only en-us is available right now
• phpcs

[Alkarex]

Looks good :-)

[math-GH]

Ready for review

[Alkarex]

Try to run the new make fix-all (not fully production ready yet, as it requires npm install and composer install to be run advance)

[Alkarex]

Hum, it looks there is some old code in there. Maybe an old branch?

[math-GH]

the failed test I will check later

[Alkarex]

It is not only the tests: it looks like some files are based on old versions, e.g. main.js

[Frenzie]

There's no need to use anonymous functions for this kind of usage.

Suggested change

	input.addEventListener('click', function() { onchange_maxNumbersOfAccounts(this); });
	input.addEventListener('click', onchange_maxNumbersOfAccounts);

And then something like:

// I think we use ev rather than e or event? Not 100% sure otoh.
function onchange_maxNumbersOfAccounts(ev) {
  const elem = ev.target;

[math-GH]

I use the function also for
onchange_maxNumbersOfAccounts(input);
any suggestion to fix it?

[Frenzie]

Did you mean to write onchange_selectInputChanger? The same way,

function onchange_selectInputChanger(ev) {
  const elem = ev.target

Another thing is the name btw, but I'll comment about that on another line.

[math-GH]

ev.target does not work for input while calling onchange_maxNumbersOfAccounts(input);

[Frenzie]

I think something like an init event might be a touch more elegant, but let's leave it like this then. Any performance/memory related to anonymous functions is completely irrelevant here.

	input.dispatchEvent(new Event('change', {
		bubbles: true,
		cancelable: true,
	}));

I would like the event listener and/or the function name to be less deceptive about what's going on, however. ;-) change is probably the right event to use here.

[math-GH]

thanks, it helped me a lot. That are the small little things I cannot google it, I need it told from an expert :)

[Frenzie]

The tl;dr is that if you add a lot of event listeners your page's memory usage can balloon, especially combined with anonymous functions. If you ever see a page that's just doing something like document.addEventListener('click', … and then checking if ev.target === some element, that's why. (Or perhaps not on the document, but in any case on the parent container div/form/whatever instead of the individual checkboxes.) While that's definitely not an issue here, one thing I've learned as a maintainer is that letting through something seemingly innocuous will suddenly be copied by someone else several years later as an example to follow, almost guaranteed. :-) Besides the memory usage, all the bubbling through all of the elements that have an event listener attached can also take much longer overall when you have a lot of them.

It's particularly when you're adding anonymous functions programmatically, like here in a for loop, that it can become problematic. For one-offs it probably doesn't make an appreciable difference, at least in memory usage, although it can affect performance in slightly unexpected ways (e.g., a regularly declared function might be JIT-compiled better than an anonymous one, at least in the past).

I'm not sure if what I steered this towards is necessarily the best way to approach this particular problem; you could also write something like, at its simplest:

const elem = ev.target || ev;

I just happen to find it more elegant to use the event listener itself instead of hacking the function to take either an event or an element. Another, probably clearer and better approach would be some indirection, i.e., a function that always takes an element (your original function) and a second non-anonymous function for the event listener that simply calls the first function with ev.target.

So much for tl;dr… but in any case, I hope that explains why adding anonymous functions in a for loop triggered my radar. I don't like pages that waste a lot of memory. ;-)

[Frenzie]

I would name this something more generic, otherwise it's very easy to get at best strange and at worst misleading function names like we see here. However, the function name is correct that you should probably be listening for change, not for click.

Suggested change

	function onchange_selectInputChanger(elem) {
	function updateSelectInput(elem) {

[math-GH]

@Alkarex : I do not understand why main.js makes such problems. I thought that I used the newest version from edge branch

[math-GH]

I updated main.js from edge manually. Now it works and passed the checks

[Alkarex]

Let's make a stable release 1.19 before we land this PR in the following 1.20 work (due in particular to new translation strings)

[math-GH]

Cannot find the root of this test issue

Run composer run-script translations && git diff --exit-code
> cli/manipulate.translation.php -a format
PHP Parse error:  syntax error, unexpected single-quoted string "option", expecting ")" in /home/runner/work/FreshRSS/FreshRSS/cli/i18n/I18nFile.php(66) : eval()'d code on line 174
Script cli/manipulate.translation.php -a format handling the translations event returned with error code 255
Error: Process completed with exit code 255.

@aledeg Could you please check?

[Alkarex]

Regression fixed in #4312