From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jonatan Schlag To: development@lists.ipfire.org Subject: Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs Date: Sun, 21 Feb 2021 20:06:23 +0100 Message-ID: In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0690864763104547997==" List-Id: --===============0690864763104547997== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Hi, first thanks for the patchset. Overall it looks quite good Am Sonntag, den 21.02.2021, 11:38 +0100 schrieb Leo Hofmann: > Hello Michael, > > thank you for looking into these patches. I'll answer below! > > Am 19.02.2021 um 20:22 schrieb Michael Tremer: > > Hello, > > > > > On 18 Feb 2021, at 14:30, Leo-Andres Hofmann < > > > hofmann(a)leo-andres.de> wrote: > > > > > > Add default values and mark fields as required. > > > > > > Signed-off-by: Leo-Andres Hofmann > > > --- > > > html/cgi-bin/zoneconf.cgi | 10 ++++++++-- > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > diff --git a/html/cgi-bin/zoneconf.cgi b/html/cgi- > > > bin/zoneconf.cgi > > > index 9d01d06ce..bbd042ffc 100644 > > > --- a/html/cgi-bin/zoneconf.cgi > > > +++ b/html/cgi-bin/zoneconf.cgi > > > @@ -478,6 +478,9 @@ END > > > if ($access_selected{"NONE"} eq "") { > > > $highlight = $_; > > > } > > > + > > > + # default VLAN tag if not configured > > > + $zone_vlan_id = 1 unless > > > looks_like_number($zone_vlan_id); > > I am not sure if it is a good idea to add a default here. > > > > Isn’t there a danger that people will hit save prematurely and > > connect the wrong VLAN with another one? This was also my first thought. > > > > Usability issues like that cannot be prevented entirely, but I was > > wondering if this didn’t make it easier to run into that error. Thats definitly the case. > > Agreed, this could happen if you are careless! > However, I hope you only enable VLAN if you are sure which ID you are > supposed to use. In that case, having a default value appear makes it > obvious where you need to put your desired ID. > But now I'm not sure about this one. I can revert this, of course. I would vote for without default value. There is just absolutey no save default value here and people could assume that this is something like, "I am clicking save and I am done". And thats not the case people have to think about vlan ids. I unterstand that field might be hard to spot, but that should not be solved by a default value. Another question which came to my mind while reviewing this: Is there any good error when the vlan id is wrong? Shure it is checked in the html, but i have not found any backend check ... (Not a problem with your patch, but i want to mention it.) Apart from this linter is happy too. Nice work. Jonat an PS.: Apart from this reading the file took 10 mins, till I found out that $uc has something todo with zones ... . At least a good learning how to not name variables :-) > > > > print < > > > > > @@ -486,7 +489,7 @@ END > > > > > > > > > > > > - > > value="$zone_vlan_id" $field_disabled> > > > + > > value="$zone_vlan_id" required $field_disabled> > > > > > > END > > > ; > > > @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and > > > prepare form elements for each zone > > > my $stp_available = $ethsettings{"${uc}_MODE"} eq "bridge"; # > > > STP is only available in bridge mode > > > my $stp_enabled = $ethsettings{"${uc}_STP"} eq "on"; > > > my $stp_priority = $ethsettings{"${uc}_STP_PRIORITY"}; > > > + > > > + # set priority to default value if no numerical value is > > > configured > > > + $stp_priority = 32768 unless looks_like_number($stp_priority); > > This is very good. > Thanks :) > > Since this is in this patchset and comes with the dependency to the > > other code above, I cannot pull this in with the STP patchset where > > it actually belongs. > > > > But I guess we should merge this all together anyways :) > I still have these commits in my local git. I could just submit these > last two changes if that makes your job easier (and you don't mind me > submitting everything multiple times)! > > -Michael > > > > > # form element modifiers > > > my $checked = ""; > > > @@ -532,7 +538,7 @@ END > > > # priority input box HTML > > > my $row_2 = < > > > > > - > > id="STP-PRIORITY-$uc" name="STP-PRIORITY-$uc" min="1" max="65535" > > > value="$stp_priority" $disabled> > > > + > > id="STP-PRIORITY-$uc" name="STP-PRIORITY-$uc" min="1" max="65535" > > > value="$stp_priority" required $disabled> > > > > > > END > > > ; > > > -- > > > 2.27.0.windows.1 > > > > Regards, Leo --===============0690864763104547997==--