Fixes, Changes & Improvements After Review#384
Fixes, Changes & Improvements After Review#384ExtremeFiretop merged 10 commits intoExtremeFiretop:WebFunfrom
Conversation
Quite a few changes have been made so that future reviews are easier. The Linter tool report had over 800 warnings combined (ASP & shell files) so a lot of changes were made to reformat the code to conform to coding standards.
|
Due to all the significant changes made to the way some things were done before, I would strongly recommend removing all traces of the previous "installation" of the WebGUI page (entries in the "/jffs/addons/custom_settings.txt", "/tmp/menuTree.js", "service-event", "services-start" files) so that you start with a "fresh & clean" installation. Here are some screenshots: |
Adjusted the widths of 1st table to account for the case where the long string for "ProductID + ModelID" is shown.
Does this address the situation when someone has the shell script loaded and is playing with settings in the WebUI at the same time? That was the last issue I still had to address that was on my to-do list hahaha |
Will do! I can uninstall the old version and then test with a clean install ;) |
Yes, that was the intended goal. One can have the SSH CLI menu open, while the WebGUI page is also running, and then select actions to perform. But, of course, they do not actively keep polling the configuration file so any changes made from one UI will not be reflected on the other immediately; you'll have to "refresh" the UI in order to re-read the modified configuration file. But this is the case for every add-on that is available out there; it would not be a unique limitation. |
|
I just submitted some simple changes to adjust the width to account for the possible long string of "ProductID / ModelID" in the rare cases where they happen to be different. Here is a "fake example" just to illustrate the case. |
For fun, I just tried to replace the .asp file and Fresh install is a STRONG recommended coming from my previous 1.4.0 After clearing everything from the old 1.4.0 and reinstalling the production 1.3.9 and upgrading to this gave me the expected result. Will start testing this evening! |
Might you consider adding additional menu options in shell script for both the main menu and the Advanced Options Menu to perform a user initiated menu refresh (i.e., reload configuration file on demand)? Skynet currently does this as "[r] --> Reload Menu". |
Some code improvements & fine-tuning.
I like the general idea but not as an explicit menu option duplicated in multiple places. |
|
I just submitted a new commit with improvements to the script-locking mechanism. Please use this latest code for testing & validation. |
Yes, that would be a great way to implement it! I didn't like the duplicated menu options either, but without a way to initiate a reload from either menu, one would need to exit completely out of the script — your solution avoids that. |
OK, it's on my to-do list :>). |
Testing with this latest commit (most likely not caused by commit), I discovered an inconsistency between how reloading settings do or do not happen when switching between main menu and Advanced Options Menu (or reverse) in shell script — toggling a checkbox is handled differently than chnaging value in text box. Scenario:
This scenario appears to be consistent when toggling checkboxes or changing values on both menus. Suggested Solution: Force a reload of configuration file whenever switching from main menu to Advanced Options Menu (or vice versa). This would be the same action as your proposed "user-initiated reload of the configuration file" using a known key combination. |
Fantastic! Thank you Martinski! I have the latest version and can see the adjusted widths now :) |
Got it! :) |
Wow guys! I take time off and show up to work being done, I love this community sometimes!
I can replicate this behavior as you describe it, will investigate a bit now. |
Seems to be the issue is that these values are only loaded once when the script is loaded currently and not when the menu is reloaded. (Such as the F/W Update Check setting) The current lines at 1592 and 1593 Might want to be moved or replicated within the menu loop I'm assuming |
Minor fix to the color for the "Changelog Approval" value when set to DISABLED.
Added/modified code to be able to fully "refresh" the configuration settings shown in the CLI "Main Menu" and the "Advanced Options" menu on demand as indicated by the user by typing the following key combination: <Ctrl>R + <Ctrl>L followed by the <Enter> key.
|
I just made a commit that addresses the issue reported by @TheS1R and also handles the "user-initiated reload of the configuration file" by typing the following key combination:
|
Very good catch!! Please retest using the very latest script changes. |
Your quick and on the ball this morning @Martinski4GitHub ! Will test again shortly! Thanks for fixing the report! |
For the new "reload" key combination, I refactored & reused a lot of the code already written for the "offline" key sequence, so I decided to implement it while I had the solution "fresh" in mind this morning. It didn't take too long once I got rolling :>). |
Reload works perfectly for me! I did encounter some anomalies when installing this latest script. FYI, to install, I have been executing the following script (I have a similar one for @ExtremeFiretop_'s repository_): First, the script falsely detected a firmware update — I believe that I saw an error flash by about being unable to read the changelog. The email contents follows: Rerunning check for updates found none. Second, I wound up with multiple instances of the MerlinAU web UI tab, namely user2.asp and user7.asp. To resolve, I removed two MerlinAU entries from menuTree.js, removed both .asp/.title pairs from /www/user, and I reran sh /jffs/scripts/MerlinAU startup. |
Huh... It shouldn't do that... I had scripted it to check the contents of MenuTree before attempting to append a new entry. Will investigate momentarily |
Thanks for addressing so quickly. Will test this afternoon. I have a meeting with financial advisor in 30 minutes (considering consolidating retirement accounts to one advisor going forward) so it will most likely be after that. |
I had a few extra minutes to test this. Works perfectly now! |
Did you try the new key sequence |
I'm unable to update the value at all from the WebUI if the script is running currently. But when I made the report; it was actually between switching from the main menu to the advance menu, without using the key sequence (which to my understanding is refreshing it similarly) |
Okay; I found the problem why I was unable to update the value at all. That was still my bad. |
Ah, OK, understood. That's part of the "growing pains" when the code is still in flux & changing fast during the initial development where things are not yet "stable." |
Using the new key sequence to "reload & refresh" is currently the best way to make sure the CLI menus are accurately updated - short of exiting & relaunching the script. Technically, users are not supposed to run & open both UIs at the same time, make configuration changes, and expect both UIs to automatically reflect those changes (even by switching menus). We're not dealing with that kind of real-time OS performing dynamic polling & refresh cycles. IMO, given the current "WebGUI + CLI menu" framework, the best we can do is provide an easy way for the user to initiate a "reload & refresh" of the CLI menus as needed. The new key sequence now provides that functionality. Anything else we do would be on a best-effort basis. If and when the WebGUI page is released "to the wild" and comes online, my answer to users who complain about some CLI menu options not reflecting the values from the WebGUI while both are running at the same time will be: "Use the key combination to reload & refresh." |
Okay perfect, that's enough of an explanation for me to note the key sequence as something we should add to the Wiki when we move to 1.4.0 with the WebUI! Basically I can already see the potential of users going "it doesn't work like the other options!" But at that point we will just forward them to the Wiki with this new entry to use the key sequence or close the script completely. |
I can also see at least one user complaining about the WebGUI page not "refreshing" automatically after some configuration change is made via the CLI menu while both are running at the same time. The possible scenarios can start to get silly if users expect that both UIs are supposed to refresh automatically with changes made by any one of the UIs. Honestly, my short answer in all cases will be: |
Or, "Use the browser's equivalent of the script's key sequence to reload and refresh!" (aka refresh browser window) |
| @@ -15,6 +15,10 @@ readonly SCRIPT_NAME="MerlinAU" | |||
| ## Set to "master" for Production Releases ## | |||
| SCRIPT_BRANCH="dev" | |||
|
|
|||
| ##FOR TESTING/DEBUG ONLY## | |||
There was a problem hiding this comment.
This can be removed when going to production
- Code changes & improvements to make mounting the WebGUI page more robust. - Fix for calling F/W Update checks when ENABLED.
|
I just committed new changes to make mounting the WebGUI page more robust. |
Some code improvements in the script-locking mechanism.
Fixes in the WebGUI JavaScript code to detect and convert F/W version strings to numbers correctly for comparisons purposes. Minor changes to shell script.
Fixed issues with detecting & converting Alpha/Beta F/W versions.
Modified code to wait for the call to the "/usr/sbin/webs_update.sh" to return instead of executing it in the background.
|
Some outstanding things I'll check into tomorrow:
Off to bed for now, goodnight buddy! I'll be up bright and early tomorrow to poke at this more with you ;) |
Yeah, I noticed the 1st issue today when I was testing, so it was on my to-do list. |
Like you said in the forums, slowly but surely we will get there! It's not a small over-hall. I mean, it started fairly large with my PR and now looking at it with yours merged it, it's just very apparent how much of an over-hall it is. I can tell while reviewing the PR that some of the shell script changes weren't strictly required, things like renaming or moving some functions may have not been required but it does look much cleaner so in the end it's an improvement all the same. Half of the reported stuff in this PR I could of addressed in mine, but I didn't want to cause too much of a merge conflict so I figured we would work one PR at a time. Tomorrow, back to mine (unless you come up with fixes first) |
|
Also worth mentioning; I just completed my first ever update, triggered by the WebUI. No CLI at all. |
Oh yeah, adding a WebGUI is not a slam dunk!! There's a lot of new functionality that needs to be created just in the shell script alone (in addition to all the new code in the ASP file), and "old code" needs to be modified to account for the new interactions between the WebGUI and the shell script (e.g. lock mechanism). So yeah, it's going to take some time.
Honestly, for a long time, I've been wanting to rename some variables because of the warnings I keep getting from the Linter tool. Variable names like "SETTINGSFILE" & "SCRIPTVERPATH" trigger a coding style warning (using all capital letters without any "spacing" (i.e. underscore) is considered a "bad" coding practice). Since adding the WebGUI requires a major overhaul of the code, I decided to do some renaming, especially after the 850+ warnings I got from running the Linter tool on your original PoC code. It's certainly much cleaner now, and it makes it easier to read & review.
Good call!! |
Yeah, we're getting there, inching closer, although, IMO, I think we're still at least a month away from a Beta test release. |
This is fixed with the latest PR #386 |
Submitted PR: #387 |
















Lots of changes have been made so that future reviews are much easier. The Linter tool reported over 850 warnings combined (both shell & ASP files) so the really significant warnings were buried with all the rest. To remedy this, lots of changes were made simply to comply with our coding style & standards and remove over 700 warnings.
But also, some fixes and improvements were made to both the shell script and the HTML/JavaScript code (too numerous to list them all).
Heere are significant changes to the way we used to do some things:
The "Lock" file now has a "type" parameter so that we can differentiate between "CLI Menu calls" vs "CLI option" calls.
There are 2 new CLI parameters:
Parameter install to run the installation routine which downloads, install & set up the files, including the webGUI page.
/jffs/scripts/MerlinAU.sh install
Parameter startup to run initialization routines following a router reboot, including setting up the webGUI page.
/jffs/scripts/MerlinAU.sh startup
One additional entry is added to the "services-start" hook script to automatically run the "startup" call.
The WebGUI page was modified to show different statuses with different colors to readily see what's enabled, disabled, or TBD.
Added "check and validation" routines for the option to set the number of postponement days to make sure only valid values are allowed and eventually written to the config file.