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

[Basic UI] Add support for webaudio #1426

Merged
merged 2 commits into from
Aug 15, 2022
Merged

Conversation

wborn
Copy link
Member

@wborn wborn commented Jun 22, 2022

Similar to #1422 but for Basic UI. 🙂

@ghys
Copy link
Member

ghys commented Jul 6, 2022

Making a suggestion/pledge to make it opt-in as in #1422 (comment), because of the 6 max SSE connections per domain.

Since Basic UI pages are generated server-side there could be an additional setting in the service configuration which would conditionally enable this code.

image

@lolodomo
Copy link
Contributor

lolodomo commented Jul 7, 2022

I agree with the need of an option.

@wborn
Copy link
Member Author

wborn commented Jul 9, 2022

Config option added. ✔️

Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Wouter Born <github@maindrain.net>
@lolodomo
Copy link
Contributor

I will test and review.

@wborn
Copy link
Member Author

wborn commented Aug 14, 2022

@lolodomo if the changes don't work, make sure you have:

  • Reloaded Basic UI after enabling the Basic UI Web Audio setting
  • Navigated to a sitemap
  • Interacted with the Basic UI (or change your browser setting to always allow audio playback for the site)

@@ -42,6 +42,15 @@
</options>
<default>false</default>
</parameter>
<parameter name="webAudio" type="text" required="true">
Copy link
Contributor

Choose a reason for hiding this comment

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

You used the same pattern (text type) as for other parameters.
I do not understand why boolean type was not used for enableIcons, condensedLayout, ... parameters.
But your new parameter is at least coherent with others.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I kept the new parameter coherent with the other config parameters. It seems that @kaikreuzer added the inital config parameter in eclipse-archived/smarthome#540 like this. Maybe because it easier to understand (compared to a checkbox)?

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM. I will merge after a successful test.

@lolodomo lolodomo added the basic ui Basic UI label Aug 15, 2022
@lolodomo
Copy link
Contributor

Tested and working with barking.mp3

@lolodomo lolodomo merged commit ba45055 into openhab:main Aug 15, 2022
@lolodomo
Copy link
Contributor

Thank you @wborn

@lolodomo lolodomo added this to the 3.4 milestone Aug 15, 2022
@wborn wborn deleted the basicui-webaudio branch August 15, 2022 08:45
wborn added a commit to wborn/openhab-docs that referenced this pull request Aug 15, 2022
The Web Audio sink is now also supported by Main UI and Basic UI, see:

* openhab/openhab-webui#1422
* openhab/openhab-webui#1426

Signed-off-by: Wouter Born <github@maindrain.net>
Confectrician pushed a commit to openhab/openhab-docs that referenced this pull request Aug 21, 2022
The Web Audio sink is now also supported by Main UI and Basic UI, see:

* openhab/openhab-webui#1422
* openhab/openhab-webui#1426

Signed-off-by: Wouter Born <github@maindrain.net>

Signed-off-by: Wouter Born <github@maindrain.net>
@wborn
Copy link
Member Author

wborn commented Sep 12, 2022

Maybe you can add the "enhancement" label to this PR so this feature ends up in the final release notes @openhab/webui-maintainers ?

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

Successfully merging this pull request may close these issues.

3 participants