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.

Sign up for GitHub

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

Jump to bottom

separate settings matcher to work on each property individually #4018

Merged
merged 8 commits into from Nov 3, 2022
Merged

separate settings matcher to work on each property individually #4018

merged 8 commits into from Nov 3, 2022

Conversation

stschr
Copy link
Contributor

@stschr stschr commented Aug 5, 2022

This change should fix both #3961 and also #3967
More intense testing or samples deemed beneficial.

@stschr stschr marked this pull request as ready for review August 26, 2022 08:48
@stschr stschr force-pushed the sschr/prefBasedSelection_3961 branch from 7920f7e to b513d27 Compare September 1, 2022 14:01
@dsilhavy
Copy link
Collaborator

dsilhavy commented Sep 5, 2022

@stschr Thank you for this, looks really good. Before I do the final code review: Did you already sign the feedback agreement: https://dashif.org/docs/DASH-IF-Feedback-Agreement-3-7-2014.pdf : If not can you please send the signed agreement to me to forward to DASH-IF.

Thanks

@dsilhavy dsilhavy added this to the 4.5.0 milestone Sep 5, 2022
@dsilhavy dsilhavy modified the milestones: 4.5.0, 4.6.0 Sep 20, 2022
@stschr stschr force-pushed the sschr/prefBasedSelection_3961 branch from b513d27 to 4959923 Compare September 20, 2022 15:44
@stschr stschr marked this pull request as draft September 21, 2022 07:23
@stschr stschr force-pushed the sschr/prefBasedSelection_3961 branch from 51ab0cf to 4c247c9 Compare September 21, 2022 07:57
@stschr stschr marked this pull request as ready for review September 21, 2022 07:59
@dsilhavy
Copy link
Collaborator

In addition to the two comments above: There is a reference to mediaController.matchSettings in TextController which needs to be resolved according to the new logic as well.

@stschr stschr force-pushed the sschr/prefBasedSelection_3961 branch from 3bb7a49 to 3fbef1c Compare October 13, 2022 16:11
@stschr
Copy link
Contributor Author

stschr commented Oct 13, 2022

On TextController referencing mediaController.matchSettings :
At least this issue is fixed now, as I re-added the old code as interim solution. Bad thing is that we have duplicated code now.

I can work on the TextController as well, but from what I have seen, there are no related unit tests so far. so, this might take a little longer... All audio related code should be fixed.

Up to you (@dsilhavy) to decide whether to close this PR now and file a new issue, or to keep this open an dwait for some more code on text.

@stschr stschr force-pushed the sschr/prefBasedSelection_3961 branch from 4224484 to 100f7dc Compare October 19, 2022 11:54
@dsilhavy dsilhavy merged commit 2b0848e into Dash-Industry-Forum:development Nov 3, 2022
@dsilhavy dsilhavy mentioned this pull request Feb 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsilhavy dsilhavy dsilhavy requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.5.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@stschr @dsilhavy