From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Adolf Belka (ipfire)" To: development@lists.ipfire.org Subject: Re: [PATCH v2 6/6] zoneconf.cgi: Improve VLAN & STP inputs Date: Mon, 22 Feb 2021 23:27:03 +0100 Message-ID: In-Reply-To: <8A7EEE6D-A2B8-4703-B072-1DB10C19FAB9@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============5067493173791817681==" List-Id: --===============5067493173791817681== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi all, On 22/02/2021 20:02, Michael Tremer wrote: > Hi, > >> On 21 Feb 2021, at 10:38, Leo Hofmann wrote: >> >> 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 w= rote: >>>> >>>> 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 =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. >>> >>> Isn=E2=80=99t there a danger that people will hit save prematurely and co= nnect the wrong VLAN with another one? >>> >>> Usability issues like that cannot be prevented entirely, but I was wonder= ing if this didn=E2=80=99t make it easier to run into that error. >> Agreed, this could happen if you are careless! >> However, I hope you only enable VLAN if you are sure which ID you are supp= osed to use. In that case, having a default value appear makes it obvious whe= re 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 = default. Sorry for late reply. For me, I had no problem figuring out where I should pu= t the VLAN ID. I am happy with the system as it was but no hard feelings agai= nst having a default value either. A decision to stay with empty by default is fine with me. Regards, Adolf > I will merge the patch and edit this change out. > >>>> print <>>> >>>> @@ -486,7 +489,7 @@ END >>>> >>>> >>>> >>>> - >>>> + >>>> >>>> END >>>> ; >>>> @@ -513,6 +516,9 @@ foreach (@zones) { # load settings and prepare form = elements for each zone >>>> my $stp_available =3D $ethsettings{"${uc}_MODE"} eq "bridge"; # STP is = only 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 :) >>> 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 be= longs. >>> >>> 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)! > No need for such a small thing. > > But thanks anyways :) > > -Michael > >>> -Michael >>> >>>> # 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 >>>> >> Regards, Leo --===============5067493173791817681==--