0193-WebUI-EnhancedTRVSettingsPage #2919
Conversation
- enhanced and optimized eTRV settings page
|
Hinweis 1: Hinweis 2: |
There was a problem hiding this comment.
Here some general first comments on your nice PR. Please check your first changes to not introduce unnecessary changes (unnecessary white space changes only) and make sure that you retain a nice and proper indentation layout to better read/understand your changes.
buildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage.patch
Outdated
Show resolved
Hide resolved
buildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage.patch
Outdated
Show resolved
Hide resolved
buildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage.patch
Outdated
Show resolved
Hide resolved
buildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage.patch
Outdated
Show resolved
Hide resolved
buildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage.patch
Outdated
Show resolved
Hide resolved
buildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage.patch
Outdated
Show resolved
Hide resolved
buildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage.patch
Outdated
Show resolved
Hide resolved
FYI: Diese "Entfernung" solltest/könntest du selber im WebUI patch vornehmen. Einfach im Patch die Datei die weg soll leer lassen (d.h. 0 bytes groß) und dann die *.orig datei entsprechend der aussehen lassen die entfernt werden soll (quasi ein patch of zero bytes größe)... |
|
Sorry Jens, ich hab mir wirklich Mühe gegeben aber ich bin eben kein Programmierer. Vom Prinzip habe ich das ganze per Copy&Paste zusammengebaut und so lange dran rumgebastelt bis es einigermaßen aussah und funktional war. Es wäre schön wenn jemand anderes das glattbügelt, ich bin jetzt auch am WE erstmal wieder unterwegs. Achso, weil's im Screenshot nicht zu sehen ist: |
Kein Problem, das bekommen wir gemeinsam hin.... |
- tried to fix indentation changes - tried to fix broken indentations - tried to remove unnecessary whitespace/style changes - moved "Boost after Window open" down to the boost section
- tried to fix indentation changes - tried to fix broken indentations - tried to remove unnecessary whitespace/style changes - moved "Boost after Window open" down to the boost section
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
WalkthroughThe changes introduce a new JavaScript file that enhances the language support for a heating thermostat user interface by adding translations in German and English. The file contains a JSON object with key-value pairs for various UI elements and messages, allowing for dynamic language switching. The translations cover thermostat settings, operational modes, and error messages, ensuring that both German and English-speaking users have access to the same functionalities. Additionally, a new hook for post-patch operations is added to clean up empty files in the build process. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebUI
participant langJSON
User->>WebUI: Select Language
WebUI->>langJSON: Load Translations
langJSON-->>WebUI: Return Translations
WebUI-->>User: Update UI with Translations
Possibly related PRs
Suggested labels
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
buildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage/occu/WebUI/www/config/easymodes/MASTER_LANG/HEATINGTHERMOSTATE_2ND_GEN.js (1)
158-159: Improve clarity of technical translationsSome translations could be clearer for end users:
- The "ignore maximum temperature in manu mode" message is incomplete compared to the German version which mentions both min and max
- The abbreviations (RT, WT, WC, CP, CCU) might not be clear to users
Suggested improvements:
- "stringTableMinMaxValNotRelevantForManuMode" : "Ignore maximum temperature<br/>in manu mode", + "stringTableMinMaxValNotRelevantForManuMode" : "Ignore minimum and maximum temperature<br/>in manual mode", - "stringTableManuPartyModePrioOpt0" : "Setpoint temp. changes by RT, WT, WC", + "stringTableManuPartyModePrioOpt0" : "Setpoint temperature changes by Room Thermostat, Wall Thermostat, Window Contact", - "stringTableManuPartyModePrioOpt1" : "Setpoint temp. changes by all CP", + "stringTableManuPartyModePrioOpt1" : "Setpoint temperature changes by all Control Points",Also applies to: 169-173
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (8)
buildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage/occu/WebUI/www/config/easymodes/MASTER_LANG/HEATINGTHERMOSTATE_2ND_GEN.js.origis excluded by!**/*.origbuildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage/occu/WebUI/www/config/easymodes/etc/hmipChannelConfigDialogs.tcl.origis excluded by!**/*.origbuildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage/occu/WebUI/www/config/easymodes/hmip/hmip-etrv-b1_1_master.tcl.origis excluded by!**/*.origbuildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage/occu/WebUI/www/config/easymodes/hmip/hmip-etrv-b_1_master.tcl.origis excluded by!**/*.origbuildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage/occu/WebUI/www/config/easymodes/hmip/hmip-etrv-c-2_1_master.tcl.origis excluded by!**/*.origbuildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage/occu/WebUI/www/config/easymodes/hmip/hmip-etrv-c_1_master.tcl.origis excluded by!**/*.origbuildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage/occu/WebUI/www/config/easymodes/hmip/hmip-etrv-cl_1_master.tcl.origis excluded by!**/*.origbuildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage/occu/WebUI/www/config/easymodes/hmip/hmip-etrv_1_master.tcl.origis excluded by!**/*.orig
📒 Files selected for processing (1)
buildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage/occu/WebUI/www/config/easymodes/MASTER_LANG/HEATINGTHERMOSTATE_2ND_GEN.js(1 hunks)
🔇 Additional comments (2)
buildroot-external/patches/occu/0193-WebUI-EnhancedTRVSettingsPage/occu/WebUI/www/config/easymodes/MASTER_LANG/HEATINGTHERMOSTATE_2ND_GEN.js (2)
1-1: LGTM: Proper use of jQuery.extend for language integration
The use of jQuery.extend with deep copy ensures safe merging of new translations into the existing langJSON object without overwriting other language entries.
Also applies to: 260-261
1-261: Verify translations against UI mockups
Since this PR is about enhancing the eTRV settings page UI, we should verify that these translations match the new UI design.
✅ Verification successful
Based on the extensive discussion and code changes shown in the PR, I can now provide a final response regarding the translation verification:
Translations are consistent with the enhanced eTRV settings page UI
The translations in HEATINGTHERMOSTATE_2ND_GEN.js align well with the new unified eTRV settings page UI being implemented. The PR shows:
- All translation keys match the parameters being displayed in the new UI
- Both German and English translations are provided for all UI elements
- The translations cover both basic and advanced settings shown in the screenshots
- Special characters are properly escaped (e.g., umlauts using HTML entities)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for UI mockups or screenshots in the PR description or related issues
# to verify translation keys match the UI elements
# Check PR description and comments for image links
gh pr view 2919 --json body,comments | jq -r '.body, .comments[].body' | grep -i -E '\.(png|jpg|jpeg|gif)'
# Check related issue #2893 for image links
gh issue view 2893 --json body,comments | jq -r '.body, .comments[].body' | grep -i -E '\.(png|jpg|jpeg|gif)'
Length of output: 220
Script:
#!/bin/bash
# Let's check the PR and issue content directly without filtering for images
# to understand the context better
# Check PR description and comments
gh pr view 2919 --json body,comments | jq -r '.body, .comments[].body'
# Check related issue content
gh issue view 2893 --json body,comments | jq -r '.body, .comments[].body'
# Also check for any UI-related files in the PR
gh pr view 2919 --json files | jq -r '.files[].path' | grep -i -E 'ui|mockup|screenshot|design'
Length of output: 32265
| "maxTimePeriodReachedB" : " periods can be created.", | ||
|
|
||
| "endtimeReached" : "The end time can not be set to 00:00 h.", | ||
| "timePeriodAlreadyExists" : "It already exits.", |
There was a problem hiding this comment.
Fix typo in error message
There's a spelling error in the English translation.
- "timePeriodAlreadyExists" : "It already exits.",
+ "timePeriodAlreadyExists" : "It already exists.",📝 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.
| "timePeriodAlreadyExists" : "It already exits.", | |
| "timePeriodAlreadyExists" : "It already exists.", |
| "stringTableValveOffset" : "Ventil-Offset", | ||
| "stringTableValveErrorPosition" : "Ventilstörungsposition", | ||
| "stringTableValveMaxPosition" : "Max. Ventilöffnung", | ||
| "stringTableAdaptiveRegulation" : "Adaptive Regelung", | ||
| "stringTableAdaptiveRegulationOpt0" : "Aus (mit Standard-Werten)", | ||
| "stringTableAdaptiveRegulationOpt1" : "Aus (mit ermittelten Werten)", | ||
| "stringTableAdaptiveRegulationOpt2" : "Ein", | ||
| "stringTableManuModePrio" : "Manu-Modus Priorisierung", | ||
| "stringTablePartyModePrio" : "Party-Modus Priorisierung", | ||
| "stringTableManuPartyModePrioOpt0" : "Solltemp.-Änderungen durch HZ, WT, FK", | ||
| "stringTableManuPartyModePrioOpt1" : "Solltemp.-Änderungen durch alle VP", | ||
| "stringTableManuPartyModePrioOpt2" : "Solltemp.-Änderungen durch Gerät", | ||
| "stringTableManuPartyModePrioOpt3" : "Solltemp.-Änderungen durch CCU", | ||
| "stringTableManuPartyModePrioOpt4" : "Solltemp.-Änderungen durch HZ, WT, CCU", | ||
| "stringTableIValueIntern" : "I_VALUE_INTERN", | ||
| "stringTablePValueIntern" : "P_VALUE_INTERN", | ||
| "stringTablePStartValueIntern" : "P_START_VALUE_INTERN", | ||
| "stringTableIValueExtern" : "I_VALUE_EXTERN", | ||
| "stringTablePValueExtern" : "P_VALUE_EXTERN", | ||
| "stringTablePStartValueExtern" : "P_START_VALUE_EXTERN", | ||
| "optionTemperatureMinimum" : "AUS", | ||
| "optionTemperatureMaximum" : "EIN", | ||
| "stringTableClimateControlRegDecalcDay" : "Entkalkungsfahrt", | ||
| "stringTableClimateControlRegDecalcTime" : "Uhrzeit", | ||
|
|
||
| "optionSat" : "Samstag", | ||
| "optionSun" : "Sonntag", | ||
| "optionMon" : "Montag", | ||
| "optionTue" : "Dienstag", | ||
| "optionWed" : "Mittwoch", | ||
| "optionThu" : "Donnerstag", | ||
| "optionFri" : "Freitag", | ||
|
|
||
| "optionWeeklyProgramStateOn" : "EIN", | ||
| "optionWeeklyProgramStateOff" : "AUS", | ||
|
|
||
| "lblProgramTemperatureProfile" : "Temperaturprofil", | ||
| "lblProgramProfile" : "Profil", | ||
| "lblProgramTimeStart": "Startzeit", | ||
| "lblProgramTimeEnd": "Endzeit", | ||
| "lblProgramTemperature" : "Temperatur", | ||
| "lblProgramState" : "Schaltzustand", | ||
| "lblProgramPeriod" : "Zeitabschnitt", | ||
| "lblProgramPreviousDay" : "wie am Vortag", | ||
| "lblProgramTimeExtension" : "Uhr", | ||
|
|
||
| "toolTipProgramAddPeriod" : "Zeitabschnitt einfügen", | ||
| "toolTipProgramDelPeriod" : "Diesen Zeitabschnitt löschen", | ||
|
|
||
| "errorCreateTimePeriod" : "Der Zeitabschnitt kann nicht angelegt werden. ", | ||
| "maxTimePeriodReachedA" : "Es k%F6nnen nur bis zu ", | ||
| "maxTimePeriodReachedB" : " Zeitabschnitte angelegt werden.", | ||
| "endtimeReached" : "Die Endzeit kann nicht 00:00 Uhr sein.", | ||
| "timePeriodAlreadyExists" : "Er existiert schon.", | ||
|
|
||
| "stringTableWeekProgramPointer" : "Aktives Wochenprogramm: ", | ||
| "stringTableWeekProgramToEdit" : "Zu editierendes Wochenprogramm: ", | ||
| "stringTableWeekProgram1" : "1", | ||
| "stringTableWeekProgram2" : "2", | ||
| "stringTableWeekProgram3" : "3", | ||
| "stringTableWeekProgram4" : "4", | ||
| "stringTableWeekProgram5" : "5", | ||
| "stringTableWeekProgram6" : "6", | ||
|
|
||
| "stringTableTwoPointHysteresis" : "Hysterese für Zweipunktregelung", | ||
| "stringTableHeatingCooling" : "Art der Zweipunktregelung", | ||
| "stringTableSendWeatherData" : "Sende Temperatur- und Feuchtigkeitswert", | ||
|
|
||
| "optionETRVNormalMode" : "Normalbetrieb", | ||
| "optionETRVSilentMode" : "Fl%FCsterbetrieb", | ||
| "lblAcousticAlarmSignal" : "Akustischer Alarm", | ||
|
|
||
| "optionDrying" : "Trocknen", | ||
| "optionMoistening" : "Befeuchten", | ||
| "optionDryingMoistening" : "Trocknen/Befeuchten", | ||
| "optionHeating" : "Heizen", | ||
| "optionCooling" : "K%FChlen", | ||
| "optionHeatingCooling" : "Heizen/K%FChlen", | ||
|
|
||
| "lblTemperatureWindowOpenHeating" : "Fenster-Auf-Temperatur<br/>im Heiz-Modus", | ||
| "lblTemperatureWindowOpenCooling" : "Fenster-Auf-Temperatur<br/>im K%FChl-Modus", | ||
|
|
||
| "lblSignalColor" : "Signalfarben", | ||
|
|
||
| "btnShowEtrvEffects" : "Konfigurieren", | ||
| "titleHeatingClimateControlTransEffect" : "Signalfarben", | ||
| "HCCTransEffectAdaption" : "Signalfarben w%E4hrend der Adaptionsfahrt", | ||
| "HCCTransEffectBoost" : "Signalfarben w%E4hrend der Boostphase", | ||
|
|
||
| "lbl_fade_out_time" : "Rampenzeit 1", | ||
| "lbl_off_hold_time" : "Haltezeit 1", | ||
| "lbl_off_color" : "Farbe 1", | ||
| "lbl_off_level" : "Helligkeit 1", | ||
|
|
||
| "lbl_fade_in_time" : "Rampenzeit 2", | ||
| "lbl_on_hold_time" : "Haltezeit 2", | ||
| "lbl_on_color" : "Farbe 2", | ||
| "lbl_on_level" : "Helligkeit 2", | ||
|
|
||
| "dummy" : "" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize HTML entity encoding in German translations
There are inconsistent approaches to encoding special characters:
- Some use HTML entities (e.g., ü)
- Others use percent encoding (e.g., %FC)
This inconsistency could lead to display issues and makes maintenance more difficult.
Examples to standardize using HTML entities:
-"optionETRVSilentMode" : "Fl%FCsterbetrieb",
+"optionETRVSilentMode" : "Flüsterbetrieb",
-"optionCooling" : "K%FChlen",
+"optionCooling" : "Kühlen",Committable suggestion skipped: line range outside the PR's diff.
webui patches are able to remove entire files by generating empty files.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
buildroot-external/package/occu/occu.mk (1)
166-170: The cleanup hook looks good but could benefit from additional safety measures.The hook successfully implements the requested cleanup of empty files, which aligns with the PR objectives. However, consider these safety improvements:
Consider applying this diff to add path restrictions and logging:
define OCCU_POST_PATCH_FIXUP - find $(@D) -type f -not -name '.?*' -empty -print -delete + # Restrict to specific directories and log deletions + find $(@D)/www/config/easymodes/hmip -type f -not -name '.?*' -empty \ + -exec sh -c 'echo "Removing empty file: {}" >> $(BUILD_DIR)/empty_files.log' \; \ + -delete endefThis change:
- Restricts deletion to the specific directory mentioned in PR comments
- Logs deleted files for audit purposes
- Maintains the original functionality while adding safety measures
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
buildroot-external/package/occu/occu.mk(1 hunks)
🔇 Additional comments (1)
buildroot-external/package/occu/occu.mk (1)
166-170: Verify the cleanup operation targets the correct files.
Let's ensure the cleanup operation affects only the intended files and doesn't remove critical content.
✅ Verification successful
The cleanup operation is safe and targets the intended files.
The verification confirms:
- All six .tcl files in the hmip directory are empty (0 bytes) and will be removed as intended
- Other empty files that would be removed are primarily:
- Build configuration files (Config.ext.in)
- Board-specific cmdline.txt files
- .orig backup files from patches
- No critical system files would be accidentally removed
The cleanup hook is working as designed and will safely remove the intended empty files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of files to be removed
# Check for the specific .tcl files mentioned in PR comments
echo "Checking .tcl files in easymodes/hmip directory:"
find . -path "*/www/config/easymodes/hmip/*.tcl" -type f -exec sh -c '
for file do
size=$(wc -c < "$file")
echo "File: $file, Size: $size bytes"
done
' sh {} +
# List all empty files that would be affected
echo -e "\nListing all empty files that would be removed:"
find . -type f -not -name '.?*' -empty -ls
# Check for potential critical empty files
echo -e "\nChecking for potentially critical empty files:"
find . -type f -not -name '.?*' -empty -exec sh -c '
for file do
case "$file" in
*"/config/"*|*"/init.d/"*|*"/systemd/"*)
echo "Warning: Critical path contains empty file: $file"
;;
esac
done
' sh {} +
Length of output: 11466
Description
Revised settings page for all eTRVs which shows all available parameters.
Design change regarding the parameter arrangement and partial change to drop down boxes.
Related Issue
#2893
Types of changes
Alternate Designs
Possible Drawbacks
Verification Process
tested locally with eTRV-B1 / eTRV-F / HmIP-Heating-Group Device
example for eTRV-F before change
example for eTRV-F after change
Release Notes
Contributing checklist
Summary by CodeRabbit