luci icon indicating copy to clipboard operation
luci copied to clipboard

luci-app-firewall: make a dropdown list for flow offloading options

Open arinc9 opened this issue 3 years ago • 7 comments

Either software or hardware offloading can be used at a time. Make a dropdown list for them to reflect this on the firewall section of LuCI.

arinc9 avatar Feb 14 '23 21:02 arinc9

I wanted to import the cbiRichListValue variable from interfaces.js like this:

import { cbiRichListValue } from '../network/interfaces.js';

cbiRichListValue.prototype.renderWidget = function(section_id, option_index, cfgvalue) {
	var choices = this.transformChoices();
	var widget = new ui.Dropdown((cfgvalue != null) ? cfgvalue : this.default, choices, {
		id: this.cbid(section_id),
		sort: this.keylist,
		optional: false,
		select_placeholder: this.select_placeholder || this.placeholder,
		custom_placeholder: this.custom_placeholder || this.placeholder,
		validate: L.bind(this.validate, this, section_id),
		disabled: (this.readonly != null) ? this.readonly : this.map.readonly
	});

	return widget.render();
};

But I kept getting the "import declarations may only appear at top level of a module" error. So I currently define the variable again on zones.js but change "optional: true" to "optional: false".

arinc9 avatar Feb 14 '23 21:02 arinc9

@jow- looking forward to your review.

arinc9 avatar Feb 18 '23 11:02 arinc9

@jow- another ping regarding this

arinc9 avatar Dec 25 '23 13:12 arinc9

I don't see any inherent problems in doing so. But does it need to be redefined just to change the boolean? Won't it inherit and override?

systemcrash avatar Dec 25 '23 13:12 systemcrash

@systemcrash I don't know JS at all and am not interested in learning it. I'd had crafted this with ChatGPT back in the day to resolve a logical mistake that's still there. It's been more than a year which this patch has been waiting for a proper review. I would really appreciate it if you'd apply whatever you're suggesting and resubmit so that this fix can finally find its way to LuCI.

arinc9 avatar Mar 26 '24 18:03 arinc9

So I tested it on v22. Works fine, but for the missing 'require ui';.

It does the same thing in the same amount of clicks. Is there any reason this must go in?

systemcrash avatar Mar 27 '24 00:03 systemcrash

Because the current representation of the flow offloading options is wrong. As I described on the commit log, only one flow offloading option can be used at a time.

arinc9 avatar Mar 27 '24 05:03 arinc9