Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

[UI-library] Prevent errors on disabled setting or stats panel. #392

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

timbotimbo
Copy link
Contributor

Relevant components:

  • Signalling server
  • Frontend library
  • Frontend UI library
  • Matchmaker
  • Platform scripts
  • SFU

Problem statement:

You can disable the settings and stats panel when using the ui-library.

const application = new Application({
	stream,
	settingsPanelConfig: {
		isEnabled: false,
		visibilityButtonConfig: { creationMode: UIElementCreationMode.Disable }
	} ,
	statsPanelConfig: {
		isEnabled: false,
		visibilityButtonConfig: { creationMode: UIElementCreationMode.Disable }
	} 
});

However doing so might give you various errors because the settingsPanel or statsPanel variable will be undefined.

  • If you only disable the settings panel, the stats button will break.
    The console shows an error as it tries this.settingsPanel.hide();.

onclick

  • If you only disable the stats panel, you get the reverse with the settings button breaking.
  • However the stats panel is also called repeatedly in other functions like
    onPlayerCount, onVideoInitialized and onStatsReceived.
    Although these don't seem to break anything visible to the user, they throw hundreds of console errors during a short stream.

stats-error

Solution

Adding some ? operators on usage of this.settingsPanel and this.statsPanel to avoid throwing errors.

Documentation

N / A

Test Plan and Compatibility

Some additional null checks shouldn't break any compatibility or features.

The stats and settings button don't break anymore in manual testing.

Backporting

These errors show up starting with version 5.2 (which I use), but I'm making this pull request on master.
But because of code changes I'm not sure if a backport from master will fix all occurences in 5.2 and 5.3.

Please let me know If I need to make separate pull requests for other versions.

Signed-off-by: timbotimbo <timbotimbo@users.noreply.github.com>
Copy link
Collaborator

@Belchy06 Belchy06 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@lukehb lukehb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's a good idea. Code looks good to me. Lgtm.

@Belchy06 Belchy06 merged commit af5339b into EpicGames:master Oct 19, 2023
4 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 19, 2023
Signed-off-by: timbotimbo <timbotimbo@users.noreply.github.com>
(cherry picked from commit af5339b)
github-actions bot pushed a commit that referenced this pull request Oct 19, 2023
Signed-off-by: timbotimbo <timbotimbo@users.noreply.github.com>
(cherry picked from commit af5339b)
@github-actions
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
UE5.2 Backport failed because of merge conflicts

You might need to backport the following PRs to UE5.2:
- Merge branch 'master' into patch-1
- merged in origin master
UE5.3
UE5.4

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

backport --pr 392

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

3 participants