Skip to content
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

Feature/streamlist #10

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

Feature/streamlist #10

wants to merge 9 commits into from

Conversation

illanos
Copy link
Contributor

@illanos illanos commented Jan 24, 2023

Now you can add a list of feeds to the stream viewer
Now you can open a feed from a list
Now you can close all feeds

@@ -39,8 +41,13 @@ public MyApplication()
var loadModelFileAccess = AzureFileShareFileAccess.CreateFromUri(new Uri("https://waveengineagentdiag159.file.core.windows.net/models?st=2022-10-26T11%3A46%3A02Z&se=2028-10-27T18%3A46%3A00Z&sp=rl&sv=2018-03-28&sr=s&sig=dOR9IQtYCPMYfoP7TouKuh9UXjPQUMABAFLYkSbaPR0%3D"));
loadModelFileAccess.Cache = new DiskCache("models");

var imageGalleryFileAccess = AzureFileShareFileAccess.CreateFromUri(new Uri("https://xrvgallerystorage.file.core.windows.net/galleryimages/?sv=2021-06-08&ss=f&srt=sco&sp=rwdlc&se=2024-11-03T21:21:33Z&st=2020-11-03T13:21:33Z&spr=https&sig=Xh73u%2FIVcw00vCm%2BN3z5EbyaxaIuISfCUUk0mdCiDnI%3D"));
var imageGalleryFileAccess = AzureFileShareFileAccess.CreateFromUri(new Uri("https://xrvdevelopment.file.core.windows.net/tests?sv=2021-06-08&ss=f&srt=sco&sp=rl&se=2027-01-26T21:57:31Z&st=2023-01-25T13:57:31Z&spr=https&sig=B7Ds43k2m2fLC3pyRg2A1auTxZj8y1SALQh4iLVz3lk%3D"));
Copy link
Member

Choose a reason for hiding this comment

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

Why changing this? You are using storage account creating to check Azure file access in CI workflow, so it can break tests, or its contents can be removed by the tests themselves! It's true that we need to unify our samples in a single storage account, but that's not part of this PBI. Undo this change please.

})
.AddModule(new PainterModule());
{
Streams = new Streams[]
Copy link
Member

Choose a reason for hiding this comment

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

Acceptance criteria talks about a single URL for a JSON file containing all video sources. We don't need this Streams array here, just a property named SourcesListFileUrl. Module should retrieve the file and parse its contents.

[Optional] To change UI layout as less as possible, the proposal is that current JSON structure to be something like:
[ { "name": "Group/Category 1", "feeds": [ { "name": "House By lake", "url": "http://..." }, { "name": "Miramar", "url": "http://..." }, ] }, ]

So we can have categories in the left list, and feeds in the right part of the list.

private Dictionary<string, string> feedDic = new Dictionary<string, string>();

private float playerDistance = 0.8f;
private string playerDistanceTag = "playerDistanceTag";
Copy link
Member

Choose a reason for hiding this comment

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

Why changing previous distance? Leave all the windows to the default distance and remove these lines

var streamWindowEntity = this.streamWindow.Instantiate();

// Repositories list view
this.streamListView = streamWindowEntity.FindComponentInChildren<ListView>(true, tag: "PART_repositories", true, true);
Copy link
Member

Choose a reason for hiding this comment

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

All these "PART_*" things should be controlled by a component that controls window state. It seems that you have copy&pasted from module viewer module, but that should be moved.

/// <summary>
/// Gets current Selected url from list.
/// </summary>
public string SelectedUrl => this.feedListView.Selected.FirstOrDefault().ToString();
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to expose this information out of the modules. Remove this and also SelectedStream, that should be moved to a separated control to handle streaming sources list window.


private Entity CreateButton(string buttonText, Action releasedAction)
{
var buttonPrefab = this.assetsService.Load<Prefab>(CoreResourcesIDs.Prefabs.TextButton);
Copy link
Member

Choose a reason for hiding this comment

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

This way of creating UI instead of using a prefab could be a nightmare. Again it has been copy&pasted from module viewer, but all this should be moved to a prefab in that module too (not in this PR of course, just things related with streaming viewer)

var size = new Vector2(0.30f, 0.30f);
w = this.xrv.WindowsSystem.CreateWindow((config) =>
{
config.LocalizedTitle = () => streamUrl;
Copy link
Member

Choose a reason for hiding this comment

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

We said that URL could contain sensitive data, it would be better placing feed name here. It also will be easier for the user to identify the window.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants