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

Hosts file editor #20462

Merged
merged 60 commits into from
Oct 13, 2022
Merged

Hosts file editor #20462

merged 60 commits into from
Oct 13, 2022

Conversation

davidegiacometti
Copy link
Collaborator

@davidegiacometti davidegiacometti commented Sep 10, 2022

Summary of the Pull Request

New utility for managing the hosts file.

PR Checklist

  • Closes: HOST file editor #20204
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated
  • New binaries: Added on the required places
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

1
2
3
4

TODO

  • UI/UX/Icon
  • More tests coverage
  • Module interface
  • Settings page
  • Settings:
    • Startup warning
    • Additional lines position: top/bottom
  • OOBE page
  • Logging
  • Telemetry
  • Pipelines
  • Setup
  • Bug report tool
  • User docs

Known issues

  • Not opened in foreground when launched from the settings page

Validation Steps Performed

@davidegiacometti davidegiacometti marked this pull request as draft September 10, 2022 11:20
@davidegiacometti
Copy link
Collaborator Author

davidegiacometti commented Sep 10, 2022

@niels9001 do you have time to help with UI/UX and icon? Not urgent at all 😃

A few questions for the core team:

  • An utility like this doesn't have a strong dependency with the runner and also the concept of enable/disable but do we still want to have the module interface? I guess we need it for the settings.
  • What do you have in mind for the environment variables editor?
  • Can we early decide for the name of the utility?

CC @crutkas @jaimecbernardo @stefansjfw

@htcfreek
Copy link
Collaborator

htcfreek commented Sep 10, 2022

I know it is a draft. But I like to give some ux ideas:

  • Moving sort button between info bars and list.
  • Adding table headers.
  • Adding a "open settings" button.
  • What about having a "view hosts file" button?

What are the symbols on the second column for?

We can add a setting where to add comments (above the entry or behind).

Does it save imidiatly? I don't like souch apps. Maybe a save changes feature would be good to have.

We can add a start menu shortcut to make opening easy. This makes the utility available using start menu, start menu's search and PT Run.

@davidegiacometti
Copy link
Collaborator Author

I know it is a draft. But I like to give some ux ideas:

  • Moving sort button between info bars and list.
  • Adding table headers.
  • Adding a "open settings" button.

Thank you.
UI still needs a lot of improvements.

  • What about having a "view hosts file" button?

Do you mean a button that opens the hosts file in the default editor?
I think the goal of this tool is to not edit the hosts file manually anymore.

What are the symbols on the second column for?

Ping result.

We can add a setting where to add comments (above the entry or behind).

Comments are in line.

Does it save imidiatly? I don't like souch apps. Maybe a save changes feature would be good to have.

Yes, this is really a nice experience and aligned with PT settings.

We can add a start menu shortcut to make opening easy. This makes the utility available using start menu, start menu's search and PT Run.

Don't think this will be possible if we make the app running under PT runner and PT module interface.
Asked the core team in my previous message what's the ideal solution for this kind of utilities.

PowerToys.sln Outdated Show resolved Hide resolved
PowerToys.sln Outdated Show resolved Hide resolved
@htcfreek
Copy link
Collaborator

  • What about having a "view hosts file" button?

Do you mean a button that opens the hosts file in the default editor?
I think the goal of this tool is to not edit the hosts file manually anymore.

I don't want to have editing capabilities. But maybe a raw file view is a good addition. Maybe someone has to view the raw content for some reasons. It can be open in the default editor or a second page in the tool that shows the whole file content in a read only edit. (Then you don't have to navigate through the file system.)

@niels9001
Copy link
Contributor

@niels9001 do you have time to help with UI/UX and icon? Not urgent at all 😃

Loving this 😁👍. I can definitely help out on the UI, will pick this up next week. Mind if I push directly to this branch?

@crutkas
Copy link
Member

crutkas commented Sep 10, 2022

Suggestion. Have a big warning at the top that can be a setting to turn off? “Warning! altering this file has direct real world impact of how this computer resolves URLs”

On by default.

@davidegiacometti
Copy link
Collaborator Author

@niels9001 do you have time to help with UI/UX and icon? Not urgent at all 😃

Loving this 😁👍. I can definitely help out on the UI, will pick this up next week. Mind if I push directly to this branch?

Yeah, feel free to push directly.
Thank you! 😃

@niels9001
Copy link
Contributor

niels9001 commented Sep 14, 2022

@niels9001 do you have time to help with UI/UX and icon? Not urgent at all 😃

Loving this 😁👍. I can definitely help out on the UI, will pick this up next week. Mind if I push directly to this branch?

Yeah, feel free to push directly. Thank you! 😃

image

With error:
image

@davidegiacometti I had something like this in mind - thoughts?

Some questions:

  • Do we need the enable/disable context menu entries if we have the ToggleSwitch?
  • Do we need a loading indicator for when the ping is happening?

@htcfreek
Copy link
Collaborator

htcfreek commented Sep 14, 2022

@niels9001 do you have time to help with UI/UX and icon? Not urgent at all 😃

Loving this 😁👍. I can definitely help out on the UI, will pick this up next week. Mind if I push directly to this branch?

Yeah, feel free to push directly. Thank you! 😃

@davidegiacometti I had something like this in mind - thoughts?

Some questions:

  • Do we need the enable/disable context menu entries if we have the ToggleSwitch?
  • Do we need a loading indicator for when the ping is happening?

