Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

luci-mod-network: DHCP tabs redesign phase 2 #7178

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

systemcrash
Copy link
Contributor

This is a continuation of the work done in #6705. It implements GUI for dnsmasq based tags. Tags can trigger DHCP options (to be sent in DHCP responses), and vice-versa: DHCP client options and properties e.g. MAC/Vendor/User Class can trigger creation of tags. Recommended for users who know what they are doing, since invalid option:value combos can crash dnsmasq.

To describe tags succinctly is a challenge, such that a newcomer understands them more deeply, since they operate in two domains: ingress and egress. I spent a while coming up with the current descriptions. Suggestions welcome, but modifier beware. Syntax and descriptions are terse since you are expected to know what options and values you need.

Note: tags are still called 'networkid' in the dnsmasq.init script, which is really old terminology and should be updated. Current dnsmasq manpage and docu details 'tags' as the current terminology with little more than a footnote mentioning networkid any more.

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <[email protected]> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Tested on: 23.05.3
  • ( Preferred ) Screenshot or mp4 of changes:
  • Description: (describe the changes proposed in this PR)
dhcp-phase2.mp4

tag names shall not match:
- network devices/interfaces
- service names

Signed-off-by: Paul Donald <[email protected]>
_('Options to be added for this tag.'));
so.rmempty = true;
so.optional = true;
so.placeholder = '3,192.168.10.1,10.10.10.1';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be nice to add a validation there so that a combination of wrong option doesn't crach the dnsmasq?

Or even better can we not abstract this further without having to look up in the documentation of dnsmasq what option for example '3' exactly means?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe code golf one day.

But the user is nevertheless expected to know what settings they need. (There are lots of them).

so.rmempty = false;
so.optional = false;

so = ss.option(form.Value, 'networkid', _('Set this Tag'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we not replace this string Set this Tag with string ... To this matching Tag, then it would remain consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In short: no. That would change the meaning of how this tag tab works.

ss.rowcolors = true;

so = ss.option(form.DynamicList, 'dhcp_option',
_('Apply these DHCP Options...'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is a table I would prefer that we change the following strings Apply these DHCP Options... and ... To this matching Tag. This text should not be seen as a sentence. Maybe this can`t be translated like this in other languages.

Apply these DHCP Options... -> DHCP options
... To this matching Tag -> Matching tag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other languages are very capable of expressing this. I want these settings to be abundantly clear, because tags are evidently tough to grasp.

so.optional = true;
so.placeholder = '3,192.168.10.1,10.10.10.1';

so = ss.option(form.Value, 'tag', _('...To this matching Tag'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change text ...To this matching Tag?
See above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would confuse its meaning (implying how tags are used here)

ss.modaltitle = _('Edit Match');
ss.rowcolors = true;

so = ss.option(form.Value, 'match', _('Match this client option(+value)...'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether we can form a sentence from the table headings in all languages.

Match this client option(+value)... to Matching client option
...to Set this Tag to Setting tag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose verb here as more concrete than gerund. Setting tag becomes very confusing. Is it a noun? No - it isn't. It is not an instantiation either. It needs to describe the action.

so.optional = false;
so.placeholder = '61,8c:80:90:01:02:03';

so = ss.option(form.Value, 'networkid', _('...to Set this Tag'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...to: this is to say: in order to. There is a clear consequence. Cause and effect.

ss.modaltitle = _('Edit UC');
ss.rowcolors = true;

so = ss.option(form.Value, 'userclass', _('Match User Class...'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether we can form a sentence from the table headings in all languages.

Match User Class... to Matching user class
...to Set this Tag to Setting tag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment about gerund vs verb.

so.rmempty = false;
so.optional = false;

so = ss.option(form.Value, 'networkid', _('...to set this Tag'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous :)

ss.modaltitle = _('Edit VC');
ss.rowcolors = true;

so = ss.option(form.Value, 'vendorclass', _('Match Vendor Class...'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether we can form a sentence from the table headings in all languages.

Match Vendor Class... to Matching vendor class
...to Set this Tag to Setting tag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Translators do a fine job.

so.rmempty = false;
so.optional = false;

so = ss.option(form.Value, 'networkid', _('...to set this Tag'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@systemcrash
Copy link
Contributor Author

Thanks for the ideas @feckert. Tags are difficult...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants