Fix updateSelectizeInput(), fixes #2624 #2625
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alandipert
force-pushed
the
fix-selectize-update
branch
from
8205b86
to
d4727e0
Compare
alandipert
changed the title
alandipert
force-pushed
the
fix-selectize-update
branch
from
d4727e0
to
cfb13fd
Compare
wch
reviewed
Sep 19, 2019
|
This PR should be targeted toward the rc-v1.40 branch instead of master. |
alandipert
force-pushed
the
fix-selectize-update
branch
2 times, most recently
from
8fa4f43
to
ccd31b0
Compare
wch
reviewed
Sep 19, 2019
NEWS.md
Outdated
| @@ -26,7 +26,7 @@ shiny 1.4.0 | |||
|
|
|||
| * Added `resourcePaths()` and `removeResourcePaths()` functions. ([#2459](https://github.com/rstudio/shiny/pull/2459)) | |||
|
|
|||
| * Resolved [#2515](https://github.com/rstudio/shiny/issues/2515): `selectInput()` now deals appropriately with named factors. ([#2524](https://github.com/rstudio/shiny/pull/2524)) | |||
| * Resolved [#2515](https://github.com/rstudio/shiny/issues/2515): `selectInput()` and `selectizeInput()` now deal appropriately with named factors. Note that `updateSelectInput()` and `updateSelectizeInput()` **do not** yet handle factors; their behavior is unchanged. ([#2525](https://github.com/rstudio/shiny/pull/2525)) | |||
There was a problem hiding this comment.
Oops, this should reference 2524 and 2625.
wch
approved these changes
Sep 19, 2019
alandipert
force-pushed
the
fix-selectize-update
branch
from
ccd31b0
to
8f2a28a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes #2624 by aligning the choice processing behavior of
selectizeInput()/selectInput()andupdateSelectizeInput()/updateSelectInput().choicesof length 1 that were unnamed lists were considered groups, not leaves.list(x=list(123))is an example. My changes in Fix selectInput/selectizeInput handling character(1) options #2540 altered that behavior so unnamed lists of length 1 were considered leaves, not groups. The bug was revealed during testing ofupdateSelectizeInput(), because its choice processing implementation considered them groups, just like the originalchoicesWithNames()code did before I rewrote it.isGroup()to include lists and added a test.isGroup()to also include them and added tests.asNamed()inprocessGroupedChoices()to ensuremapply()would work; the function inside is what produces the correct error. I added tests for this.NAandNULLwere untested. I added tests.Notes
Note that a previous iteration of this PR included a replacement for
updateSelectizeInput()'s choice processing code that built on #2540 instead of implementing choice processing separately.Unfortunately, that code necessitated 2 walks of the
choicestree, and so was twice as slow. We are highly sensitive to performance in this case as users selectserver = TRUEspecifically for performance reasons aschoicesis large.In order to compete with the existing
updateSelectizeInput()code,choicesWithNamesRectangle()would have needed to be rewritten as a single pass ofchoices, which we deemed too significant an undertaking given the urgency of a 1.4.0 release.So, instead of doing that, we reduced the scope of this PR to a small change of the new code and the addition of tests that bring it to feature parity with the code inside
updateSelectizeInput().