edgafner / GBrowser

A Browser embedded in your IDE
Apache License 2.0
74 stars 18 forks source link

Add the option to override the user agent #63

Closed Majed6 closed 1 year ago

Majed6 commented 1 year ago

Hi,

First of all thank you for this amazing plugin.

In this PR I hope to add the ability to override the user agent in order to make this plugin more applicable to my use case.

Kind regards, Majed

Jonatha1983 commented 1 year ago

Hi @Majed6 thanks for the PR

Small question - why do you need both the checkbox and the text field for the user agent - can we just check if there is a value in the textfied and if so set the user agent as you did ? Also did you test is locally ? if yes can you write a small comment on how to test it so I can also check on my local machine - thanks again for the PR

Jonatha1983 commented 1 year ago

Also I see that we can enable the user agent check box without setting a user agent String - so better to validate this and block the user in such case ( saying this in case we can't remove the check box you added )

Majed6 commented 1 year ago

Hi @Majed6 thanks for the PR

Small question - why do you need both the checkbox and the text field for the user agent - can we just check if there is a value in the textfied and if so set the user agent as you did ? Also did you test is locally ? if yes can you write a small comment on how to test it so I can also check on my local machine - thanks again for the PR

Hi,

Thank you for accepting the feature request. The goal of the checkbox is :

  1. A better UX If the user wants to toggle between having the user agent overwritten or not without having to retype the field every time.
  2. If the user wants to set the user agent to an empty value.

We can test it by asking google what is our user agent or visiting websites like https://www.whatismybrowser.com/detect/what-is-my-user-agent/

Also I see that we can enable the user agent check box without setting a user agent String - so better to validate this and block the user in such case ( saying this in case we can't remove the check box you added )

This is intended to allow empty value.

Also the text field can be edited without checking the box to allow the user to type in the user agent and overwrite it at a later time when needed.


I would like to generalize this feature even further by making an overwrite headers table instead as follows:


+---------------+---------------+--------------------+----------------------+
|   URI Regex   |    Header     |       Value        | Overwrite(If exists) |
+---------------+---------------+--------------------+----------------------+
| mydomain\.tld | Some-Header   | some use case      | []                   |
| .*localhost   | Authorization | my secret value    | []                   |
| .*            | User-Agent    | Firefox user-agent | [X]                  |
+---------------+---------------+--------------------+----------------------+

I haven't done tables before and I'm not really sure how well this will go , so I'll get back to you on that by Sunday. Do you mind having such level of customization? I have achieved my use case with the current PR but I'm thinking about futureproofing the plugin.

Jonatha1983 commented 1 year ago

@Majed6 I think it is better to have the text field for the agent only - in case the user wants to change the user agent he needs to enter the user agent there and it will persist the next time he will open the settings.

In case he wants the default he only needs to remove the string from the text field.

If it is ok with you I will push a commit to your branch with my suggestion and you will be able to see what I mean. Let me know.

And again thanks for contributing to this project.

Majed6 commented 1 year ago

@Majed6 I think it is better to have the text field for the agent only - in case the user wants to change the user agent he needs to enter the user agent there and it will persist the next time he will open the settings.

In case he wants the default he only needs to remove the string from the text field.

If it is ok with you I will push a commit to your branch with my suggestion and you will be able to see what I mean. Let me know.

And again thanks for contributing to this project.

Absolutely go for it. 👍

Majed6 commented 1 year ago

Hi,

I managed to make it future proof for all headers. I'll do a few tests and refactoring of the new code and create a separate PR for you to choose from. Below is a picture of what it would look like.

UI

Majed6 commented 1 year ago

Here is the PR https://github.com/Jonatha1983/GIdeaBrowser/pull/64 . I prefer it over this PR since it does what I need and cover future needs. Pick the implementation that you like. Thanks again for being receptive to feature requests in your plugin.

Jonatha1983 commented 1 year ago

Closed in favor of #64