44965: Failed test: Player - Video: show - #11107
Conversation
|
The label seem to be obligatory for some browsers (e.g. Firefox), so I would not make it an optional parameter even if it breaks backwards compatibility - hence the Jour Fixe label. |
|
Sry @alex40724, I didn't have time to review this yet. I therefore removed the JF label again. Please let the UI coordinators add this in the future. |
There was a problem hiding this comment.
Hi @alex40724,
Thx a lot for your contribution to the UI framework!
Please consider the following suggestions. You do not need to follow them, but please indicate shortly why you prefer to do otherwise:
-
Player\Video::withAdditionalSubtitleFile(): to provide JF with an alternative approach, we could add a default assignment of an empty string along with a deprecation notice for 10. This would require some additional logic inside the component and renderer, but it would prevent possible fatals and allows gradual fixing of plugins.
Please implement the following changes:
-
Player\Video::getSubtitleLabels(): please remove this method from the public interface.
I have added the JF label so we can decide on the direction of this PR.
Kind regards,
@thibsy (as UI coordinator)
|
Jour Fixe, 09 MAR 2026: We highly appreciate this PR and accept it and the breaking change for 11 and trunk. For 10 we would like to have an additional PR that sets an empty string for the label to prevent problems with already existing plugins. |
|
@alex40724 I assign this to you for the time being. Thx for working on this. |
|
@thibsy The requested changes are applied to the branch. |
thibsy
left a comment
There was a problem hiding this comment.
Hi @alex40724,
Thx for working on this again.
JF only accept the solution with a default value (string $label = '') for ILIAS 10 (see @matthiaskunkel comment above). Since this has been approved before the initial release of ILIAS 11, I would suggest to apply the same fix for ILIAS 11 as well now and only make this parameter mandatory for trunk.
Therefore, please implement the following changes:
- Optional parameter: please make the new parameter optional so we do not break existing usages.
- Conflicts: I cannot merge the PR due to conflicts, please rebase the branch onto latest
release_10.
Afterwards, this should be good to go. Let me know how you want to proceed with the bugfix and usages for trunk.
Kind regards,
@thibsy (as UI coordinator)
See https://mantis.ilias.de/view.php?id=44965
The PR also includes the text changes requested here:
https://mantis.ilias.de/view.php?id=44964