Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dyn-view with hidden observable dependencies #23

Closed
benknoble opened this issue Jun 7, 2022 · 11 comments
Closed

dyn-view with hidden observable dependencies #23

benknoble opened this issue Jun 7, 2022 · 11 comments

Comments

@benknoble
Copy link
Contributor

I had a function that looked like this:

(define (make-creature-view k @e)
  (define make-player-or-monster-group-view
    (match-lambda
      [(cons _ (? player?)) (make-player-view k @e)]
      [(cons _ (cons _ (? monster-group?))) (make-monster-group-view k @e)]))
  (dyn-view @e make-player-or-monster-group-view))

[Ignore the (cons _ patterns; I am still deciding how best to restructure the data for that part.]

The make-creature-view function is used in a list-view over an observable @creatures which holds items that are either player? or monster-group? (again, ignoring the initial cons bits).

The dyn-view lets me do the job of an if-view/cond-view but without constructing the sub-views of all conditional branches when the data doesn't match what is needed (e.g., with a cond-view here, I can't avoid that the monster-group-view gets a player? or vice-versa, because the sub-views are constructed greedily). At least, I think this is why I am using dyn-view; I can't remember.

But here's where things get interesting: the view returned by make-player-view has no dependencies other than @e, which (conveniently) the dyn-view also monitors and reacts to. On the other hand, make-monster-group-view creates a view that depends on an observable not mentioned here. I update it in response to other events and was expecting the GUI to update with new information, but it doesn't (presumably because dyn-view doesn't know about it, so can't forward updates, so nothing is redrawn).

There's a very silly hack to fix this (notice the obs-combine and the extra cons patterns):

(define (make-creature-view k @e)
  (define make-player-or-monster-group-view
    (match-lambda
      [(cons _ (cons _ (? player?))) (make-player-view k @e)]
      [(cons _ (cons _ (cons _ (? monster-group?)))) (make-monster-group-view k @e)]))
  (dyn-view
    ;; HACK: Combine @e with @ability-decks to register dyn-view dependency on
    ;; @ability-decks; but, the actual observable we care about is still just
    ;; @e. (You can see that we ignore the car of the resulting pair in the
    ;; match patterns above.) This is because make-monster-group-view creates
    ;; a view that depends on @ability-decks, but dyn-view isn't aware of this
    ;; dependency.
    (obs-combine cons @ability-decks @e)
    make-player-or-monster-group-view))

This works (!) but isn't scalable or maintainable: if more dependencies are added to make-monster-group-view, they need to balloon into the obs-combine and match, too. And if one is forgotten, there's a sort of "spooky action at a distance" bug like the lack of updates I described above.

I actually first tried a slightly better version that used (obs-combine (lambda (a e) e) @ability-decks @e), but that didn't work. It only improved the match, too; my points above still apply.

Have you encountered something like this before? Do you have a suggestion to alleviate this? It's like I want the static parts of if-view but the dynamic parts of dyn-view (again, because of the issue I have with disjoint data).

@Bogdanp
Copy link
Owner

Bogdanp commented Jun 8, 2022

This is a limitation of dyn-view, and one of the reasons it's not documented yet/why I'm not sure it's a good idea. The problem is that while it can discover what dependencies the generated views have, it has no way to register those deps with the renderer. Probably the easiest fix would be to make dyn-view add its own observers to dependencies reported by its children, but I'll have to sit down and think that through in case it would cause other problems. In the mean time, using obs-combine is the right approach here, though you also have to make sure your monster group view uses the ability decks value from the combined observable and not the main one (if you don't, things might work out most of the time, but you're going to be depending on a race between the observables).

@benknoble
Copy link
Contributor Author

This is a limitation of dyn-view, and one of the reasons it's not documented yet/why I'm not sure it's a good idea. The problem is that while it can discover what dependencies the generated views have, it has no way to register those deps with the renderer. Probably the easiest fix would be to make dyn-view add its own observers to dependencies reported by its children, but I'll have to sit down and think that through in case it would cause other problems.

