search

rioj7 / command-variable

Visual Studio Code extension for variable substitution via ${command:commandID}
54 stars 10 forks source link

Pre-populate selection of "multiPick" lists when the storage is empty #96

Closed l-inc closed 1 month ago

l-inc commented 1 month ago

There seems to be an inconsistency between how the selection lists with "multiPick": "true" and with "multiPick": "false" allow to select the default returned when just pressing "Enter":

These kinds of defaults are different to the existing attribute default, which is returned by the command (without saving to the storage) when "Esc" is pressed.

Is there a way to preset the check boxes in the multiPick lists?

rioj7 commented 1 month ago

@l-inc Can you tell a bit more about the addLabelToTop problem? Do you use it with multiPick? If addLabelToTop is defined and there is no label that has a value equal to the resolved addLabelToTop nothing should happen.

l-inc commented 1 month ago

@rioj7, the problem with addLabelToTop is that it does exactly what it states, which is to put the list entry with the specified label at the top.

The effective benefit of this behaviour for the single-pick lists is that the user can press "Enter" and accept the entry at the top as a default.

This benefit is missing for the multi-pick lists because addLabelToTop does not automatically checkmark the entry at the top. And even if it did, there would be no possibility to checkmark multiple entries. My understanding is that this effectively renders addLabelToTop useless for multi-pick lists.

Besides, referencing the entries by label is currently problematic with multi-pick lists anyway if the labels and values differ. That's because of the inconsistency between the multi-pick and single-pick lists when handling value that consists of key-value pairs. For the single-pick lists, the keys and values in the key-value pairs become storage keys and values respectively, whereas multi-pick lists store the key-value pair as a single object, which cannot be used for the purpose of retrieving the labels.

rioj7 commented 1 month ago

@l-inc you can try v1.65.6 and add the picked property.

This issue was about the: preset the check boxes in the multiPick lists

Re-reading the text it looks like you have other questions/issues: addLabelToTop behavior, .....

The inconsistency between how the selection lists with "multiPick": "true" and with "multiPick": "false" allow to select the default returned when just pressing "Enter" is due to how VSC handles Enter for showQuickPick, the extension has no control over that. Maybe it can when implementing a custom pick list but that is a major piece of work.

addLabelToTop also kicks in when nothing is found in the storage for the given key

What behavior do you see with single pick lists and addLabelToTop when the key is not found in the storage?

addLabelToTop was not initially intended to be used for multi pick lists.
For multi pick lists a valid option is: nothing checked.
So always checking the item added to the top is not the right thing to do.

I have created a new issue for the handling of key-value list option in a multi pick list.

If I have missed another issue please create a separate issue.

l-inc commented 1 month ago

@rioj7

you can try v1.65.6 and add the picked property.

Thank you. It works. However, considering that the storage used to store the state of the multi-pick lists and the storage used to store the key-value pairs retrievable via remember differ, the benefit of this kind of configuration is lower than it could be. That's because these 2 storage locations easily get desynchronized in 2 ways:

  1. If no persistent storage is defined in commandvariable.remember.persistent.file, then restarting VS Code may result in the older state of the multi-pick list whereas remember will fallback to some (desired) defaults. In this case, I'd like to pre-populate the list with these defaults. I've noticed the new undocumented property clearStorage, but to be used in sync with the remember store, it shouldn't always be set to true, which is problematic to implement at the moment.
  2. If the remember store is updated independently via a different task/command. In this case, I'd like to retrieve the values from the remember store.

The inconsistency between how the selection lists with "multiPick": "true" and with "multiPick": "false" allow to select the default returned when just pressing "Enter" is due to how VSC handles Enter for showQuickPick, the extension has no control over that. Maybe it can when implementing a custom pick list but that is a major piece of work.

I may have not explained my point well. I didn't mean the visual inconsistency. Bringing an option to the top is a valid means to default-select it by just pressing "Enter". The functional benefit of addLabelToTop is this default selection by pressing "Enter", whereas bringing the option to the top is an innocuous visual side-effect. The inconsistency I tried to emphasize is that multi-pick lists did not have a feature with an equivalent functional benefit. The property picked alleviates this somewhat, but taking the desynchronization issues from above into account, it is not equivalent to addLabelToTop.

addLabelToTop also kicks in when nothing is found in the storage for the given key

What behavior do you see with single pick lists and addLabelToTop when the key is not found in the storage?

The behavior is as expected: the list is unchanged. The point I was trying to make with this sentence is that addLabelToTop had an additional benefit of allowing to specify something meaningful as a default even in cases when nothing was found in the remember store. This was to show the difference to multi-pick lists that did not provide such a possibility.

So always checking the item added to the top is not the right thing to do.

I agree. I didn't mean to suggest otherwise.

I have created a new issue for the handling of key-value list option in a multi pick list.

Thank you. I brought this issue up because I thought that the design for pre-populating the multi-pick lists could be affected by the way the problem with key-value object storage is resolved. Now, I'm not sure how to use picked to pre-populate the multi-pick lists with something stored in the remember store.

l-inc commented 1 month ago

P.S.

addLabelToTop was not initially intended to be used for multi pick lists.

I'd say that this is a design problem with this property. Its name puts an emphasis on the visual side-effect rather than on the actual functional feature it brings. A possible alternative to this would be a property called defaultSelect (or alike) that takes a sequence of 0 or more labels. Then it could work consistently for both single-pick and multi-pick lists. It would bring the specified option to the top for single-pick lists and it would select 0 or more check-boxes for multi-pick lists.