dropdown function is inefficient

[goolord]

the dropdown function is currently implemented like this

dropdown :: forall k t m. (DomBuilder t m, MonadFix m, MonadHold t m, PostBuild t m, Ord k) => k -> Dynamic t (Map k Text) -> DropdownConfig t k -> m (Dropdown t k)
dropdown k0 options (DropdownConfig setK attrs) = do
  optionsWithAddedKeys <- fmap (zipDynWith Map.union options) $ foldDyn Map.union (k0 =: "") $ fmap (=: "") setK
  defaultKey <- holdDyn k0 setK
  let (indexedOptions, ixKeys) = splitDynPure $ ffor optionsWithAddedKeys $ \os ->
        let xs = fmap (\(ix, (k, v)) -> ((ix, k), ((ix, k), v))) $ zip [0::Int ..] $ Map.toList os
        in (Map.fromList $ map snd xs, Bimap.fromList $ map fst xs)
  modifyAttrs <- dynamicAttributesToModifyAttributes attrs
  let cfg = def
        & selectElementConfig_elementConfig . elementConfig_modifyAttributes .~ fmap mapKeysToAttributeName modifyAttrs
        & selectElementConfig_setValue .~ fmap (T.pack . show) (attachPromptlyDynWithMaybe (flip Bimap.lookupR) ixKeys setK)
  (eRaw, _) <- selectElement cfg $ listWithKey indexedOptions $ \(ix, k) v -> do
    let optionAttrs = fmap (\dk -> "value" =: T.pack (show ix) <> if dk == k then "selected" =: "selected" else mempty) defaultKey
    elDynAttr "option" optionAttrs $ dynText v
  let lookupSelected ks v = do
        key <- T.readMaybe $ T.unpack v
        Bimap.lookup key ks
  let eChange = attachPromptlyDynWith lookupSelected ixKeys $ _selectElement_change eRaw
  let readKey keys mk = fromMaybe k0 $ do
        k <- mk
        guard $ Bimap.memberR k keys
        return k
  dValue <- fmap (zipDynWith readKey ixKeys) $ holdDyn (Just k0) $ leftmost [eChange, fmap Just setK]
return $ Dropdown dValue (attachPromptlyDynWith readKey ixKeys eChange)

this repeats a ton of work and calls a bunch of O(n*log n) functions. it also takes a Map as an argument, despite the fact that it just ends up turning it into a Bimap, which can lead to some confusion as Bimap will delete nonunique keys / values. if you write the function like this instead:

dropdown :: forall k t m. (DomBuilder t m, MonadFix m, MonadHold t m, PostBuild t m, Ord k) => 
  k -> Dynamic t (Bimap k Text) -> DropdownConfig t k -> m (Dropdown t k)
dropdown k0 options (DropdownConfig setK attrs) = do
  defaultKey <- holdDyn k0 setK
  modifyAttrs <- dynamicAttributesToModifyAttributes attrs
  let indexedOptions :: Dynamic t (Map k Text)
      indexedOptions = BM.toMap <$> options
  let cfg = def
        & selectElementConfig_elementConfig . elementConfig_modifyAttributes .~ fmap mapKeysToAttributeName modifyAttrs
        & selectElementConfig_setValue .~ attachPromptlyDynWithMaybe (flip BM.lookup) options setK
  (eRaw, _) <- selectElement cfg $ listWithKey indexedOptions $ \k v -> do
    let optionAttrs = (\dk v' -> "value" =: v' <> if dk == k then "selected" =: "selected" else mempty) <$> defaultKey <*> v
    elDynAttr "option" optionAttrs $ dynText v
  let eChange :: Event t (Maybe k)
      eChange = attachPromptlyDynWith (flip BM.lookupR) options $ _selectElement_change eRaw
  dValue <- holdDyn k0 $ fmapMaybe id $ leftmost [eChange, fmap Just setK]
  pure $ Dropdown dValue (fmapMaybe id eChange)

you eliminate a tremendous amount of work, at the expense of the "value" attribute not being the index.
if it is important that the value attribute stay the index, it would be trivial to just zip the index with one of the keys.

alternatively, instead of taking in a Bimap, it would also be possible to take some indexed data structure like a Set of pairs or something, but that would require an alternative to the listWithKey function to be written.

as long as this isn't controversial, it would be trivial for me to change this to taste and pr my changes

[goolord]

I have implemented this and it's currently in reflex-dom-contrib reflex-frp/reflex-dom-contrib#62