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

Add configurable show/hide of TV audio input, as well as other audio inputs from favourites list. Updated ReadMe and config.schema #219

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rfiorentino1
Copy link

This pull request closes issue #206, and adds/updates the following:

  • added ability to customize which zones have TV audio shown/enabled in Homekit. Zone names are updated dynamically every time homeBridge restarts, but previous config settings will persist in config.json if zones are the same.
  • added config option to hide other audio inputs from favorites list/Homekit inputs
  • added note about excluding airplay (as I discovered the hard way, luckily in a test environment)
  • updated maxFavourites in config.schema to be accessible for screen readers (slider was showing value of 56 by default, an changing it did not effect the config)
  • updated readMe to include the above, as well as fixing a few screen reader accessibility issues, and clarifying the Group logic of ZP.
  • added a Contents to top of readMe.

Of course, completely open to suggestions etc, just trying to help :) Everything works normally for me, but feel free to test/varify. Only thing about hiding input is that it seems to take a while to switch to the next shown input and skip over them, but haven't looked into why yet. Honestly doesn't really bother me but something I noticed. I know this is a lot of code to read changes through, and for that I apologize.

Note: all in-page links/section links in contents were previously broken locally, now fixed. However, they still don't work for me on Github. Not sure if github broke something in their UI render etc but everything looks correct now on my end. Do they work for you?

…xed max-favorites value accessibility in config-schema

- Added option to hide airplay, Line-in, and TV sources from the TV inputs in HomeKit upon startup, subsequently taking them out of the favorites list when cycling through inputs. Eventually I want to separate the TV audio out of this, and make dynamic checkboxes (maybe under advanced) for zones which actually have connected TVs, so that only those zones will have TV audio enabled. But for now this is just a catch all, when I tried to implement the dynamic TV stuff things kept breaking ;) I'll keep playing with it but wanted to start incrementally.

- fixed MaxFavourites to be a text field that is visible to the VoiceOver screenreader on Mac OS. It was previously a slider that the screenreader saw as being set to 56 and if you changed the value, nothing actually changed in the config.

- made force S2 a checkbox instead of an edit field.

- added note about excluding airplay (as I discovered the hard way, luckily in a test environment)

-  Please feel free to test with different variables etc, but everything works normally for me.
- added ability to customize which zones have TV audio shown/enabled in Homekit. Zone names are updated dynamically every time homeBridge restarts, but previous config settings will persist in config.json if zones are the same.
- added config option to hide other audio inputs from favorites list/Homekit inputs
- added note about excluding airplay (as I discovered the hard way, luckily in a test environment)
- updated maxFavourites in config.schema to be accessible for screen readers (slider was showing value of 56 by default, an changing it did not effect the config)
- updated readMe to include the above, as well as fixing a few screen reader accessibility issues, and clarifying the Group logic of ZP.
- added a Contents to top of readMe.
@ebaauw ebaauw self-requested a review June 9, 2024 12:38
Copy link
Owner

@ebaauw ebaauw left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. I do appreciate the textual changes to the README; I am not a native speaker and might use funny ways to express myself.

Unfortunately, I cannot merge the PR like this; please change the following:

  • Stick to the standard layout and two-space indentation;
  • Don't (try to) write config.schema.json, as it is supposed to be read-only to the plugin.

As mentioned in #206, I want to persist the visibility of the TV inputs in cachedAccessoires. Unfortunately, I have yet to find the time to do so. When needed, I'm also happy to add some additional characteristics to enable HDMI input, etc.

* [Caveats](#caveats)


<a id="prerequisites"></a>
Copy link
Owner

Choose a reason for hiding this comment

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

I think markdown automatically creates link targets for chapter headings (###). At least that works in the Wiki sections of GitHub. Not sure why the links don't work when viewing on GitHub - I need to investigate.

`brightness` | `false` | Flag whether to expose volume as `Brightness` when `service` is `"switch"` or `"speaker"`. Setting this flag enables volume control from Siri, but not from Apple's Home app.
`excludeAirPlay` | `false` | Flag whether not to expose zone players that support Airplay, since they natively show up in Apple's Home app.
`brightness` | `false` | Flag whether to expose volume as Brightness when "service" is "switch" or "speaker". Setting this flag enables volume control from Siri, but not from Apple's Home app.
`excludeAirPlay` | `false` | Flag whether not to expose zone players that support Airplay, since they natively show up in Apple's Home app. Note that if you only have an S2 system, enabling this option will essentially render the plugin unusable, as all zones will be hidden from Homekit.
Copy link
Owner

Choose a reason for hiding this comment

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

That is technically not correct; there are still plenty of older zoneplayers supported by S2 that don't support Airplay (like the Play:1 and Play:3).

@@ -23,9 +23,20 @@
},
"excludeAirPlay": {
"title": "Exclude AirPlay 2",
"description": "Exclude AirPlay 2 zone players that are already exposed to Apple's Home app.",
"description": "Exclude AirPlay 2 zone players that are already exposed to Apple's Home app. (Please Note: If your system is S2 only, enabling this option will remove all zones from Homebridge. Use with caution.)",
Copy link
Owner

Choose a reason for hiding this comment

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

Technically not true, see comment in README.

"type": "boolean"
},
"filterFavourites": {
Copy link
Owner

Choose a reason for hiding this comment

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

That makes sense to me.

"type": "boolean",
"default": false
},
"forceS2": {
Copy link
Owner

Choose a reason for hiding this comment

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

Damn, that was missing. Good catch!

const ZpListener = require('./ZpListener')

// Constructor for ZpPlatform. Called by homebridge on load time.
const fs = require('fs');
Copy link
Owner

Choose a reason for hiding this comment

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

Please stick to the standard layout.

// Parse config.json into this.config.
parseConfigJson (configJson) {
parseConfigJson(configJson) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please stick to the standard layout.

tvIdPrefix: 'TV',
SpeakerService: this.Services.hap.Switch,
VolumeCharacteristic: this.Characteristics.hap.Volume,
filterFavourites: false, // Default value
Copy link
Owner

Choose a reason for hiding this comment

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

Flags that default to false don't need to be initialised, as the check fails for undefined.

VolumeCharacteristic: this.Characteristics.hap.Volume
}
const optionParser = new homebridgeLib.OptionParser(this.config, true)
maxFavourites: 96,
Copy link
Owner

Choose a reason for hiding this comment

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

Please stick to indentation of two spaces.



// Update the config schema with dynamic zone checkboxes
async updateConfigSchema() {
Copy link
Owner

Choose a reason for hiding this comment

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

Is this updating (trying to update) config.scheme.json in the directory where Homebridge ZP is installed? That's a no-go. This directory is supposed to be read-only to the process running Homebridge ZP.

ebaauw added a commit that referenced this pull request Jun 16, 2024
ebaauw added a commit that referenced this pull request Jun 16, 2024
Changes from #219.
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