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

feat: Add host proxy pattern for global proxy configuration #752

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

divyam234
Copy link

@divyam234 divyam234 commented May 13, 2024

@mayswind PR related to discussion on #751

@@ -83,6 +83,18 @@
<em>[<span translate>Preview</span>] <span ng-bind="context.titlePreview"></span></em>
</div>
</div>
<div class="row">
<div class="setting-key setting-key-without-desc col-sm-4">
<span>{{ 'hostProxyPatternName' | translate }}</span>
Copy link
Owner

Choose a reason for hiding this comment

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

Please use the actual displayed content in English instead of placeholders to avoid unfriendly content that may be displayed when the language resources are not loaded.

</div>
<div class="setting-value col-sm-8">
<textarea class="form-control" rows="4" ng-model="context.settings.hostProxyPattern"
ng-change="setHostProxyPattern(context.settings.hostProxyPattern)" ng-keyup="inputKeyUp($event, true)"></textarea>
Copy link
Owner

Choose a reason for hiding this comment

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

AriaNgSettingsController does not have a function called "inputKeyUp", you can remove the ng-keyup attribute.

@@ -318,6 +318,21 @@
addUri: function (context, returnContextOnly) {
var urls = context.task ? context.task.urls : null;
var options = buildRequestOptions(context.task ? context.task.options : {}, context);
var proxyRegex= ariaNgSettingService.getHostProxyPattern().trim();
Copy link
Owner

Choose a reason for hiding this comment

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

Please format your code (a space should be appended to the left side of the equal symbol)

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's better to check ariaNgSettingService.getHostProxyPattern() is defined and is not null. I think you can just use the following code to replace Line 321-Line 322

if (ariaNgSettingService.getHostProxyPattern() && ariaNgSettingService.getHostProxyPattern().trim()) {
  // ...
}

if (proxyRegex) {
var patterns = proxyRegex.split('\n');
for (var i = 0; i < patterns.length; i++) {
patterns[i] = patterns[i].trim()
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a semicolon at the end of the line

patterns[i] = patterns[i].trim()
if (patterns[i]) {
var lastIndex = patterns[i].lastIndexOf('|');
if (lastIndex === -1) continue
Copy link
Owner

Choose a reason for hiding this comment

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

Please do not write the if condition and the actual execution code on the same line. You can just write like the following code

if (lastIndex < 0) {
    continue;
}

if (lastIndex === -1) continue
var hostRegex = patterns[i].slice(0, lastIndex).trim();
var httpProxy = patterns[i].slice(lastIndex + 1).trim();
if (urls[0].match(hostRegex)) options['all-proxy'] = httpProxy
Copy link
Owner

Choose a reason for hiding this comment

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

Check if the all-proxy is not set? Should the priority of the parameter in tasks be higher than the priority of the global one?

Copy link
Owner

Choose a reason for hiding this comment

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

The urls may be null in Line 319, please check if it is null first.

@@ -310,6 +310,8 @@
'Cannot connect to aria2!': 'Cannot connect to aria2!',
'Access Denied!': 'Access Denied!',
'You cannot use AriaNg because this browser does not meet the minimum requirements for data storage.': 'You cannot use AriaNg because this browser does not meet the minimum requirements for data storage.',
'hostProxyPatternName': 'Host Proxy Pattern',
'hostProxyPatternDescription': 'Specify host regex pattern for proxy per line. If the host matches the pattern, the proxy will be used. Example: .*\\.example\\.com|proxy.com:8080 .*\\.example\\.org|proxy.org:8080',
Copy link
Owner

Choose a reason for hiding this comment

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

I suggest you just write one example since you already mentioned one item per line

@@ -169,6 +169,11 @@
$rootScope.setTheme(value);
};

$scope.setHostProxyPattern = function (value) {
ariaNgSettingService.setHostProxyPattern(value);
$rootScope.setProxyHostPattern(value);
Copy link
Owner

Choose a reason for hiding this comment

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

Don't need to call $rootScope.setProxyHostPattern(value), because there is no function called setProxyHostPattern in root scope.

@@ -306,7 +306,8 @@ Cannot initialize WebSocket!=Impossibile inizializzare WebSocket!
Cannot connect to aria2!=Impossibile connettersi a aria2!
Access Denied!=Accesso negato!
You cannot use AriaNg because this browser does not meet the minimum requirements for data storage.=Non puoi utilizzare AriaNg perché questo browser non soddisfa i requisiti minimi di archiviazione dati.

hostProxyPatternName=Modello proxy host
hostProxyPatternDescription=Specificare il modello regex host per il proxy per riga. Se l'host corrisponde al modello, verrà utilizzato il proxy. Esempio: .*\.example\.com|proxy.com:8080 .*\.example\.org|proxy.org:8080
Copy link
Owner

Choose a reason for hiding this comment

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

Please leave a blank line before the header of a new group

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