That makes sense, and is kind of what I figured. It's unfortunate that cond-view really won't work here without producing errors (though IIRC they don't prevent the application from running).

In the mean time, using obs-combine is the right approach here, though you also have to make sure your monster group view uses the ability decks value from the combined observable and not the main one (if you don't, things might work out most of the time, but you're going to be depending on a race between the observables).

That is unexpected, in some way, but also makes sense. Unfortunately the use of the @ability-decks in make-monster-group-view needs to be an observable—is your suggestion just to @-wrap the correct value, then?

At any rate, this might give me the motivation I need to drop some of the cons objects and put more things in structs (there's already a large number floating around, but no harm in adding more).

Bogdanp added a commit that referenced this issue Jul 9, 2022
@Bogdanp
Copy link
Owner

Bogdanp commented Jul 10, 2022

I just pushed a set of changes to track hidden deps in dyn-views and to make it so that if-view (and derivatives like cond-view and case-view) short circuit. This should make it easier to create disjoint views like you're trying to do, though there are some caveats still:

;; While the `case-view' short circuits, changing the underlying type
;; of an observable isn't a completely safe operation because derived
;; observables aren't immediately garbage-collected when a view is
;; destroyed.
;;
;; For example, when `@current-character' is a `human', the
;; `human-editor' procedure creates a derived obervable that maps
;; `@current-character' to a string name using `human-name'. After
;; that view is replaced, that mapping lingers for some time until it
;; is garbage-collected, and if the "type" of character changes, it
;; raises an error asynchronously. That error can be safely ignored
;; since it's on a different thread, but that's not exactly pretty.
;; Instead, we use this procedure to ensure disjoint views always get
;; observables of the correct type by plugging in dummy values when
;; the type changes.

@benknoble
Copy link
Contributor Author

benknoble commented Jul 10, 2022

This is wonderful! I haven't updated to try this out yet, but I plan to soon.

I completely follow the coerce problem in your link, but I'm still trying to digest what's happening in 87580f8 where you say that referring to a static view is a problem: does this mean that static views should be considered something of an anti-pattern (i.e., all views should be returned by functions)?

@Bogdanp
Copy link
Owner

Bogdanp commented Jul 11, 2022

Yes, I think they should generally be avoided. Ultimately, views are objects with internal mutable state so re-using the same view can be problematic.

In that case specifically, if-view calls destroy on its children when they get replaced, so the static view would get torn down after its first use and on subsequent uses it would appear empty. This would've been a problem before this set of commits too, but commit bebc226 makes it more apparent since it removes children from containers to avoid retaining them unnecessarily.

@Bogdanp
Copy link
Owner

Bogdanp commented Jul 11, 2022

I want to expand on this some more, in case I forget some of these details by the next time I get to work on this stuff.

Initially, when I came up with view<%>, I imagined view implementations would end up being stateless. That's why create returns a native widget instance and why update and destroy get passed those instances. In those cases, you can pass the same instance of a view<%> around multiple times and it won't cause any issues because the instances don't have any internal state. By the time I started implementing container views, I stopped worrying about retaining that property, because it made the implementation of those views much simpler. However, that breaks the promise that view<%>s are purely representations of GUIs, which I'm not too happy about, but, in practice, it doesn't seem to cause many issues (of all the GUIs I've written with gui-easy so far, that example was the only one where I had reused a view like that).

I think for now I'm going to leave things as they are so we can see how much this trips people up. If it turns out to cause a lot of pain and confusion, what we can do is change those views that are stateful to parameterize their state per native widget instance. To give a concrete example, if-view's then-view and else-view would change from being (or/c #f (is-a/c view<%>)) to (weak-hasheq/c any/c (or/c #f view<%>)). Alternatively, we could wrap native widgets in a mixin that lets us store contextual data directly on those instances, simplifying some of the state & memory management.

Bogdanp added a commit that referenced this issue Jul 11, 2022
Bogdanp added a commit that referenced this issue Jul 11, 2022
@benknoble
Copy link
Contributor Author

Ok, that seems to make sense to me at the moment.

One last question: I saw a current-renderer parameter at a glance; is that considered public/stable? I don't recall seeing an update to the docs mentioning it, but it is certainly something I'm interested in. Though, using (render (dialog …)) without a parent argument hasn't caused any trouble for me so far…

@Bogdanp
Copy link
Owner

Bogdanp commented Jul 11, 2022

No, it won't be public. I added it for internal use only and mostly as a way of getting around changing the view<%> interface s.t. create, update and destroy take an additional renderer param. I probably will eventually change the interface and get rid of it.

You can bind the top level window's renderer (or store it in your own param) and use it to make modal dialogs already (see "examples/modal.rkt"). What you can't currently do is bind the renderer for a dialog because rendering a dialog blocks until the dialog is closed. I haven't yet thought of a nice way to get around that.

@benknoble
Copy link
Contributor Author

Got it, thanks!

@benknoble
Copy link
Contributor Author

benknoble commented Jul 12, 2022

Here's an example of making your own parameter, for the record. I couldn't think of a thread-safe/re-entrant way to do it without a parameter containing a thunk:

#lang racket
(require racket/gui/easy)
(define cr (make-parameter (thunk #f)))
(define root
  (parameterize ([cr (thunk root)])
    (render
      (window
        (button
          "Click me"
          (lambda ()
            (render
              (dialog
                (text "hello"))
              ((cr)))))))))

The view construction could then be placed in its own function(s) as long as it had access to the parameter. Thunk-ing is necessary because without it the value root is evaluated in the first form of parameterize before root is bound to the value that parameterize would return (i.e., it is undefined)!

@benknoble
Copy link
Contributor Author

Thank you for this; unless you feel strongly otherwise, I'm going to close this issue because I was successfully able to update to 0.5 and use a cond-view where I had this dyn-view before. 🎉 ❤️

benknoble added a commit to benknoble/frosthaven-manager that referenced this issue Jul 13, 2022
Thanks to work by @Bogdanp in
Bogdanp/racket-gui-easy#23, I can replace the
`dyn-view` + `obs-combine cons` hack with an actual `cond-view`, now
that `if-view` and descendants are more dynamic and do not force
ahead-of-time evaluation.

Note that `make-monster-group-view` returns to receiving `@ability-decks`
like it effectively did in
90114f2
Then, because `@action` is derived from it (along with `@mg` and
therefore `@e`), the returned view correctly depends on
`@ability-decks`, which was the reason for the `obs-combine cons` hack
in the first place!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants