Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multicheckbox without extra string input #1361

Merged
merged 4 commits into from Mar 12, 2016
Merged

Conversation

tormec
Copy link
Contributor

@tormec tormec commented Sep 21, 2015

I think it would be "nice" to have a multicheckbox without the additional string input.

The widget multicheckbox could accept a parameter, say _nostring with value on, which disable the extra string, otherwise (default) it is created with it.

So the syntax would be:

// multicheckbox without the additional string input
$meta['multi'] = array('multicheckbox', '_nostring' => 'on', '_choices' => array('a','b','c','d'));

// multicheckbox with the additional string input (default)
$meta['multi'] = array('multicheckbox', '_choices' => array('a','b','c','d'));

@Chris--S
Copy link
Collaborator

Is the commit missing something, what is supposed to happen when nostring is on?

@Chris--S
Copy link
Collaborator

I misread the code before, but I do think your change is an improvement. I like the idea/reasoning behind the change.

Iirc, part of the reason for having an "other" value is to handle unknown configuration values. Can I suggest modifying this slightly.

_other - how to handle values not specified in _choices.  accepted values: 
  'never' (non-recognised choices are not permitted, no "other" string input is provided, any other value found will be lost in updating), 
  'preferred' (only display "other" string input if a non-recognised choice is found), 
  'always' (always display "other" string input, the default value).  
  used by 'multicheckbox', ignored by others. 

I'm open to better language for the parameter and its possible values.

What do you/others think?

@tormec
Copy link
Contributor Author

tormec commented Sep 21, 2015

Let me better understand.

  1. Your '_other' => 'never' is equivalent to mine '_nostring' => 'on'?
  2. '_other' => 'preferred' means, for example:
$meta['multi'] = array('multicheckbox', '_nostring' => 'on', '_choices' => array('a','b','c','d', 'other, specify));

If so, I like it!

@Chris--S
Copy link
Collaborator

  1. Yes. So to prevent the input[type=text] control appearing, you would put
$meta['multi'] = array('multicheckbox', '_other' => 'never', '_choices' => array('a','b','c');

If somehow (e.g. via manual config file editing) the actual value includes a different value (e.g. 'foo') then this value is NEVER shown and if the config file is saved it is discarded. A parameter value of 'never' should only be used when the any other choice is invalid/ignored.
2. '_other' => 'preferred' means, if the actual value of this setting contains only the listed choices (e.g. 'a,b'), no input[type=text] is shown. If the actual setting includes another value (e.g. 'a,foo'), then the other input control is shown containing the extra value(s). A parameter value of 'preferred' should be used when non-listed values are valid, but the listed choices are PREFERRED.
3. '_other' => 'always' as now. The other input control is always shown.

@tormec
Copy link
Contributor Author

tormec commented Sep 21, 2015

I beg your pardon, but I've to be honestly: point 2 is outside my ability because if the idea was to use javascript, to check if is selected a particular value, I don't know how to implement it (I don't know javascript).
Feel free to close this pull request and open another with your suggestions.

@Chris--S
Copy link
Collaborator

2 isn't in JavaScript. [pseudo code] create other input if always or (preferred and !empty($other))

@tormec
Copy link
Contributor Author

tormec commented Sep 22, 2015

Sorry if I'm so pedant but there is still somthing escapes me.

Consider the syntax:

$meta[multi'] = array('multicheckbox', '_other' => 'preferred', '_choices' => array('a','b','c','d','foo'));

and for default none of the choices is already checked, that is:

$conf['multi'] = '';

I think there are two different approach to implement the point 2.
A)
The user ticks a and foo. Then he goes at the bottom of the page and clicks "save".
Only now you can control what was checked and beacuse of was ticked foo you show the extra input string.
If the user unselects foo the extra input string will be removed only after the page will be saved again.

B)
(This method requires javascript)
The user ticks a and foo. It isn't necessary to save the page. As soon as foo is ticked the extra input string, which already exists but for default is disabled, changes its state and becomes "active" allowing user to write in.
If the user unselects foo the extra input string back "disable" but won't removed even after the page is saved.

  • I suppose you want realize method A), do you?
  • If so, honestly, I prefer method B) because A) has and odd behaviour. Unfortunately I don't know javascript and this is way I didn't propose it.
  • If neither A) nor B) is what you mean, I think it's better to close this PR and open another (and, again, sorry for my inefficiency).

Therefore, in both A) and B), how are you going to check if foo was selected or not?
Suppose we try to implement method A), you might want to modify the previous syntax in something like this:

$meta[multi'] = array('multicheckbox', '_other' => 'preferred', '_preference' => 'foo', '_choices' => array('a','b','c','d','foo'));

and then use:

// convert from comma separated list into array + combine complimentary actions
$value = $this->_str2array($value);
$default = $this->_str2array($this->_default);

// create a copy of the array with the checked value
$val = (new ArrayObject($value))->getArrayCopy();

// ...

if ($this->_other == 'always' or ($this->_other == 'preferred' and in_array($this->_preference, $val))) {
// show the extra input string
}

Have I mixed up the issue?

@Chris--S
Copy link
Collaborator

Don't (ever) worry about seeking clarification.

'foo' wouldn't be in _choices. I will add more later when I'm back at my computer.

Permit three values:
- always (default), the other input field is always displayed
- exists, the other input field is only displayed when the setting
          contains value(s) not listed in choices*.
- never, the other input field is never displayed. If the setting
         contains any value(s) not listed in choices they will be
         discarded on saving.
* This means, under normal circumstances the admin will not see
  the other input field.  It will only appear after manual editing
  to enter a non-listed _choice or if an update were to remove a
  previously selected _choice
@Chris--S
Copy link
Collaborator

I created a PR against your master for you to review, do you see what I mean now?

What do you think?
[ If you agree, you can merge the PR which (I think) should update this PR :-) ]

@tormec
Copy link
Contributor Author

tormec commented Sep 22, 2015

Finally!! Now it's all clear.
The real problem was to reproduce such behaviour.

Let me sum up and explain, step by step, (also for other readers) when exists could come useful.

  1. Initially a plugin uses the syntax:
$meta[multi'] = array('multicheckbox', '_choices' => array('a','b','c','d'));

this means the extra string is shown. Let's suppose the user inserts an extra value not in _choices [1].
2. In the next release the author's plugin decides to use the multicheckbox without the extra string, but, in order to be backwardly compatible, adopts:

$meta[multi'] = array('multicheckbox', '_other' => 'exists', '_choices' => array('a','b','c','d'));

which means:

  • if the user has inserted extra values, they will be printed in the extra string;
  • otherwise the extra string won't show.

Thanks for your patience!

[1]
if the user inserts in the extra string a value which already exists in _choices but:

  • is already ticked, then the extra string will be removed and nothing else;
  • is not already ticked, then the extra string will be removed and the relative value will be ticked.

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@Chris--S
Copy link
Collaborator

👍

splitbrain added a commit that referenced this pull request Mar 12, 2016
multicheckbox without extra string input
@splitbrain splitbrain merged commit ce0758a into dokuwiki:master Mar 12, 2016
@splitbrain
Copy link
Collaborator

Thanks @tormec could you please update the documentation at https://www.dokuwiki.org/devel:configuration#configuration_metadata ?

@tormec
Copy link
Contributor Author

tormec commented Mar 12, 2016

Sure. I will do it as soon as possible.

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.

None yet

5 participants