SketchUp / htmldialog-inputbox

UI::HtmlDialog example recreating UI.inputbox functionality in the SketchUp Ruby API
MIT License
6 stars 5 forks source link

Dropdown Boxes Do Not Show Default Selection #8

Open matthes-b opened 3 years ago

matthes-b commented 3 years ago

The dropdown input controls generally work as intended. However, dropdown controls always start out showing an empty selection. The user has to manually operate the dropdown and make a selection to see the current value.

The expected behavior (to mimic the original UI.inputbox) would be that the first value of the input array should be displayed as the dialog opens.

The obvious solution would be to control this in the <option> tag by adding something along the lines of <option v-for="option in input.options" **v-bind:selected="$index === 0"**>.

However, it appears this is not possible in vue if v-model is being used, as vue will always override this depending on model data.

Vue documentation indicates the default selection needs to happen somewhere inside here:

    let app = new Vue({
      el: '#app',
      data: {
        **selected: (correct value here...)**
        options: {
          title: 'Untitled',
          inputs: [],
         },
      },

However up to now I have not been able to determine exactly how to set the first element of the input array in this data structure so it gets displayed when the dialog opens.

DanRathbun commented 3 years ago

The dropdown input controls generally work as intended. However, dropdown controls always start out showing an empty selection.

I am not seeing this problem on MS Windows, SketchUp 21.1.332, with the latest commit Thomas has made. (And I do not remember ever seeing an issue where the default was not set as the selected value for dropdowns or listboxes.)


When you report issues, please follow the template: ie: platform, SketchUp version, repository version*.

* We have to give you a pass on the repository version as Thomas has been making changes without changing the version in the "extension.json" file.

The SketchUp version is important as the Chromium framework is updated every few major releases. The SU 2021.1 release updated the UI::HtmlDialog class to Chromium v 88.0.4324.150.

DanRathbun commented 3 years ago

The expected behavior (to mimic the original UI.inputbox) would be that the first value of the input array should be displayed as the dialog opens.

I'm sorry, this is not correct. Using UI.inputbox, if the coder does not specify a default for a dropdown, then it will be rendered in an unchosen state. If the end user does not make a selection then and empty string is returned in the result array for such dropdown controls.

The HtmlUI inputbox will follow suit. If the coder does not specify a default or purposefully passes nil as the default, then VueJS will render the HTML control in an unselected state.

This is normal as it should be. The coder can test for the empty return and know then that the end user skipped that control.

Vue documentation indicates the default selection needs to happen somewhere inside here:

... meaning the data object of the Vue app object. And it normally does, the current value is set from data.options.inputs[index].value (aka all the Vue components have: v-model="input.value").

What is probably confusing in that Vue doc example you linked to, is that it is using a variable name "selected" which is also a HTML attribute identifier. This only appears to give it more meaning then intended. The name of that variable (actually an object property) could be anything, ... "desired", "wanted", "highlighted", ... whatever , it's only an name binding an HTML element value to a JS object property value.

matthes-b commented 3 years ago

Apologies, yes this is was on SU2020 [20.2.172] Chromium 64.0.3282.119 and Windows 10.

I have some machines on which I cannot upgrade on every release due to production reasons. But I shall compare with the latest SU and repository version.

I do note, however, that I am passing the exact same arguments I was sending to UI.inputbox to HtmlUI.inputbox (I have written myself a little toggle to flip from one to the other) and that means when I provide defaults to the old inputbox then I can start with the dropdown in a selected state (seeing the default value) and I am not getting the same result in HtmlUI.inputbox.

Will check again on newest SU.

DanRathbun commented 3 years ago

Apologies, yes this is was on SU2020 [20.2.172] Chromium 64.0.3282.119 and Windows 10.

Okay I have tested on SU2017, SU2018, SU2020 and SU2021 (all latest releases.) I do not have SU2019 installed as it's installer had problems so I uninstalled and manually cleaned my registry of it's error keys.

I do not have any issues with default values for dropdowns or listboxes or textboxes or checkboxes being properly set.

matthes-b commented 3 years ago

Quick update: I just found some use case where I do get the default selection in the dropdown, but I still have another case where I don't - so most likely it has something to do with the parameters that are being passed in. Bear with me while I track this down...

DanRathbun commented 3 years ago

Ok, make sure that you have the same html file as in the repo.

... most likely it has something to do with the parameters that are being passed in.

I think I only tested strings. It would be smart to test all data types.

matthes-b commented 3 years ago

Ok, I think I found the issue. It's more of a bug exploit in the old inputbox - and not a bug in the new one.

I am of course referring to the usage of UI.inputbox with four arguments which is the only way to create dropdowns in classic UI.inputbox.

I had a case in my code where the option strings being passed into inputbox as a pipe-separated string contained some trailing spaces as padding. Interestingly, old UI.inputbox was so forgiving it would still match a non-padded default value string to a padded option string and would then still start with the dropdown in selected state.

The new HtmlUI.inputbox is no longer as forgiving - which is fine. I cleared the trailing spaces and now starting the dropdowns in selected state with the desired default value works as before.

DanRathbun commented 3 years ago

Yes we definitely do not want to bring the various bugs in UI.inputbox over to this html dialog class.

Add to this that the bugs manifest differently on Mac then on Windows.

So annoying! It has been like this since I started with SketchUp 13 years ago, and no amount of complaining has been good enough to get it fixed.


Padding is/was an attempt to workaround the several bugs in the prompt rendering widths and the edit control widths.

(The main bug in UI.inputbox is that the edit control widths are being set to the prompt widths. And in some cases no amount of workarounds set the control widths wide enough to display verbose options properly. Ie, they go off to the right under the scrollbar.)

The spacing between prompt column and input column is also 0. (This causes coders to try to test their prompts for max length and add 2 spaces to the longest one.)

It would have been better had they just fixed the layout problems and right stripped whitespace from prompts, defaults and options. Then all the complaints would end.

I had a case in my code where the option strings being passed into inputbox as a pipe-separated string contained some trailing spaces as padding. Interestingly, old UI.inputbox was so forgiving it would still match a non-padded default value string to a padded option string and would then still start with the dropdown in selected state.

My memory is tickled by a vague idea that in one of the release cycles the workaround padding of option values was acknowledged and internally right stripped so that they could match defaults.

But the lack of proper documentation has resulted in a bug report (even though I think the options whitespace right-strip was intentional) ...

Issue 588: inputbox returns string with trailing spaces truncated, in which I said ...

When there is only 1 value the bug also prevents the picklist control from setting the default value, even though it is listed as the first choice.

I think that the scenario where the default would be whitespace padded was overlooked. Mr. Gerard showed a valid example where existing object name properties (ie, for definitions, materials, etc.) might be used for default values and may be right padded.

This causes another workaround where the default must be stripped, and then the return value (which is automatically stripped) must be compared against the options array using name = ary.find {|name| value == name.rstrip } (or a similar Regexp) so that the found name can be used subsequently to reference the resource as it's named in the collection.

This scenario is likely rare, but informing coders of whitespace stripping for options (and not for defaults) in the UI.inputbox documentation sure would be nice.


Now, all that said, ... the HtmlUI::Inputbox does not do any whitespace stripping in ArgumentParser#get_options_args().

This led me to find several design errors in the code.

One is that only when using the advanced hash argument is the default checked to be sure that it exists in the options list. (see HtmlUI::Dropdown#initialize and HtmlUI::Listbox#initialize.)

Another is that ArgumentParser#get_options_hash() is wiping out the defaults in HtmlUI::Inputbox when named arguments are omitted. (I'll open another issue for this.) DONE, see issue #11

matthes-b commented 3 years ago

Hah, glad to hear I am not the only one who resorted to string padding trying to control the width of the old UI.inputbox.

Of course, undocumented value stripping should not be brought across into the new inputbox.

What might be considered is to have a log/console warning along the lines of "Warning: trailing whitespace detected in options string. This might lead to unexpected behaviour in default selection". This could help avoid some confusion for other coders who will also likely be converting existing code by pointing it at the new HtmlUI.inputbox.

DanRathbun commented 3 years ago

I'm not sure about the warning. Most experienced rubyists think they are noise. But whenever I do puts warnings, I append if $VERBOSE as a condition on the end. (It at least allows control by the coder.)

This could help avoid some confusion for other coders who will also likely be converting existing code by pointing it at the new HtmlUI.inputbox.

You do understand that this is not a common library, but an example for a custom library for other coders to wrap in their own namespaces and modify to their own tastes ?

matthes-b commented 3 years ago

You do understand that this is not a common library, but an example for a custom library for other coders to wrap in their own namespaces and modify to their own tastes ?

Of course. Just thinking about everybody stepping into the same pitfall... This this github thread now being here might also be enough.

thomthom commented 3 years ago

I'll accept a PR that emits a warning if it's a trivial change. Otherwise my understanding is that we can close this?

DanRathbun commented 3 years ago

Otherwise my understanding is that we can close this?

It was due to whitespace padding in option values from code using the native UI::inputbox method. So it was pilot error. ;)

I'll accept a PR that emits a warning if it's a trivial change.

I'll go along with a warning ... as long as it is conditional upon $VERBOSE or Sketchup::debug_mode? (which is SU2016+,) so the coder can decide whether the warning shows or not.

This MAY only be needed for using the HtmlUI::inputbox wrapper. The advanced hash paradigm uses the HtmlUI::Dropdown and HtmlUI::Listbox classes whose constructors check that the default value is one of the given options. An ArgumentError exception is raised if not.


ie ... where a warning message PAD_WARNING constant has been defined at the top of ArgumentParser ...

  class ArgumentParser

    PAD_WARNING ||= 'Warning:(HtmlUI::inputbox) Trailing whitespace detected'<<
    " in options string.\n"<<'This may cause default value not to match and'<<
    " control will render without initial selection.\n"

And then replace the args.size > 2 block (beginning line 76) with ...

      if args.size > 2 && args[2].is_a?(Array)
        # NOTE: Empty string options map to empty arrays:
        arguments[:list] = args[2].map { |object| object.to_s.split('|') }
        title_index += 1
        # Conditionally warn if any option has trailing whitespace values:
        if $VERBOSE || Sketchup::debug_mode?
          # Check for trailing whitespace padded options:
          trailing_whitespace = /\S+\s+\z/i
          # (arguments[:list] is an array of options arrays)
          warn(PAD_WARNING) if arguments[:list].any? { |list|
            next false if list.empty?
            list.any? { |opt| opt =~ trailing_whitespace }
          }
        end
      end
matthes-b commented 3 years ago

A warning may help some people who wish to migrate from old to new inputbox. Otherwise this thread here will also help them. Ok to close.

DanRathbun commented 3 years ago

I can do the PR but will need to refresh my repository branching skills. Normally the Issues are closed when the pull request is merged.

A warning may help some people who wish to migrate from old to new inputbox.

A Migration wiki would be benefit here, rather than relying upon users perusing closed issues.