[confirm] implement prompt for trade agreement screen #915
Conversation
Co-authored-by: Alan <[email protected]>
top aligned, not centered
| @@ -40,10 +40,6 @@ function ConfirmConf:init() | |||
| error('context must be set to a bounding focus string') | |||
| end | |||
|
|
|||
| if registry[self.id] then | |||
| error('id already registered: ' .. tostring(self.id)) | |||
There was a problem hiding this comment.
I didn't realize this change was added. I think the duplicate ID check is still useful. Does registry really need to persist across script runs? If it does, we could keep a separate local registry of just the names and use that to check for duplicate names within the same script invocation.
There was a problem hiding this comment.
If registry does need to persist between script runs, then I think this change is also problematic because it results in every entry in registry being clobbered. If clobbering every entry is ok, then clearing registry at the beginning of the script should also be ok, and we can keep the duplicate name check as it was before.
There was a problem hiding this comment.
The registry is read only after initialization. We have some leeway in how we treat it.
For an end user, the registry will not be changing between runs, so I think the solution we decide on should focus on developers. Any solution here, including the existing one, would work just fine for end users.
The contents of the registry are important at two particular points: when the config file is read in and when the overlay is initialized. The registry at config load time determines what new entries are added to the stored and live state. The registry at overlay init time determines what frames are instantiated for click detection.
If you edit in a new config to the registry, a global, multi-initialized registry will allow the overlay to adopt the new config without reloading (as long as the config doesn't define an intercept frame), but the dev will have to run GUI/confirm to manually enable it since it won't be in the state. If it does include an intercept frame, then the overlay will need to be reloaded (which is usual for overlay development).
If the registry is local and single initialized, then the overlay will have to be reinitialized to even be aware of the addition. The config will also need to be explicitly enabled in the session to have it appear in the state.
Tl;dr, I think it gets unnecessarily complicated if we allow multi init. How about this:
Option 1: we reinstate the next check (or something like it) and add a note to developers that devel/clear-script-env confirm should be run to reinitialize and the overlay needs to be reloaded.
Option 2: we put the registry in a module as a local and let module reload semantics keep it to single init and handle reload when it is edited.
There was a problem hiding this comment.
Right, this is only relevant to developers. I don't like option 1 because it's inconsistent with most other scripts (although the previous implementation admittedly required reload(), which was also atypical for scripts, but typical for plugins). I don't understand how option 2 would work.
I just now saw 86dcafb, which supersedes this conversation. If resetting REGISTRY whenever internal/confirm/specs.lua is run works, that's fine with me. I wasn't sure if that was possible or if some state was tied to the contents of REGISTRY that would get cleared by doing this.
That was the "put it into a module" option, which I tried and it works well. I was worried about desynchronizing the registry and the config state, so I put them both in the same module and that solves that. |
Fixes DFHack/dfhack#3912