purebred-mua / purebred

A terminal based mail user agent based on notmuch
GNU Affero General Public License v3.0
139 stars 19 forks source link

Refactor HasToggleableList, and other cleanups #472

Closed frasertweedale closed 1 year ago

frasertweedale commented 1 year ago

Related to https://github.com/purebred-mua/purebred/issues/471, and accomplishes some of the refactors and cleanup I had in mind when filing that issue - but without any behavioural changes (hopefully!)

commit 20b614ad01a92127b0ab45ab365cc761cf72c943
Author: Fraser Tweedale <frase@frase.id.au>
Date:   Thu Jul 28 16:42:34 2022 +1000

    ui: simplify 'toggle'

    Simplify the Toggleable class and in particular the 'toggle' effect.
    The previous approach was "two phase" - first find the index of the
    current item, then toggle it.  We now avoid this by way of the
    'listSelectedElementL' Traversal, which targets the currently
    selected item.

    `listSelectedElementL` has been merged into Brick itself, but is not
    yet released.  We can migrate to the Brick-supplied version after it
    has been released.

commit b5b4f328c81adccc9c3b6bd37d8b1a6d54dd3f28
Author: Fraser Tweedale <frase@frase.id.au>
Date:   Fri Jul 29 11:34:57 2022 +1000

    ui: toggledItemsL: target "inner" values

    The toggledItemsL targets the whole `Toggleable a`, which includes
    the "is toggled" `Bool`.  This makes it an unlawful `Traversal` -
    you can changed the state that says which elements are targeted.  It
    is also unneeded.  Functions are forced to handle a (Bool, a), but
    always discard the first element.

    Fix these problem by changing 'toggledItemsL' to target the inner
    `a` values of the `Toggleable a` list items.  This makes it a lawful
    `Traversal` and simplifies the traversal functions.

commit 2e5257862c5f5dce21b9c0538d37531322e5c494
Author: Fraser Tweedale <frase@frase.id.au>
Date:   Tue Aug 2 22:01:31 2022 +1000

    ui: use type applications instead of Proxy

    Make things just a little tidier in our Purebred.UI.Actions by using
    -XTypeApplications.  This avoids the need to construct and
    explicitly pass around Proxy values.

commit 431d0e44fb7fe20fb35edcfedca98dead306574b (HEAD -> refactor/toggleable, origin/refactor/toggleable)
Author: Fraser Tweedale <frase@frase.id.au>
Date:   Tue Aug 2 22:08:24 2022 +1000

    ui: extract untoggleAll and toggledItemsL from class

    The `untoggleAll` and `toggledItemsL` functions are defined in terms
    of other members of `HasToggleableList` and its superclass
    `HasList`.  Extract them from the class as top-level values.
frasertweedale commented 1 year ago

looks at ci

welp. I thought this should be a mechanical refactor with no behavioural change. Obviously I was wrong. Converting to draft until I discover why it broke (or give up).

frasertweedale commented 1 year ago

OK, found and fixed the errors. Ready for review.

romanofski commented 1 year ago

Good stuff!