LocalJobs fixes #5648
Merged
LocalJobs fixes #5648
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
- Don't store `self.__job`, because that is invalidated by any modification to the path (e.g. `setFromString()`). Instead get it by a JobPool lookup each time it is needed. - Don't use the `JobPool.jobs()` index as the path, because it is not stable. Deleting a job changes the list index for other jobs, meaning that paths would change what they are referring to. This was particularly noticeable when deleting selected jobs - the selection would jump to some apparently random other job. - Preserve root and filter in `copy()` and `childNames()`. This meant adding a new `JobPool.jobsDict()` method to provide stable unique IDs for jobs, which also means that `remove()` is no longer linear in the number of jobs.
This fixes the following error when deleting a job at an inopportune moment in the UI : ``` Instance referenced by WeakMethod GafferDispatch.LocalDispatcher.__messagesChanged() no longer exists ```
The problem we're dealing with here is that selection is retained even when the paths don't exist any more, so when a job is removed we need to do an update (wherein we will realise that the selection doesn't refer to a job).
murraystevenson
approved these changes
Jan 31, 2024
There was a problem hiding this comment.
LGTM. Thanks for addressing these!
I did wonder if it would be better to move the selection to the job immediately above the old selection...
I suppose that is the usual behaviour, though I agree it doesn't seem particularly useful in this context. Let's keep that WIP squirrelled until someone finds a good reason for its existence...
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 few bugs in the LocalJobs panel that showed up while testing #5636. I'm making a separate PR for these because independent fixes for problems that already exist on
main. When deleting the selected jobs, the selection now reverts to empty rather than jumping to some random job. I did wonder if it would be better to move the selection to the job immediately above the old selection, and even did most of the work of implementing that, but I actually don't see it being much use to me so I backed it out. I've got the WIP for that squirreled away though if we do want it.