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

Safe widgets reordering api #3941

Merged
merged 9 commits into from
Nov 20, 2024

Conversation

saurtron
Copy link
Contributor

@saurtron saurtron commented Nov 19, 2024

Work done

  • Create an internal mechanism at widgetHandler to queue methods with callin list reordering side effects.
    • Doesn't change the widget facing api, but related methods will now be queued after the callin loop.
    • Most widgets shouldn't notice any difference and this shouldn't break any existing widgets (see below for an exception though).
  • Affected methods: InsertWidget, RemoveWidget, EnableWidget, DisableWidget, ToggleWidget, LowerWidget, RaiseWidget, UpdateWidgetCallIn, RemoveWidgetCallIn (they all should become safe to use with this PR).
  • It does affect tests since those usually need immediate enabling/disabling (see below for related proposal). I adapted the current ones in master in this PR so they still work.
  • Adapted widget profiler too, since it needs to hook the real UpdateWidgetCallIn and InsertWidget (now with 'Raw' appendix).

Addresses Issue(s)

Discussion

  • Same idea as: Add a mechanism so gadgetHandler reordering api can be used safely. #3907 (this is the second part of the same effort).
  • As commented above, this affects a number of tests using EnableWidget and DisableWidget directly, also some in PRs.
    • I noticed all of them use these just for a common idiom of enabling the test widget with locals and then restoring common state.
    • I propose augmenting Test api with two methods, to simplify test code and also better hide implementation details: see this commit.
    • Haven't added that in this PR since it's out of scope, still I think it's worthy for a future PR (can add that to this PR if you want though since it's kind of related).
    • Other options for tests could be call PerformReorders at certain places, but I don't think this will work very well since it can be too late for the test code so not a general solution.
    • Another option would be to give them a widgetHandler proxy with immediate calls, but this can be fragile since they can run into queued side effects from other methods.
    • All in all, I think test writers will need to be aware of the queueing and either call PerformReorders themselves when needed or directly use Raw methods depending on the situation. I'm all for documenting this but not sure where.
  • The following methods have special handling too: ConfigureLayout, DrawScreen, Update, MouseMove, MouseRelease, since they don't get wrapped like other callins and/or have special reordering side effects.

Test steps

  • Checkout branch
  • See everything still works

Additionally, the following snippet can be used for testing at least default ordering and enabled widgets after initialization doesn't change with this PR:

       Spring.Echo("ORDERING")
       for _, w in pairs(self.widgets) do
               Spring.Echo(w.whInfo.basename, w.whInfo.layer)
       end
       Spring.Echo("END ORDERING")
       Spring.Echo("DrawScreen")
       for _, w in pairs(self.DrawScreenList) do
               Spring.Echo(w.whInfo.basename, w.whInfo.layer)
       end
       Spring.Echo("END ORDERING")
  1. Add snippet at the end of widgetHandler:Initialize, both for master and safe-ordering-widgets branches:

  2. Manually cut everything not between ORDERING and second END ORDERING at infolog.txt. Save in two txt files file1.txt and file2.txt, each for the run with each branch.

  3. Run cut -d' ' -f2- file1.txt > f1.txt and cut -d' ' -f2- file2.txt > f2.txt to remove the timestamps.

  4. diff f1.txt f2.txt -> yields empty result

Sample resulting files: f1.txt, f2.txt (yes they look like the same file)

@saurtron saurtron changed the title Safe reordering widgets Safe reordering widgets api Nov 19, 2024
@saurtron saurtron changed the title Safe reordering widgets api Safe widgets reordering api Nov 19, 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.

2 participants