Skip to content

[Proposal] ProgressManager Post-Review Update #1519

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.

Sign up for GitHub

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

[Proposal] ProgressManager Post-Review Update #1519

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e6af6b7
add alternatives considered
chloe-yeo Sep 24, 2025
48621b6
address PR comments
chloe-yeo Sep 24, 2025
e12818e
refine Property protocol alternatives considered
chloe-yeo Sep 24, 2025
2e08960
refine withProperties alternatives considered
chloe-yeo Sep 24, 2025
6845fc6
refine withProperties alternatives considered
chloe-yeo Sep 24, 2025
864df54
more detail for alternatives considerd
chloe-yeo Sep 24, 2025
67a5a09
edit proposal revision history
chloe-yeo Sep 24, 2025
3fcafe7
edit alternatives considered
chloe-yeo Sep 24, 2025
655e19d
fix typo
chloe-yeo Sep 24, 2025
6342785
fix alternatives considered
chloe-yeo Sep 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 39 additions & 4 deletions Proposals/0023-progress-manager.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,9 @@
- Expanded Alternatives Considered
* **v6** Minor Updates:
- Replaced `withProperties` method with `setCounts`
- Removed `ProgressManager.Values` struct
- Made `ProgressManager` conform to `@dynamicMemberLookup` and moved `subscript(dynamicMember:)` methods from `ProgressManager.Values` to `ProgressManager`
- Added `@dynamicMemberLookup` attribute to `ProgressManager` and `ProgressReporter`
- Changed behavior of API so that additional properties are restricted to either `Int`, `Double`, `String?`, `URL?`, `UInt64` or `Duration` types instead of `any Sendable` types
- Added overloads for `subscript(dynamicMember:)` to account for currently-allowed types
- Added requirements to `ProgressManager.Property` protocol to define summarization and termination (deinit) behavior
- Added requirements to `ProgressManager.Property` protocol to define summarization and cleanup (deinit) behavior
- Replaced `total(of:)` with overloads for `summary(of:)` to account for all available types and removed `values(of:)` method

## Table of Contents
Expand Down Expand Up @@ -1434,6 +1432,43 @@ There were discussions about representing indeterminate state in `ProgressManage
### Allow declared custom additional property to be any type that can be casted as `any Sendable`
We initially allowed the full flexibility of allowing developers to declare `ProgressManager.Property` types to be of any type, including structs. However, we realized that this has a severely negative impact on performance of the API. Thus, for now, we allow developers to only declare `ProgressManager.Property` with only certain `Value` and `Summary` types.

### Introduce `withProperties` closure as an entry point to atomically mutate multiple properties
We initially considered introducing a withProperties closure as an entry point to atomically mutate multiple properties. This approach would have used a dedicated ProgressManager.Values struct with `@dynamicMemberLookup` to provide a unified interface for modifying all properties within a single atomic operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We initially considered introducing a withProperties closure as an entry point to atomically mutate multiple properties. This approach would have used a dedicated ProgressManager.Values struct with `@dynamicMemberLookup` to provide a unified interface for modifying all properties within a single atomic operation.
We initially considered introducing a `withProperties` closure as an entry point to atomically mutate multiple properties. This approach would have used a dedicated `ProgressManager.Values` struct with `@dynamicMemberLookup` to provide a unified interface for modifying all properties within a single atomic operation.


However, this design had several drawbacks:
- Limited atomicity: While it allowed atomic mutation of `totalCount` and `completedCount`, it could not achieve the same atomicity for additional properties due to their differences in backing storage
Copy link
Contributor

@itingliu itingliu Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would add a few sentences here about why backing storage (an implementation detail) would affect this change of design. Why does it matter whether they're all stored into one single storage or multiple separate ones?

Either way, is it correct to say that if we are to enforce a whole atomicity for all of the additional properties, we would have to synchronize the access with a big lock for all of the properties, and would hinder performances?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reasons related to backing storage, my line of thinking is that If we were to allow totalFileCount and completedFileCount to be stored in a struct the way totalCount and completedCount are, we should then also allow developers to do the same. But this will then then mean we have to allow developers to declare any ProgressManager.Property, making us store (any Sendable). But for performance reasons, we do restrict them to only certain types such as Int, UInt64 etc. I think it would be more about consistency between what our pre-declared custom properties can do vs what developers' custom properties can do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why should an API user care about how totalFileCount and completedFileCount are stored, whether they're stored in a struct or not 😉 ? It could also be guarded by a semaphore for concurrent access. Would that allow atomically mutating everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm you're right, those are implementation details, I think the thing that would matter most to developers, especially UI developers, will be the performance cost that comes with having withProperties. Because withProperties acquires the lock and does the access and mutation of additional properties after, and releasing the lock before returning the values, the way we make these properties Observable is much less performant. We have to register all access and mutations of additional properties onto only one keypath because the access and withMutation calls on the ObservationRegistrar, which may cause unexpected and unnecessary redraws of the UI, putting performance cost on developers who opt to report additional properties with their progress via withProperties.

- Less intuitive API: The withProperties closure promoted a less natural pattern for accessing and mutating additional properties, particularly when combined with `@dynamicMemberLookup` on the nested `ProgressManager.Values` struct
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Less intuitive API: The withProperties closure promoted a less natural pattern for accessing and mutating additional properties, particularly when combined with `@dynamicMemberLookup` on the nested `ProgressManager.Values` struct
- Less intuitive API: The `withProperties` closure promoted a less natural pattern for accessing and mutating additional properties, particularly when combined with `@dynamicMemberLookup` on the nested `ProgressManager.Values` struct

- Inconsistent access patterns: We would need to use different patterns for accessing fraction-related properties and additional properties
Copy link
Contributor

Choose a reason for hiding this comment

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

Were there discussions or feedback in the current version that was supportive of this pattern? What was the justification back then, and are they no longer valid?

Copy link
Contributor Author

@chloe-yeo chloe-yeo Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually the discussion that I had in mind was covered under less intuitive API, this is not needed.


We ultimately decided to replace this approach with a more streamlined design:
- `setCounts` closure: Atomically update `totalCount` and `completedCount`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify why only totalCount and completedCount are considered here, and why do you think that there needs to be a closure to atomically update both of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, sounds good!

- `ProgressManager` directly using `@dynamicMemberLookup`: Applied directly to the ProgressManager class, enabling intuitive dot-syntax access for all additional properties (e.g., manager.totalFileCount = 100)
- Consistent access pattern: Both accessing and mutating additional properties now use the same dot-syntax pattern, making the API more discoverable and natural to use

With this change, we can atomically update `totalCount` and `completedCount` via the `setCounts` closure, and access and mutate additional properties via dot syntax.

### Minimal requirements for `ProgressManager.Property` protocol
We initially considered a simpler version of the ProgressManager.Property protocol that required only two components:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
We initially considered a simpler version of the ProgressManager.Property protocol that required only two components:
We initially considered a simpler version of the `ProgressManager.Property` protocol that had two requirements:

I think we just call them requirements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change this!


- `Value` - The type for individual property values
- `defaultValue` - A default value when the property isn't explicitly set

In this simplified approach, the framework would automatically:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify what "the framework" refers to? Did you mean Foundation would do the below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but let me reword this!

- Aggregate all values throughout a progress tree into `Array<Value>`
- Provide access via `values(of:)` method returning arrays of individual values
- Provide access via `total(of:)` method returning computed summaries (e.g., `Int` for `totalFileCount`)
- Decide whether values are dropped or retained when `ProgressManager` instances are deinitialized

However, we realized that there were performance issues with the automatic aggregation of additional properties into an `Array`, and also that there is much less flexibility for specifying the `Summary` type they need, and summarization and cleanup behavior we need for their custom `ProgressManager.Property` type.

We decided to introduce additional requirements that would yield better performance and provide more flexibility:
- `Summary` - Explicit type for summaries
- `reduce(into:value:)` - Custom logic for incorporating individual values into summaries
- `merge(_:_:)` - Custom logic for combining summaries from different `ProgressManager` instances
- `finalSummary(_:_:)` - Custom behavior when `ProgressManager` instances are deinitialized

With these additional requirements, custom `ProgressManager.Property` types can be much more useful with greater flexibility to define behaviors according to specific needs.

## Acknowledgements
Thanks to
- [Tony Parker](https://github.com/parkera),
Expand Down