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-app-firewall: make a dropdown list for flow offloading options #6247

Closed
wants to merge 1 commit into from

Conversation

arinc9
Copy link
Contributor

@arinc9 arinc9 commented Feb 14, 2023

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.

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.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
@arinc9
Copy link
Contributor Author

arinc9 commented Feb 14, 2023

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
Copy link
Contributor Author

arinc9 commented Feb 18, 2023

@jow- looking forward to your review.

@arinc9
Copy link
Contributor Author

arinc9 commented Dec 25, 2023

@jow- another ping regarding this

@systemcrash
Copy link
Contributor

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?

@arinc9
Copy link
Contributor Author

arinc9 commented Mar 26, 2024

@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.

@systemcrash
Copy link
Contributor

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?

@arinc9
Copy link
Contributor Author

arinc9 commented Mar 27, 2024

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
Copy link
Contributor Author

arinc9 commented Oct 23, 2024

Thanks for picking this up @systemcrash! I've been tying up the loose ends in my life so I'm very happy to see this happen.

@jow-
Copy link
Contributor

jow- commented Oct 23, 2024

Note that this invalidated the translations for the hw offloading option.

@systemcrash
Copy link
Contributor

I didn’t see them immediately compatible but I’ll look at updating those next. Thanks @jow-

@PalebloodSky
Copy link

Minor comment but I would word it "Flow Offloading" next to dropdown since that's commonly used, instead of "Offloading type". Nevertheless thanks for the commit.

@systemcrash
Copy link
Contributor

Fixed.

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.

4 participants