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

fix: improve scope handling #1189

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

w-lfchen
Copy link
Contributor

@w-lfchen w-lfchen commented Sep 3, 2024

Description

i hate state.

this pr aims to close #992 and close #794
there are architectural issues which cause the behavior described in the issue

basically, the CustomWidgetInvocation.scope field is only set once and then used for the scopes of children widgets. since it is never updated, it will never change, but this is needed when variables update.

Additional Notes

this should be tested well to prevent introducing a regression here.

if you are reading this, please test this with your configs and report whether the changes broke anything or if everything still works

Checklist

  • I added my changes to CHANGELOG.md, if appropriate.
    i don't think this is noteworthy enough
  • I used cargo fmt to automatically format all code before committing
  • clean up warnings that result from the changes

@@ -288,6 +288,9 @@ fn build_children_special_widget(
gtk_container: &gtk::Container,
custom_widget_invocation: Rc<CustomWidgetInvocation>,
) -> Result<()> {
// open a new scope for the children
// TODO this might be unnecessary, evaluate whether we can just use the calling_scope
let scope = tree.register_new_scope("children".into(), Some(calling_scope), calling_scope, HashMap::new())?;
Copy link
Contributor Author

@w-lfchen w-lfchen Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initial idea: register a new scope for the children, the name might need adjusting
i've pretty much copied this from the build_loop_special_widget function

@w-lfchen
Copy link
Contributor Author

w-lfchen commented Sep 4, 2024

currently, all children are in the same scope.
it might be better to create a new scope for every child

@rtoijala
Copy link

rtoijala commented Sep 5, 2024

Very nice to see such a quick response :). I finally had time to test this MR.

This breaks at least the following case:

(defwindow broken
  :monitor 0
  (widg :v "foo"))

(defwidget widg [v]
  (wrap
    {v}))

(defwidget wrap []
  (box
    (children)))

Trying to open broken fails with this message:

INFO  eww::app        > Opening window broken as 'broken'
Action failed with error: failed to open window `broken`

Caused by:
    Variable v not in scope

Without the changes in this MR, the window opens and displays "foo".

@w-lfchen
Copy link
Contributor Author

w-lfchen commented Sep 5, 2024

@rtoijala thank you so much for your testing!
i've updated some things (everything here is pretty hacky), and now your example works!
the following config seems to work as well (might be of use as a starting point for you):

(defpoll var
  :interval "1s"
  :initial '[0]'
'echo [0]')

(defwindow broken
  :monitor 0
  (box "test"
    (widg :v var)))

(defwidget widg [v]
  (wrap
  {v}))

(defwidget wrap []
  (box
    (for dummy in var
      (box
        (children)))))

please try to break things again, and thanks again for your cooperation!
by the way, if you run eww logs with a config containing a variable that is periodically updated, you should see how many handlers are registered for it. if that number grows indefinetly, something is wrong (but it should also throw the deallocation message).
this might be of use in your attempts to break things as well (will obviously be removed before merging)

@w-lfchen
Copy link
Contributor Author

w-lfchen commented Sep 6, 2024

i'll have to see whether i can investigate #675 as well
it doesn't seem to be directly related to this issue, but closing that one as well with this pr would be neat

hypernova7 added a commit to hypernova7/eww that referenced this pull request Oct 13, 2024
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

Successfully merging this pull request may close these issues.

Widget got deallocated [BUG] eww slowly fills ram
2 participants