Improve Layout #36

minimapletinytools · 2021-01-24T22:04:25Z

This PR is paired with this issue explaining the changes.

Test cases for changes are here as it depends on reflex-vty-test-host which has not been merged yet.

Unfortunately the reflex frp network is a little convoluted. I think it pretty much has to be this way if we want to use the existing Layout monad design.

This PR is feature complete and has been tested.

Things left todo before PR is ready to be merged

~~improve documentation, possible add diagram showing how events are processed.~~
~~finish implementing and test "no focus" cases~~
~~finish implementing LayoutDebugTree~~
~~write test cases using https://github.com/pdlla/reflex-vty-test-host~~
~~update example code to use new Layout stuff~~

src/Reflex/Vty/Widget/Layout.hs

cgibbard · 2021-01-25T18:48:38Z

src/Reflex/Vty/Widget/Layout.hs

+
+-- TODO should prob be (Branch [LayoutDebugTree] | Leaf PosDim
+-- but it's weird cuz a leaf node won't know it's PosDim until combined with a Region...
+data LayoutDebugTree t = LayoutDebugTree_Branch [LayoutDebugTree t] | LayoutDebugTree_Leaf


I'm a little confused about what's going on with this type... t is phantom here? Is the idea to provide live debugging info about the layout tree? Maybe it would be useful to use Data.Tree for its drawTree function? I'm not sure.

I outlined it's intent in the attached issue but yeah basically what you said except for automated testing not debugging (pardon the name). I think it might be quite useful for non-testing cases too.

This part of the code is entirely incomplete. Once folks are OK with the overall implementation I'll move forward with finishing this part of the code.

cgibbard · 2021-01-25T18:53:11Z

src/Reflex/Vty/Widget/Layout.hs

+emptyLayoutDebugTree :: LayoutDebugTree t
+emptyLayoutDebugTree = LayoutDebugTree_Leaf
+
+class IsLayoutReturn t b a where


This polymorphism seems unnecessary. Maybe just make a record data type something like:

data LayoutInfo t = LayoutInfo { layoutInfo_numChildren :: Int , layoutInfo_focusedDyn :: Dynamic t (Maybe Int) , layoutInfo_tree :: LayoutDebugTree t }

and then pair it with the result of the layout thing, and a simplified version which discards this info can be provided alongside?

That seems like it should do away with the awkward ambiguous types.

Yes, I think you're right. I had really hoped that there wouldn't need to be multiple variations of fixed/stretch and the interface could be seamless between sublayout and non-sublayout cases. I don't think that's possible anymore in which case this type class is unnecessary 😭

I decided to keep the typeclass because it makes the implementation of tile simpler. The typeclass is hidden in the package. I could also just remove it and modify tile_ implementation

cgibbard · 2021-01-25T19:28:36Z

Thanks for working on this problem! It's something that Ali and I were thinking about how to do early on, but one of our approaches to it got mired in some details surrounding Adjustable, and we ended up leaving something oversimplified in its place.

minimapletinytools · 2021-01-26T00:08:25Z

You're welcome! Glad to be contributing to such an awesome project!

I built off the existing implementation and felt what I ended up with got a little convoluted :(. It does however work and it seems to me that the complexity is inevitable if the network needs to pass through the Layout/VtyWidget boundary.

If the overall approach and modified interface looks OK I'll start finalizing this PR (which may take a while).

cgibbard · 2021-01-26T08:34:33Z

I think it's a good start, and that we should definitely do something along these lines, even if it ends up looking different by the time it's 100% ready to go. Starting from something that works is always great. :)

If you can get a simple version of this building, then it'll be easier for us to have a deeper look at it and maybe figure out how to make it more satisfying if possible.

Our original idea was to have the Layout monad be the thing that kept track of tab order, so that it would usually be unnecessary to have tab order leave one runLayout and enter another and vice-versa, but if there's a reasonable scheme to make that possible, all the better. (The original way it was going to do that was with a forwards-and-backwards State monad that let us send events forward and backward to adjacent tiles in the Layout, but state monads of any kind are tricky to write satisfactory Adjustable instances for. Avoiding that shenanigans and just doing the thing in a way that involves a bit of global oversight seems the way to go.)

minimapletinytools · 2021-02-01T00:02:00Z

The newest revision is mainly cleanup and documentation. It should compile and run now as well. For a straight-forward example, you can run easyExample in example.hs.

Still need to implement LayoutDebugTree and write tests. Note that I'll be writing the tests in my own fork for now as the vty testing framework I wrote/want to use depends on this PR

minimapletinytools · 2021-02-01T00:12:55Z

src/Reflex/Vty/Widget/Layout.hs

-Description: Monad transformer and tools for arranging widgets and building screen layouts
-}
-{-# LANGUAGE FlexibleInstances #-}
+{-# LANGUAGE AllowAmbiguousTypes        #-}


BTW, what linter do y'all prefer?

We haven't been using one, but I will say that generally we don't align things beyond the margin (apart from in cases where it will make code correctness much more obvious), since it leads to people who aren't using the same tools doing a bunch of extra work to keep stuff aligned, and the subsequent whitespace changes make the diffs harder to examine.

minimapletinytools · 2021-02-04T06:32:08Z

The latest revision implements LayoutTree and fixes a few minor issues. I also wrote unit tests for Layout using my own testing framework. Please take a look here

PR is feature complete. Looking forward to y'alls feedback.

Related plug, I'd like to merge my testing framework reflex-vty-test-host someday since I think it's super useful for testing. It's dependent on this PR which has yet to be reviewed 😭

…per methods

cgibbard · 2021-02-22T15:14:12Z

Having another look now, and I'll take a look at the testing one right after.

ali-abrar · 2021-03-14T05:08:44Z

@pdlla Your PR got me thinking and I'm going to improve this a bit further this weekend prior to merging. I think some of the awkwardness you're working around can be avoided entirely (though it'll involve breaking changes).

ali-abrar · 2021-03-15T13:46:50Z

src/Reflex/Vty/Widget/Layout.hs

 -- | The Layout monad transformer keeps track of the configuration (e.g., 'Orientation') and
 -- 'Constraint's of its child widgets, apportions vty real estate to each, and acts as a
-- switchboard for focus requests. See 'tile' and 'runLayout'.
+-- switchboard for focus requests. See 'tile_' and 'runLayout'.
 newtype Layout t m a = Layout


I've come to think it was a mistake to combine the layout and focus selection functionality into one monad. All of this might be substantially easier if the two concepts were separate. One of the problems with the old implementation was that the NodeId order was significant - i.e., things were focused in the order they were created. We shouldn't depend on that kind of ordering at all - which I think is what the Int you've got here is all about.

We can use ordered-containers so that we get efficient lookups while preserving order.

I agree with your points. I definitely hope for a simpler implementation. Having tab navigable focus that just works is great of course but this code is rather unreasonable to understand/modify limiting its usefulness for developers.

Unclear what your proposed alternative is apart from simply removing this feature. It seems to me passing focus events into a nested layout monad tree would involve re-implementing what we have here one way or another (ordered-containers vs nodeid<->index bimap will only make things a little less complex).

ali-abrar · 2021-03-23T01:13:07Z

This ended up growing into a larger refactor than originally intended and introduces many breaking changes (though I think they're good ones). Let me know what you think about this interface.

To make that diff more consumable, I added a changelog entry, but that's still a lot of information. The executive summary is:

Layout and Focus have been broken up into separate monads (rather than piggy-backing on layout for focus information)
Most widgets have been made "classier" so that they can be dropped in anywhere in a layout
the layout combinators have been refactored to (I hope) make layout construction more comprehensible

minimapletinytools · 2021-03-23T03:38:18Z

This ended up growing into a larger refactor than originally intended and introduces many breaking changes (though I think they're good ones). Let me know what you think about this interface.

To make that diff more consumable, I added a changelog entry, but that's still a lot of information. The executive summary is:
* Layout and Focus have been broken up into separate monads (rather than piggy-backing on layout for focus information)

* Most widgets have been made "classier" so that they can be dropped in anywhere in a layout

* the layout combinators have been refactored to (I hope) make layout construction more comprehensible

Cool, turning into a PR to make it easier for me to comment. Probably won't get through it today.

ali-abrar · 2021-04-09T17:59:05Z

Closing in favor of #43

temp PR

e089d0d

minimapletinytools mentioned this pull request Jan 24, 2021

Layout does not support tab focus shifting on nested layouts #37

Closed

minimapletinytools requested a review from ali-abrar January 24, 2021 22:08

ali-abrar requested review from cgibbard, a team and Ericson2314 January 25, 2021 14:38

cgibbard reviewed Jan 25, 2021

View reviewed changes

src/Reflex/Vty/Widget/Layout.hs Outdated Show resolved Hide resolved

cgibbard reviewed Jan 25, 2021

View reviewed changes

src/Reflex/Vty/Widget/Layout.hs Outdated Show resolved Hide resolved

cgibbard reviewed Jan 25, 2021

View reviewed changes

works now

6eec462

minimapletinytools requested a review from cgibbard January 31, 2021 23:54

minimapletinytools changed the title ~~[PROPOSAL] Improve Layout~~ Improve Layout Feb 1, 2021

put comment back

00a0598

minimapletinytools commented Feb 1, 2021

View reviewed changes

stylish

c733f26

minimapletinytools added 2 commits February 3, 2021 22:34

implement LayoutTree and change children to dynamic plus add some hel…

dac5ed4

…per methods

fix unfocus behavior in layout

ed239d3

ali-abrar reviewed Mar 15, 2021

View reviewed changes

ali-abrar closed this Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Layout #36

Improve Layout #36

minimapletinytools commented Jan 24, 2021 •

edited

Loading

cgibbard Jan 25, 2021

minimapletinytools Jan 25, 2021 •

edited

Loading

cgibbard Jan 25, 2021 •

edited

Loading

minimapletinytools Jan 26, 2021 •

edited

Loading

minimapletinytools Jan 31, 2021

cgibbard commented Jan 25, 2021

minimapletinytools commented Jan 26, 2021 •

edited

Loading

cgibbard commented Jan 26, 2021

minimapletinytools commented Feb 1, 2021 •

edited

Loading

minimapletinytools Feb 1, 2021 •

edited

Loading

cgibbard Feb 22, 2021

minimapletinytools commented Feb 4, 2021 •

edited

Loading

cgibbard commented Feb 22, 2021

ali-abrar commented Mar 14, 2021

ali-abrar Mar 15, 2021

minimapletinytools Mar 15, 2021

ali-abrar commented Mar 23, 2021 •

edited

Loading

minimapletinytools commented Mar 23, 2021

ali-abrar commented Apr 9, 2021

Improve Layout #36

Improve Layout #36

Conversation

minimapletinytools commented Jan 24, 2021 • edited Loading

Things left todo before PR is ready to be merged

cgibbard Jan 25, 2021

Choose a reason for hiding this comment

minimapletinytools Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

cgibbard Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

minimapletinytools Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

minimapletinytools Jan 31, 2021

Choose a reason for hiding this comment

cgibbard commented Jan 25, 2021

minimapletinytools commented Jan 26, 2021 • edited Loading

cgibbard commented Jan 26, 2021

minimapletinytools commented Feb 1, 2021 • edited Loading

minimapletinytools Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

cgibbard Feb 22, 2021

Choose a reason for hiding this comment

minimapletinytools commented Feb 4, 2021 • edited Loading

cgibbard commented Feb 22, 2021

ali-abrar commented Mar 14, 2021

ali-abrar Mar 15, 2021

Choose a reason for hiding this comment

minimapletinytools Mar 15, 2021

Choose a reason for hiding this comment

ali-abrar commented Mar 23, 2021 • edited Loading

minimapletinytools commented Mar 23, 2021

ali-abrar commented Apr 9, 2021

minimapletinytools commented Jan 24, 2021 •

edited

Loading

minimapletinytools Jan 25, 2021 •

edited

Loading

cgibbard Jan 25, 2021 •

edited

Loading

minimapletinytools Jan 26, 2021 •

edited

Loading

minimapletinytools commented Jan 26, 2021 •

edited

Loading

minimapletinytools commented Feb 1, 2021 •

edited

Loading

minimapletinytools Feb 1, 2021 •

edited

Loading

minimapletinytools commented Feb 4, 2021 •

edited

Loading

ali-abrar commented Mar 23, 2021 •

edited

Loading