-
Notifications
You must be signed in to change notification settings - Fork 420
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 Displays
querying on every access and add DisplaysChanged
#5439
Conversation
`Displays` are used in a hot path in `UserInputManager.mouseOutsideAllDisplays` so storing is much more performant than querying SDL every time.
To support `SequenceEqual` as expected, `Display` equality has to be changed. This should also make `CurrentDisplayBindable` more accurate when something other than the index changes.
Displays
querying on every accessDisplays
querying on every access and add DisplaysChanged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks correct
/// <summary> | ||
/// Invoked when <see cref="Displays"/> has changed. | ||
/// </summary> | ||
[CanBeNull] | ||
event Action<IEnumerable<Display>> DisplaysChanged; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason a callback event is used instead of making the displays list into a bindable list instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had contemplated using a BindableList
, but this was scrapped for multiple reasons:
- using it as
ItemSource
in dropdowns would make sense, but this crashes as it's not on the update thread - updating all the elements in the list requires
Clear()
ing it first, and this would cause trouble with things that expect at least one available display BindCollectionChanged
/NotifyCollectionChangedEventArgs
doesn't provide the actual list (existing consumer code expects a list)- the way displays can change means that rebuilding the entire list is almost always necessary, so an event callback is simpler and gets the job done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- using it as ItemSource in dropdowns would make sense, but this crashes as it's not on the update thread
Dropdown
should be responsible for scheduling updates of ItemSource
, similar to how it already does with Current
.
- BindCollectionChanged / NotifyCollectionChangedEventArgs doesn't provide the actual list (existing consumer code expects a list)
A BindableList
can be iterated as a list, while NotifyCollectionChangedEventArgs
would only inform addition/removal of displays.
- updating all the elements in the list requires Clear()ing it first, and this would cause trouble with things that expect at least one available display
- the way displays can change means that rebuilding the entire list is almost always necessary, so an event callback is simpler and gets the job done
Fair enough, BindableList
would be a nice addition to simplify listing of displays, but I guess it could be left off as it is until it's necessary.
peppy reported it on discord:
Displays
were queried in a hot path (every frame).Adds
DisplaysChanged
event toIWindow
so consumers can watch for display changes. For now, this doesn't check if the displays actually changed, and will be invoked every time the displays are queried.