Skip to content

Conversation

@addaleax
Copy link
Collaborator

  • Implement HADRON_ISOLATED through the networkTraffic preference.
  • Derive the value of other preferences that are shadowed by networkTraffic through a generic mechanism for doing so (we will also need this for readOnly/enableShell)
  • Add an e2e test that runs on Linux and verifies via strace that Compass does not actually perform external network I/O when --no-network-traffic has been specified.

Description

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

- Implement `HADRON_ISOLATED` through the `networkTraffic` preference.
- Derive the value of other preferences that are shadowed by
  `networkTraffic` through a generic mechanism for doing so
  (we will also need this for `readOnly`/`enableShell`)
- Add an e2e test that runs on Linux and verifies via `strace` that
  Compass does not actually perform external network I/O when
  `--no-network-traffic` has been specified.
(k) =>
(k as unknown) === key ? originalStates[k] : deriveValue(k).state
);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alenakhineika I don’t think we specified in the tech design how networkTraffic interacts with the individual preference values. I chose to use a generic mechanism for this rather than hardcoding the preference names and their interactions, also considering that for enableShell and readOnly we are going to have to do something similar again.


const preferences2 = new Preferences(tmpdir);
const fetchResult2 = await preferences2.fetchPreferences();
expect(fetchResult2.autoUpdates).to.equal(true); // (!)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Left // (!) comments here because the behavior might be surprising, I think it’s the least surprising one of all different options though.

return {
label: '&Settings',
accelerator: 'Command+,',
accelerator: 'CmdOrCtrl+,',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also want to be able to use a shortcut on non-macOS platforms 😌

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

One thing caught my attention when trying out the feature locally: I see that in tech design networkTraffic is marked as available in settings UI, are you planning to add it there in the same PR or in a separate one? (I looked through the tickets, but couldn't find a separate ticket for something like that)

@addaleax
Copy link
Collaborator Author

@gribnoysup Just had a conversation with Claudia and Alena about that, we’re not planning to explicitly add a checkbox to the settings UI for networkTraffic at this point, thinking that being able to modify all the individual privacy options will give users what they need to achieve (almost) the same effect.

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Worth marking in the tech design maybe just so we have this decision documented? Otherwise looks good to me, that's an amazing feature we are building here!

One thing that does slightly bother me is that our preferences class is now re-implementing some of the ampersand models behavior, which I think is okay as a one-off thing here, but if we find ourselves doing this again, I would maybe give it another thought if we want to just use them for now instead of re-implementing them 🤔

@addaleax addaleax merged commit c02c5c1 into main Sep 22, 2022
@addaleax addaleax deleted the 6065-dev branch September 22, 2022 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants