Add horizontal swing to climate #22043
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce new features and modifications to the climate control functionality within the Home Assistant interface. Key updates include the addition of horizontal swing modes for climate entities, enhancements to the thermostat and tile cards, and the introduction of new configuration options. The changes also include updates to the relevant TypeScript types, translations, and UI components to support these new features, ensuring a cohesive integration across the system. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (3)
src/panels/lovelace/card-features/hui-climate-swing-horizontal-modes-card-feature.ts (3)
73-75: Optimize state updates by checking for changesIn the
willUpdatemethod,this._currentSwingHorizontalModeis updated wheneverstateObjchanges. However, if theswing_horizontal_modeattribute hasn't changed, it may cause unnecessary updates.Consider checking if
this._currentSwingHorizontalModediffers fromthis.stateObj.attributes.swing_horizontal_modebefore updating:if (changedProp.has("stateObj") && this.stateObj) { - this._currentSwingHorizontalMode = - this.stateObj.attributes.swing_horizontal_mode; + const newMode = this.stateObj.attributes.swing_horizontal_mode; + if (this._currentSwingHorizontalMode !== newMode) { + this._currentSwingHorizontalMode = newMode; + } }
175-179: Ensure proper event handling with bound methodsIn the render method, event listeners use functions that may not be bound to the class instance, potentially causing issues with
thiscontext.Consider using arrow functions or the
::binding syntax to ensure proper context:@selected=${this._valueChanged} @closed=${stopPropagation}Alternatively, you can bind the methods in the constructor or use the
@eventOptions({ capture: true })decorator fromlitto ensure correct context.
205-205: Simplify static styles definitionThe
stylesproperty can be defined using the static field instead of a getter method for better readability.Consider updating the styles definition:
- static get styles() { - return cardFeatureStyles; - } + static styles = cardFeatureStyles;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- gallery/src/pages/lovelace/thermostat-card.ts (2 hunks)
- gallery/src/pages/lovelace/tile-card.ts (1 hunks)
- src/data/climate.ts (2 hunks)
- src/dialogs/more-info/controls/more-info-climate.ts (3 hunks)
- src/panels/lovelace/card-features/hui-climate-swing-horizontal-modes-card-feature.ts (1 hunks)
- src/panels/lovelace/card-features/types.ts (2 hunks)
- src/panels/lovelace/create-element/create-card-feature-element.ts (1 hunks)
- src/panels/lovelace/editor/config-elements/hui-card-features-editor.ts (4 hunks)
- src/panels/lovelace/editor/config-elements/hui-climate-swing-horizontal-modes-card-feature-editor.ts (1 hunks)
- src/panels/lovelace/editor/config-elements/hui-thermostat-card-editor.ts (1 hunks)
- src/translations/en.json (1 hunks)
Additional context used
Biome
src/panels/lovelace/editor/config-elements/hui-climate-swing-horizontal-modes-card-feature-editor.ts
[error] 142-142: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (19)
src/data/climate.ts (3)
64-64: LGTM!The new optional property
swing_horizontal_modeis added correctly to theClimateEntitytype. The naming convention, type, and optional nature of the property are consistent with the existingswing_modeproperty.
65-65: LGTM!The new optional property
swing_horizontal_modesis added correctly to theClimateEntitytype. The naming convention, type, and optional nature of the property are consistent with the existingswing_modesproperty.
80-80: LGTM!The new enum value
SWING_HORIZONTAL_MODEis added correctly to theClimateEntityFeatureenum. The naming convention and value assignment are consistent with the existing enum values.src/panels/lovelace/create-element/create-card-feature-element.ts (1)
37-37: LGTM!The addition of the new type
"climate-swing-horizontal-modes"to theTYPESset is a valid change that expands the available types for Lovelace card features. This change enables the creation of card features specifically for controlling horizontal swing modes in climate entities, which aligns with the PR objective of introducing horizontal swing functionality.The change follows the existing pattern of defining valid types in the
TYPESset and appears to be self-contained within this file without introducing any apparent issues.src/panels/lovelace/card-features/types.ts (2)
64-68: LGTM!The new interface
ClimateSwingHorizontalModesCardFeatureConfigis well-defined and follows the existing conventions in this file. The property names and types are appropriate, and the optional properties provide flexibility in configuration.
148-148: Looks good!The
LovelaceCardFeatureConfigtype is correctly updated to include the newClimateSwingHorizontalModesCardFeatureConfiginterface. This makes it a valid configuration option for Lovelace card features.src/panels/lovelace/editor/config-elements/hui-thermostat-card-editor.ts (1)
38-38: LGTM!The addition of the
"climate-swing-horizontal-modes"feature type to theCOMPATIBLE_FEATURES_TYPESarray is a straightforward and consistent change that enhances the capabilities of the thermostat card editor. It enables support for horizontal swing modes in climate control settings without introducing any breaking changes or regressions.gallery/src/pages/lovelace/tile-card.ts (3)
80-80: LGTM!The addition of the
switch_horizontal_modesproperty to theENTITIESconstant is a valid change. It introduces an array of string values representing different horizontal swing modes for the climate entity.
87-87: Looks good!The introduction of the
swing_horizontal_modeproperty in theENTITIESconstant is a sensible addition. It allows tracking the currently active horizontal swing mode for the climate entity, with an initial value of"off".
92-92: Approved.Updating the
supported_featuresproperty to includeClimateEntityFeature.SWING_HORIZONTAL_MODEis a necessary change. It correctly reflects that the climate entity now supports horizontal swing functionality, aligning with the addition of theswitch_horizontal_modesandswing_horizontal_modeproperties.src/panels/lovelace/editor/config-elements/hui-card-features-editor.ts (4)
27-27: LGTM!The import statement is correctly added to include the new feature.
60-60: Looks good!The
UI_FEATURE_TYPESarray is correctly updated to include the new feature type.
91-91: Approved.The
EDITABLES_FEATURE_TYPESset is correctly updated to include the new feature type.
109-110: Looks good to me!The
SUPPORTS_FEATURE_TYPESrecord is correctly updated to map the new feature type to its corresponding support function.src/dialogs/more-info/controls/more-info-climate.ts (3)
83-86: LGTM!The constant is correctly checking for the
swing_horizontal_modefeature support and follows the naming convention.
357-409: LGTM!The conditional block for displaying the
swing_horizontal_modeselection menu is implemented correctly. It checks for the necessary conditions, populates the menu with available modes, and disables the menu when appropriate. The code structure and logic is consistent with other similar selection menus in the file.
446-454: LGTM!The
_handleSwingHorizontalmodeChangedfunction is implemented correctly. It retrieves the new value from the event and calls the service helper with the appropriate arguments to update theswing_horizontal_mode. The function logic is consistent with other similar handler functions in the file.src/translations/en.json (1)
6215-6224: LGTM!The new "climate-swing-horizontal-modes" section follows the existing structure and naming conventions of the file. The added keys and values seem appropriate and consistent.
gallery/src/pages/lovelace/thermostat-card.ts (1)
88-92: Ensure 'supported_features' value is correctThe
supported_featuresvalue has been updated to553. This value should represent the sum of all supported feature flags for the entity. Please verify that553accurately reflects the combined supported features, including the new horizontal swing capabilities.To verify the calculation of
supported_features, run the following script:If the output matches
553, thesupported_featuresvalue is correct.Verification successful
To ensure we have a comprehensive understanding of the supported features, let's search the codebase for any additional climate-related constants or enums that might be relevant to this calculation.
Confirmed: 'supported_features' value is correct
The
supported_featuresvalue of 553 in the code snippet accurately reflects the sum of the relevant climate entity feature flags. This value corresponds to the following supported features:
- TARGET_TEMPERATURE (1)
- FAN_MODE (8)
- SWING_MODE (32)
- SWING_HORIZONTAL_MODE (512)
These features are defined in the
ClimateEntityFeatureenum insrc/data/climate.ts, and their sum (1 + 8 + 32 + 512) equals 553, matching the value in the code.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that 'supported_features' value correctly sums the feature flags. # Climate feature flags: # SUPPORT_TARGET_TEMPERATURE = 1 # SUPPORT_FAN_MODE = 8 # SUPPORT_SWING_MODE = 32 # SUPPORT_SWING_MODE_HORIZONTAL = 512 # Sum the features supported by the entity: echo $((1 + 8 + 32 + 512)) # Expected output: 553Length of output: 14
Script:
#!/bin/bash # Search for climate-related constants or enums in the codebase echo "Searching for climate-related constants:" rg -i "const.*climate" -g "*.ts" -A 5 echo "\nSearching for climate-related enums:" rg -i "enum.*climate" -g "*.ts" -A 10 echo "\nSearching for SUPPORT_ constants related to climate:" rg "const SUPPORT_.*=.*;" -g "*.ts" | grep -i "climate"Length of output: 11311
| swing_modes: ["both", "rangefull", "off"], | ||
| swing_mode: "rangefull", | ||
| swing_horizontal_modes: ["both", "rangefull", "off"], | ||
| swing_horizontal_mode: "both", | ||
| supported_features: 553, |
There was a problem hiding this comment.
Verify the validity of 'rangefull' as a swing mode option
It appears that 'rangefull' is used as a swing mode option in both swing_modes and swing_horizontal_modes. Please confirm that 'rangefull' is a valid and recognized swing mode value for the Sensibo device. If it's a typo and should be 'range_full' or another standard value, consider correcting it to ensure compatibility.
Apply this diff if correction is needed:
swing_modes: ["both", "rangefull", "off"],
swing_mode: "rangefull",
swing_horizontal_modes: ["both", "rangefull", "off"],
swing_horizontal_mode: "both",
supported_features: 553, swing_modes: ["both", "range_full", "off"],
swing_mode: "range_full",
swing_horizontal_modes: ["both", "range_full", "off"],
swing_horizontal_mode: "both",
supported_features: 553,Committable suggestion was skipped due to low confidence.
| stateObj?.attributes.swing_horizontal_modes || []; | ||
| } | ||
| if (!customize_modes && config.swing_horizontal_modes) { | ||
| delete config.swing_horizontal_modes; |
There was a problem hiding this comment.
Avoid using the delete operator for better performance
Using the delete operator can impact performance. Consider setting the property to undefined instead.
Apply this diff to fix the issue:
- delete config.swing_horizontal_modes;
+ config.swing_horizontal_modes = undefined;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| delete config.swing_horizontal_modes; | |
| config.swing_horizontal_modes = undefined; |
Tools
Biome
[error] 142-142: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
|
|
||
| private async _valueChanged(ev: CustomEvent) { | ||
| const swingHorizontalMode = | ||
| (ev.detail as any).value ?? ((ev.target as any).value as string); |
There was a problem hiding this comment.
Enhance type safety by defining the event detail type
Currently, the code uses type assertions with as any to access ev.detail.value and ev.target.value. This can be improved by defining the event type to enhance type safety and avoid potential runtime errors.
Consider defining a custom event interface and using it to properly type the event parameter. For example:
+ interface ValueChangedEvent extends CustomEvent {
+ detail: { value: string };
+ }
- private async _valueChanged(ev: CustomEvent) {
+ private async _valueChanged(ev: ValueChangedEvent) {
const swingHorizontalMode =
- (ev.detail as any).value ?? ((ev.target as any).value as string);
+ ev.detail.value;Committable suggestion was skipped due to low confidence.
| this._currentSwingHorizontalMode = oldSwingHorizontalMode; | ||
| } |
There was a problem hiding this comment.
Provide user feedback on failed service call
In the _valueChanged method, if the service call fails, the user may not be aware that their action did not succeed. Providing user feedback in case of an error would enhance user experience.
Consider adding a notification or error message to inform the user:
try {
await this._setMode(swingHorizontalMode);
} catch (err) {
this._currentSwingHorizontalMode = oldSwingHorizontalMode;
+ showToast(this, {
+ message: this.hass!.localize(
+ "ui.dialogs.more_info_control.climate.swing_mode_set_failed"
+ ),
+ });
}Make sure to import the necessary showToast function and define the appropriate localization string.
Committable suggestion was skipped due to low confidence.
|
When we introduce Horizontal swing mode, does that make the other one Vertical swing mode? To make the label shorter, we could drop the word mode: |
Yes
I think that makes sense. Would it make sense to update |
I like that, I don't see any concerns if we drop mode. This also gives us one option with Mode: |
gjohansson-ST
force-pushed
the
add-horizontal-swing
branch
from
e3ed43a
to
39d2b7d
Compare
I have introduced it with this PR but maybe should be done in a preliminary PR before introduced here. |
Proposed change
Add horizontal swing mode to
ClimateEntityCore PR: home-assistant/core#125578
Example printscreens
With both
swingandhorizontal_swingit becomes vertical and horizontal.If only
swingit remains as swingType of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation