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

[New] Run valve isolation trace #203

Merged
merged 53 commits into from
Jul 17, 2023
Merged

Conversation

des12437
Copy link
Contributor

@des12437 des12437 commented Jun 14, 2023

Description

This PR implements Run valve isolation trace in the Utility Network category.
URL to README: URL

Linked Issue(s)

  • swift/issues/4067

How To Test

  • Tap on one or more features to use as filter barriers or create and set the configuration's filter barriers by selecting a utility category.
  • Toggle "Isolated Features" to update trace configuration.
  • Tap "Trace" to run a subnetwork-based isolation trace.
  • Tap "Reset" to clear filter barriers and trace results.

Screenshots

@des12437 des12437 self-assigned this Jun 14, 2023
@des12437 des12437 requested review from a team, njarecha and yo1995 and removed request for a team June 14, 2023 22:00
@des12437 des12437 marked this pull request as draft June 16, 2023 18:09
@des12437 des12437 force-pushed the des12437/Run-valve-isolation-trace branch from a1681af to c7a9155 Compare June 16, 2023 19:23
@des12437 des12437 marked this pull request as ready for review June 16, 2023 19:26
@des12437
Copy link
Contributor Author

This PR is now ready for review. I broke up the view model into separate files and resolved all linter warnings.

@yo1995 yo1995 requested review from rolson and removed request for njarecha June 21, 2023 18:06
@rolson
Copy link
Contributor

rolson commented Jun 23, 2023

I tried running this sample and I though I see this:

image

I'm unable to hit the Trace button without any barriers:
image

Something in the UX isn't correct it seems.

I'm also seeing a warning in the code:

image

The fix-it shows how to fix it by adding:

            @unknown default:

I also think the bottom toolbar still needs work. I'll try to understand the sample a little better and make a recommendation on that.

@rolson
Copy link
Contributor

rolson commented Jun 23, 2023

Can you make the "Category" button always say category and when you tap on it, you would see a checkmark next to the chosen category? I find the current behavior is not obvious. Also, there are some spacing issues when the text changes and it is animating in with an odd transition when it changes:

image

@rolson
Copy link
Contributor

rolson commented Jun 23, 2023

Another issue I see with this sample is that once I start adding barriers I have no way to reset. To me it seems like the reset button should be available once I start adding barriers. Thoughts?

image

@des12437
Copy link
Contributor Author

Another issue I see with this sample is that once I start adding barriers I have no way to reset. To me it seems like the reset button should be available once I start adding barriers. Thoughts?

Good point. The old sample does not allow the filter barriers to be reset, but that should be supported. Since there is more toolbar real estate, perhaps having two separate buttons (Trace and Reset) to run or reset the trace would support this feature.

Copy link
Collaborator

@yo1995 yo1995 left a comment

Choose a reason for hiding this comment

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

First batch of comments.

@des12437 des12437 requested a review from yo1995 July 7, 2023 19:51
@des12437 des12437 requested a review from philium July 10, 2023 20:01
philium
philium previously approved these changes Jul 10, 2023
yo1995
yo1995 previously approved these changes Jul 14, 2023
Copy link
Collaborator

@yo1995 yo1995 left a comment

Choose a reason for hiding this comment

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

Thank you for writing this sample! It's been a great effort and while there are a few things that can be improved, overall it works well!

Only 1 remaining suggestion that causes a warning in the code.

@des12437 des12437 dismissed stale reviews from yo1995 and philium via 5fa0370 July 14, 2023 17:39
@des12437 des12437 requested review from yo1995 and philium July 14, 2023 17:40
@des12437
Copy link
Contributor Author

Thank you both! @philium @yo1995

@des12437 des12437 merged commit 444592b into v.next Jul 17, 2023
1 check passed
@des12437 des12437 deleted the des12437/Run-valve-isolation-trace branch July 17, 2023 16:03
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.

None yet

5 participants