@niels9001

  • Loading indicator for ping sounds good.
  • What does the file icon do? Open file or duplicate entry? (Maybe duplicate entry feature makes sense.)
  • The icon for ping should have at least a tool tip to describe it's function.
  • Does a infobar about running in non admin mode makes sense. Maybe some people want to open only for viewing.
  • Do you saw the ask of @crutkas for the warning that changes are saved immediately.
  • Let's have a separate column for description and make hist name bold too.

@davidegiacometti
Copy link
Collaborator Author

@niels9001 looks awesome!

  • The enable/disable entry is redundant and can be removed.
  • A pinging indicator would be nice: should also be easy to implement.

@htcfreek

  • What does the file icon do? Open file or duplicate entry? (Maybe duplicate entry feature makes sense.)

It opens a popup that allows to add extra lines (including editing unparsed ones).
Will add duplicate feature to original issue. Let's add it after v1 release.

  • Does a infobar about running in non admin mode makes sense. Maybe some people want to open only for viewing.

Still wondering how this app will interact with PT runner. Not really sure if will have a start menu shortcut.
Maybe we can add a normal launch and launch elevated button in PT settings.

  • Do you saw the ask of @crutkas for the warning that changes are saved immediately.

I have already added a startup warning.

@htcfreek
Copy link
Collaborator

htcfreek commented Sep 14, 2022

  • What does the file icon do? Open file or duplicate entry? (Maybe duplicate entry feature makes sense.)
    It opens a popup that allows to add extra lines (including editing unparsed ones).
    Will add duplicate feature to original issue. Let's add it after v1 release.

@davidegiacometti , @niels9001
To not have misunderstandings: I mean a feature for duplicate an entry and not for duplicate the hosts file. And I don't think that we need an extra PR for this.

Maybe we should use an other icon. The current one is not really descriptive to indicate what you can do. I think about something with letters.

@htcfreek
Copy link
Collaborator

htcfreek commented Sep 14, 2022

  • Do you saw the ask of @crutkas for the warning that changes are saved immediately.

I have already added a startup warning.

@davidegiacometti
I am not sure if @crutkas not meant an infobar that is always visible.

@jaimecbernardo
Copy link
Collaborator

I've added some commits:

  • Downgrade the Build Tools that were upgraded to a preview version.
  • Launches as admin by default now.

@jaimecbernardo
Copy link
Collaborator

I've merged main, added winappsdk and PowerToysInterop files so the shared ones are used.
I've also added a commit so that starting from OOBE respects the Launch as Admin setting.

@jaimecbernardo
Copy link
Collaborator

To get a better look for the time being, I've added an icon and there and have it do the immersive dark mode route:
image

image

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot for the contribution! Will try to get this through the release CI to make sure everything is in place for the installer and signing as well.

@Aaron-Junker , is your feedback addressed already as well? The PR is blocking on your review, I think ;)

@davidegiacometti
Copy link
Collaborator Author

Thank you @jaimecbernardo 🚀

@jaimecbernardo
Copy link
Collaborator

Installer looks good now. Ready to go in.

@crutkas crutkas dismissed Aaron-Junker’s stale review October 12, 2022 16:25

believe this is ready

@crutkas
Copy link
Member

crutkas commented Oct 13, 2022

@davidegiacometti think you should have the privilege of squashing and merging :)

@crutkas crutkas mentioned this pull request Oct 13, 2022
@davidegiacometti davidegiacometti merged commit b2e1337 into main Oct 13, 2022
@davidegiacometti davidegiacometti deleted the users/davidegiacometti/hosts branch October 13, 2022 11:07
@nicolus
Copy link

nicolus commented Oct 14, 2022

  • What about having a "view hosts file" button?

Do you mean a button that opens the hosts file in the default editor? I think the goal of this tool is to not edit the hosts file manually anymore.

Just my 2 cents as a random user : I'd still like to have a shortcut to edit the file in the default editor (that would ideally open as administrator). The UI is really nice to add or enable/disable a couple of hosts, but when managing dozens of them sometimes I just want to be able to copy/paste a chunk of text or use column editing to comment out 20 lines at once.

At least this is my current workflow with hostsman, which does have the option to open the hosts file in a text editor.

@crutkas
Copy link
Member

crutkas commented Oct 15, 2022

Great feedback

@htcfreek
Copy link
Collaborator

htcfreek commented Oct 15, 2022

@crutkas , @nicolus , @davidegiacometti
I know that we had a discussion about multiline input (adding multiple entries at once) in the past. I don't know what the result was. But I can imagine to add a feature for csv/txt file import or something similar.

Let's create the following two issue for feature enhancements:

  1. Add button to open hosts file in default editor as administrator.
  2. Add feature to add/import multiple entries at once.

@davidegiacometti
Copy link
Collaborator Author

@nicolus looks reasonable. Let's add a button for open the file.
@htcfreek already tracked in the issue. I'll the issue for the import.

@davidegiacometti davidegiacometti restored the users/davidegiacometti/hosts branch October 15, 2022 11:11
@davidegiacometti davidegiacometti deleted the users/davidegiacometti/hosts branch October 15, 2022 11:30
@davidegiacometti davidegiacometti mentioned this pull request Oct 16, 2022
11 tasks
@davidegiacometti davidegiacometti mentioned this pull request Oct 31, 2022
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.

10 participants