From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs Date: Mon, 22 Feb 2021 19:02:34 +0000 Message-ID: <8A7EEE6D-A2B8-4703-B072-1DB10C19FAB9@ipfire.org> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3697982449146001961==" List-Id: --===============3697982449146001961== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi, > On 21 Feb 2021, at 10:38, Leo Hofmann wrote: >=20 > Hello Michael, >=20 > thank you for looking into these patches. I'll answer below! >=20 > Am 19.02.2021 um 20:22 schrieb Michael Tremer: >> Hello, >>=20 >>> On 18 Feb 2021, at 14:30, Leo-Andres Hofmann wr= ote: >>>=20 >>> Add default values and mark fields as required. >>>=20 >>> Signed-off-by: Leo-Andres Hofmann >>> --- >>> html/cgi-bin/zoneconf.cgi | 10 ++++++++-- >>> 1 file changed, 8 insertions(+), 2 deletions(-) >>>=20 >>> 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 =3D $_; >>> } >>> + =09 >>> + # default VLAN tag if not configured >>> + $zone_vlan_id =3D 1 unless looks_like_number($zone_vlan_id); >> I am not sure if it is a good idea to add a default here. >>=20 >> Isn=E2=80=99t there a danger that people will hit save prematurely and con= nect the wrong VLAN with another one? >>=20 >> Usability issues like that cannot be prevented entirely, but I was wonderi= ng if this didn=E2=80=99t make it easier to run into that error. >=20 > Agreed, this could happen if you are careless! > However, I hope you only enable VLAN if you are sure which ID you are suppo= sed to use. In that case, having a default value appear makes it obvious wher= e you need to put your desired ID. > But now I'm not sure about this one. I can revert this, of course. Well, we do not have many votes, but let=E2=80=99s maybe leave it empty by de= fault. I will merge the patch and edit this change out. >>=20 >>> print <>> >>> @@ -486,7 +489,7 @@ END >>> >>> >>> >>> - >>> + >>> >>> END >>> ; >>> @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and prepare form e= lements for each zone >>> my $stp_available =3D $ethsettings{"${uc}_MODE"} eq "bridge"; # STP is o= nly available in bridge mode >>> my $stp_enabled =3D $ethsettings{"${uc}_STP"} eq "on"; >>> my $stp_priority =3D $ethsettings{"${uc}_STP_PRIORITY"}; >>> +=09 >>> + # set priority to default value if no numerical value is configured >>> + $stp_priority =3D 32768 unless looks_like_number($stp_priority); >> This is very good. > Thanks :) >>=20 >> 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 bel= ongs. >>=20 >> 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 e= verything multiple times)! No need for such a small thing. But thanks anyways :) -Michael >>=20 >> -Michael >>=20 >>> # form element modifiers >>> my $checked =3D ""; >>> @@ -532,7 +538,7 @@ END >>> # priority input box HTML >>> my $row_2 =3D <>> >>> - >>> + >>> >>> END >>> ; >>> --=20 >>> 2.27.0.windows.1 >>>=20 > Regards, Leo --===============3697982449146001961==--