ScriptWindow configuration fixes #6250
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is equivalent to `Editor.instanceCreatedSignal()`, providing opportunities for extensions to customise a ScriptWindow no matter how or when it is created. ScriptWindow and Editor have _very_ similar APIs at this point, and I wonder if at some point ScriptWindow might actually _be_ an Editor? The motivation for this is that several configs currently use `ScriptWindow.acquire()` in an attempt to get a ScriptWindow when a script is added. But this puts them in a tricky situation : - If they use `createIfMissing = True` then they are changing the point in time at which the ScriptWindow is created, which has proved fragile in the past. - If they use `createIfMissing = False` then they risk not receiving a ScriptWindow at all, since they are not in control of the point in time that it _is_ created. The new `instanceCreatedSignal()` provides a foolproof mechanism for a config to get access to every ScriptWindow while leaving the creation process itself well alone.
This was broken in c942d40, which moved ScriptWindow creation to _after_ the point the config files had run. Which meant that `acquire( createIfMissing = False )` always returned `None` and we never connected to the close signal.
Since c942d40, the ScriptWindow was getting created _after_ our `__scriptAdded()` callback, so we weren't updating it. We now use `ScriptWindow.instanceCreatedSignal()` to ensure we always get it set up, no matter when it is created.
|
If you do get a 1.5.4.1 release, can I also request that #6241 be included? That is a requirement for us to get it installed at IE. |
murraystevenson
approved these changes
Jan 31, 2025
There was a problem hiding this comment.
Thanks John! Looks good to me. Fixes aside, this is quite a nice improvement!
@ivanimanishi Noted! We'll include that fix in a 1.5.4.1 release.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a couple of regressions caused by c942d40, which was released in 1.5.3.0. That commit moved the ScriptWindow's
__scriptAddedslot to the back of the slot list, while before it had been at the front. This means that ScriptWindows are now created last, after all otherscriptAddedslots have run. This is a good thing - letting everyone configure the script how they want, and only then making the UI for it is far more natural.But we had a couple of configs which were using
ScriptWindow.acquire( createIfNecessary = False ), now at a time when the ScriptWindow hasn't been created yet. So they were skipping the configuration of the ScriptWindow that we needed. This PR solves that by introducing a newScriptWindow.instanceCreatedSignal()which is emitted whenever a ScriptWindow is made, and using that in the configs instead.I've targeted this PR to a new
1.5.4_maintenancebranch with the intention that we'll push the fix out immediately in a 1.5.4.1 release rather than waiting for 1.5.5.0. We want to get this fix out quickly, and there are a couple of extra things we still want to get into 1.5.5.0, and things currently merged that should wait for them. We'll just need to merge1.5.4_maintenanceto1.5_maintenanceafter making the release and then we should be back to our regular flow.