0191-WebUI-Add-NaturalSortOrderInControlTabs #2904
Conversation
jens-maus
added
💡 enhancement-ideas
WalkthroughThe changes introduce a natural sorting feature for control tabs in the WebUI by modifying the sorting method calls across several HTML files. The Changes
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (11)
buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs.patch (2)
24-31: Consider extracting common sorting logic.The sorting implementation is identical across multiple files. Consider extracting this into a shared utility function to maintain consistency and reduce code duplication.
+ // In a shared utility file (e.g., sorting.js) + function applySortByName(viewIds, ascending, natural) { + viewIds.SortByName(ascending, natural); + firstSort = true; + }
1-31: Implementation successfully adds natural sort order across control tabs.The changes consistently implement natural sorting across device, function, and room channels, fulfilling the PR objectives. A few suggestions for improvement:
- Consider adding comments explaining the sorting parameters and their effects
- Update user documentation to describe the new sorting behavior
- Consider adding a configuration option to enable/disable natural sorting as a user preference
buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs/occu/WebUI/www/rega/pages/tabs/control/hfuncchannels.htm (3)
210-218: Use object literal notation in JavaScript for better readability.When creating new objects in JavaScript, prefer using
{}overnew Object()for simplicity and performance benefits.Modify the script generation:
- Write("chn = new Object();"); + Write("chn = {};");This change makes the code more concise and adheres to modern JavaScript practices.
260-266: Optimize jQuery selector usage for improved performance.Using
[name='hssType']as a selector can be inefficient if many elements share this attribute. Consider adding a class or using a more specific selector.Example:
- <span name='hssType' class='hidden'>... + <span class='hssType hidden'>...Update the jQuery selector accordingly:
- jQuery("[name='hssType']").each(function(index, elm) { + jQuery(".hssType").each(function(index, elm) {
153-156: Simplify conditional statements for better readability.The
if...elseblock can be refactored to reduce code repetition and enhance clarity.Refactored code:
- if( oUser && oUser.UserLevel() == iulAdmin ) { - Write("<td class='GrayBkg CLASS04007' onclick='DeviceListPage.showConfiguration(event, \"CHANNEL\", "#chn.ID()#");' onmouseover='this.className=\"DeviceListRow_Highlight\";' onmouseout='this.className=\"DeviceListRow\";'>"); - } else { - Write("<td class='GrayBkg'>"); - } + string tdClass = 'GrayBkg'; + string tdAttributes = ''; + if( oUser && oUser.UserLevel() == iulAdmin ) { + tdClass = tdClass # ' CLASS04007'; + tdAttributes = " onclick='DeviceListPage.showConfiguration(event, \"CHANNEL\", "#chn.ID()#");' onmouseover='this.className=\"DeviceListRow_Highlight\";' onmouseout='this.className=\"DeviceListRow\";'"; + } + Write("<td class='"#tdClass#"' "#tdAttributes#">");buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs/occu/WebUI/www/rega/pages/tabs/control/hroomchannels.htm (3)
19-19: Ensure natural sort order is applied consistentlyAt line 19, the
SortByNamemethod is called without specifying the natural sort order parameters. To maintain consistency with the updated sorting functionality, consider applying natural sorting here as well.Apply this change:
-oTmpArray.SortByName(); +oTmpArray.SortByName(soAsc, stNatural);
215-215: Prefer object literals overnew Object()At line 215, the code uses
chn = new Object();. It is recommended to use object literals{}for better performance and readability.Apply this change:
-chn = new Object(); +chn = {};Also, update similar instances where
new Object()is used.
31-33: Correct comment syntax for better readabilityThe comments in lines 31-33 use
!as a delimiter, which might not be standard. Replace!with the appropriate comment syntax to improve code clarity.For example:
-! This prevents that a channel linked with a system variable is visible in the room list -! The Weather-Channel e. g. is of the type VARDP. -! bIsSysVarDP = ( oTmpDP.IsTypeOf( OT_VARDP ) || oTmpDP.IsTypeOf( OT_ALARMDP ) ); +// This prevents a channel linked with a system variable from being visible in the room list +// The Weather-Channel, for example, is of the type VARDP. +// bIsSysVarDP = ( oTmpDP.IsTypeOf( OT_VARDP ) || oTmpDP.IsTypeOf( OT_ALARMDP ) );buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs/occu/WebUI/www/rega/pages/tabs/control/hdevichannels.htm (3)
1-1: Remove unnecessaryxmlnsattribute from<script>tagThe
xmlns="http://www.w3.org/1999/html"attribute in the<script>tag is unnecessary and may cause issues with script execution. Thexmlnsattribute is typically not used in<script>elements in HTML documents.
52-56: Update sort function calls to include natural sortingThe
onclickhandlers for the table headers useIseSort(arChns,'name',true);. To incorporate natural sorting, consider updating these function calls to pass the natural sort parameter, ensuring consistency with the sorting implemented inUserTempViewIDs().SortByName(soAsc,stNatural);.
284-287: Clarify or remove commented-out sorting functionThe code within the
if(firstSort)block writes a commented-out function call//IseSort(arChns,'name',true);. If this is intentional for debugging or future use, consider adding a comment explaining its purpose. Otherwise, remove it to prevent confusion.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs/occu/WebUI/www/rega/pages/tabs/control/hdevichannels.htm.origis excluded by!**/*.origbuildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs/occu/WebUI/www/rega/pages/tabs/control/hfuncchannels.htm.origis excluded by!**/*.origbuildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs/occu/WebUI/www/rega/pages/tabs/control/hroomchannels.htm.origis excluded by!**/*.orig
📒 Files selected for processing (4)
buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs.patch(1 hunks)buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs/occu/WebUI/www/rega/pages/tabs/control/hdevichannels.htm(1 hunks)buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs/occu/WebUI/www/rega/pages/tabs/control/hfuncchannels.htm(1 hunks)buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs/occu/WebUI/www/rega/pages/tabs/control/hroomchannels.htm(1 hunks)
🔇 Additional comments (5)
buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs.patch (2)
14-21: Ensure backwards compatibility of modified sort behavior.
The modification to SortByName adds parameters that might affect existing sorting behavior. Please verify that this change maintains the expected order for existing use cases.
Consider adding unit tests to verify the sorting behavior with different combinations of soAsc and stNatural parameters.
4-10: Verify initialization of sorting parameters.
The new sorting call uses soAsc and stNatural parameters, but their initialization isn't visible in the provided context. Please ensure these variables are properly defined before this code block.
buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs/occu/WebUI/www/rega/pages/tabs/control/hfuncchannels.htm (1)
19-19: Confirm the availability of soAsc and stNatural constants.
The call to SortByName(soAsc, stNatural); introduces natural sorting. Ensure that the constants soAsc and stNatural are defined in this context to prevent any runtime errors due to undefined variables.
To verify, you can run the following script:
buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs/occu/WebUI/www/rega/pages/tabs/control/hroomchannels.htm (2)
254-255: Verify existence of divHTR in the DOM
In lines 254-255, new iseSubMenuControl references "divHTR". Check that an element with the ID divHTR exists in the DOM to ensure the submenu functions correctly.
33-34: Verify the exclusion of OT_VARDP from the condition
The original condition included both OT_VARDP and OT_ALARMDP, but line 34 now only checks for OT_ALARMDP. Ensure that excluding OT_VARDP is intentional, as this might affect the visibility of certain channels linked with system variables like the Weather-Channel.
To confirm whether any channels of type OT_VARDP should be excluded, run the following script:
✅ Verification successful
Based on the search results, I can now provide a definitive response about the exclusion of OT_VARDP from the condition.
The exclusion of OT_VARDP is intentional and correct
The change to only check for OT_ALARMDP and exclude OT_VARDP is intentional and has been consistently applied across multiple files in the codebase. The commented code explains that this prevents channels linked with system variables (like the Weather-Channel) from being visible in the room list, which was the previous behavior that needed to be changed.
Key evidence:
- The same change pattern appears in multiple control view files:
- hroomchannels.htm
- hfuncchannels.htm
- devices.htm
- The comment explicitly states: "This prevents that a channel linked with a system variable is visible in the room list. The Weather-Channel e.g. is of the type VARDP."
- The change is part of a larger patch (0191-WebUI-Add-NaturalSortOrderInControlTabs) that improves the UI organization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find channels that are of type OT_VARDP linked to data points.
# Expected result: List of channels of type OT_VARDP, if any.
ast-grep --pattern $'object oTmpDP = dom.GetObject($_);\nif( oTmpDP ) {\n $$$\n bIsSysVarDP = oTmpDP.IsTypeOf( OT_VARDP );\n $$$\n}'
Length of output: 10235
Script:
#!/bin/bash
# Let's try a different approach to find references to OT_VARDP in the codebase
# First, let's check the specific file content
cat "buildroot-external/patches/occu/0191-WebUI-Add-NaturalSortOrderInControlTabs/occu/WebUI/www/rega/pages/tabs/control/hroomchannels.htm"
# Then search for other similar patterns in the codebase
rg "OT_VARDP" -A 2 -B 2
Length of output: 229099
| Call("/pages/tabs/control/function.fn::ShowExtDesc()"); | ||
|
|
||
| if (showExtDesc == false) { | ||
| Write(chn.Name()#"<br/><br><span name='hssType' class='hidden'>${chType_"#chn.HssType()#"}</span>"); |
There was a problem hiding this comment.
Escape channel names to prevent potential XSS vulnerabilities.
Displaying chn.Name() without proper escaping may lead to cross-site scripting (XSS) if channel names contain malicious input.
[security]
Update the code to escape chn.Name():
- Write(chn.Name()#"<br/><br><span name='hssType' class='hidden'>${chType_"#chn.HssType()#"}</span>");
+ Write(HtmlEncode(chn.Name())#"<br/><br><span name='hssType' class='hidden'>${chType_"#chn.HssType()#"}</span>");Ensure HtmlEncode or an equivalent function properly escapes special HTML characters.
Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| string oDevHssType = oDev.HssType(); | ||
| integer chNumber = chn.ChnNumber(); |
There was a problem hiding this comment.
Add null checks for device objects to prevent runtime errors.
When accessing oDev.HssType(), there is a risk of a null reference if oDev is null. This could cause runtime exceptions.
Implement a null check before accessing oDev properties:
- string oDevHssType = oDev.HssType();
- integer chNumber = chn.ChnNumber();
+ if (oDev) {
+ string oDevHssType = oDev.HssType();
+ integer chNumber = chn.ChnNumber();
+ } else {
+ Write("<!-- Device object is null -->");
+ }Committable suggestion skipped: line range outside the PR's diff.
| ! This prevents that a channel linked with a system variable is visible in the function list | ||
| ! The Weather-Channel e. g. is of the type VARDP. | ||
| ! bIsSysVarDP = ( oTmpDP.IsTypeOf( OT_VARDP ) || oTmpDP.IsTypeOf( OT_ALARMDP ) ); | ||
| bIsSysVarDP = oTmpDP.IsTypeOf( OT_ALARMDP ) ; |
There was a problem hiding this comment.
Re-evaluate the exclusion of OT_VARDP from system variable checks.
Previously, channels linked with system variables of types OT_VARDP and OT_ALARMDP were excluded. The updated code only checks for OT_ALARMDP, potentially including OT_VARDP channels unintentionally.
Consider restoring the original condition to exclude both types:
- bIsSysVarDP = oTmpDP.IsTypeOf( OT_ALARMDP ) ;
+ bIsSysVarDP = ( oTmpDP.IsTypeOf( OT_VARDP ) || oTmpDP.IsTypeOf( OT_ALARMDP ) );Please verify if excluding OT_VARDP is still necessary for the correct functionality.
📝 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.
| ! This prevents that a channel linked with a system variable is visible in the function list | |
| ! The Weather-Channel e. g. is of the type VARDP. | |
| ! bIsSysVarDP = ( oTmpDP.IsTypeOf( OT_VARDP ) || oTmpDP.IsTypeOf( OT_ALARMDP ) ); | |
| bIsSysVarDP = oTmpDP.IsTypeOf( OT_ALARMDP ) ; | |
| ! This prevents that a channel linked with a system variable is visible in the function list | |
| ! The Weather-Channel e. g. is of the type VARDP. | |
| ! bIsSysVarDP = ( oTmpDP.IsTypeOf( OT_VARDP ) || oTmpDP.IsTypeOf( OT_ALARMDP ) ); | |
| bIsSysVarDP = ( oTmpDP.IsTypeOf( OT_VARDP ) || oTmpDP.IsTypeOf( OT_ALARMDP ) ); |
| } | ||
| } | ||
| } | ||
| oUser.UserTempViewIDs().SortByName(soAsc,stNatural); |
There was a problem hiding this comment.
Define soAsc and stNatural before use
At line 43, oUser.UserTempViewIDs().SortByName(soAsc, stNatural); uses soAsc and stNatural, but these variables are not defined in the provided code. Please ensure they are properly defined to prevent runtime errors.
For example, you might define them as:
var soAsc = true; // or appropriate value
var stNatural = true; // or appropriate value for natural sorting| Write("if (!fltHR.objPassFilter(chn ))"); | ||
| Write("hide(chn['trid']);"); |
There was a problem hiding this comment.
Initialize fltHR before usage
The variable fltHR is used in fltHR.objPassFilter(chn), but there is no prior initialization in this file. Ensure that fltHR is defined and initialized appropriately to avoid JavaScript errors.
| jQuery("#ftName").focus(); | ||
| }); | ||
|
|
||
| jQuery("[name='attention']").data('powertip', '<div>'+translateKey("lblSeeServiceMessage")+'</div>').powerTip(); |
There was a problem hiding this comment.
Ensure content is sanitized to prevent XSS vulnerabilities
At line 260, dynamic content is added using translateKey("lblSeeServiceMessage"). Verify that the returned content is properly sanitized before insertion to prevent potential cross-site scripting (XSS) attacks.
| } | ||
| } | ||
| } | ||
| oUser.UserTempViewIDs().SortByName(soAsc,stNatural); |
There was a problem hiding this comment.
Ensure soAsc and stNatural variables are defined
The variables soAsc and stNatural used in oUser.UserTempViewIDs().SortByName(soAsc,stNatural); are not defined within this code segment. Please ensure these variables are defined or imported appropriately to prevent runtime errors.
| Write("<tr id='"#trId#"'>"); | ||
| if( oUser && oUser.UserLevel() == iulAdmin ) { | ||
| Write("<td class='GrayBkg CLASS03905' name='devName' onclick='DeviceListPage.showConfiguration(event, \"CHANNEL\", "#chn.ID()#");' onmouseover='this.className=\"DeviceListRow_Highlight\";' onmouseout='this.className=\"DeviceListRow\";'>"); | ||
| } else { | ||
| Write("<td class='GrayBkg' name='devName'>"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure proper encoding to prevent XSS vulnerabilities
When dynamically generating HTML content with user or channel names, such as in chn.Name(), ensure that the values are properly encoded or sanitized to prevent cross-site scripting (XSS) attacks.
Description
Sortiert die Listen in Status und Bedienung => Geräte / Gewerke / Räume nach Natural Sort Order (https://en.wikipedia.org/wiki/Natural_sort_order)
Related Issue
#2897
Types of changes
Alternate Designs
Possible Drawbacks
Diese Änderung benötigt ReGaHss R1.00.0388.0245+ da erst hier die notwendigen Optionen für die SortByName() Funktion umgesetzt sind.
Verification Process
Release Notes
Contributing checklist
Summary by CodeRabbit
New Features
Bug Fixes