Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@guoguo338
Copy link
Contributor

@guoguo338 guoguo338 commented Aug 30, 2022

This PR is to fix issue: flutter/flutter#110324

image

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@ColdPaleLight ColdPaleLight requested a review from flar August 30, 2022 09:21
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

Some minor suggestions on the doc comment which I would like to review when done. Otherwise it looks great!

external void restore();

/// Pops the save stack until the count layer, if there is anything to pop.
/// Otherwise, does nothing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment about how this relates to getSaveCount(). Something along the lines of "the count layer that was previously retrieved with [getSaveCount]" or "After this method completes, the [count] should match the return value of [getSaveCount]".

Also, describe what happens if count > getSaveCount(). I believe it just gets ignored, but we should document that.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

/// Restores the save stack to a previous level as might be obtained from [getSaveCount].
/// If [count] is less than 1, the stack is restored to its initial state. If [count] is
/// greater than the current [getSaveCount] then nothing happens.

I hinted at what happens with a negative count and also implied that the initial save count is 1 (is that true?) - perhaps we should add a test that verifies these behaviors. I believe that one of the web implementations may not be protected against restoring too far which means that behavior will end up blowing up compared to the other implementations (check that restore protects against an empty stack and also that restoreToCount of values less than 1 all behave the same on all platforms).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Get that, I have added test to cover those corner cases.

@guoguo338 guoguo338 requested a review from flar August 31, 2022 08:10
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

Please fix the paragraph mentioned before pushing this change.

///
/// Use [save] and [saveLayer] to push state onto the stack.
///
/// If the state was pushed with [saveLayer], then this call will also
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - this paragraph is worded as if there is only one stack level being restored. Please reword it:

/// If any of the state stack levels restored by this call were pushed with
/// [saveLayer], then this call will also cause those layers to be composited
/// into their previous layers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Get, it's done.

Copy link
Member

@ColdPaleLight ColdPaleLight left a comment

Choose a reason for hiding this comment

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

LGTM!

@ColdPaleLight ColdPaleLight added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 1, 2022
@auto-submit auto-submit bot merged commit f74d243 into flutter:main Sep 1, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 1, 2022
zanderso pushed a commit to flutter/flutter that referenced this pull request Sep 1, 2022
…110796)

* f30fed5ab Ignore all translations in Matrix::TransformDirection (flutter/engine#35854)

* d8928a0b6 [Impeller] More aggressively downscale gaussian blur textures (flutter/engine#35853)

* 5454c09a0 Roll Fuchsia Mac SDK from sgXD5SyRPOxGjWV4q... to VCkCQ9VaBkOXaOnQU... (flutter/engine#35858)

* 4ef0eb243 Roll Skia from d505b9445bf6 to 74d7dcaf7a4e (1 revision) (flutter/engine#35857)

* 5cab38584 Do frustum culling after the MSAA backdrop draw (flutter/engine#35847)

* 4e6bb5da5 Roll Skia from 74d7dcaf7a4e to ca89d2781f78 (1 revision) (flutter/engine#35859)

* 5dc718f92 Roll Skia from ca89d2781f78 to 8c7e99e0546e (2 revisions) (flutter/engine#35860)

* c3a956050 Roll Skia from 8c7e99e0546e to defbe6b80d7b (1 revision) (flutter/engine#35861)

* 1f7fe40d8 Roll Skia from defbe6b80d7b to d8a45111ba9a (1 revision) (flutter/engine#35862)

* f74d2439e Add function restoreToCount to Canvas. (flutter/engine#35798)

* 152228e27 Roll Skia from d8a45111ba9a to 97f1748bbfc0 (1 revision) (flutter/engine#35863)

* 08394d74c Fixed the nullability annotation on codec `writeValue:` (flutter/engine#35814)

* 185f4f595 Revert "roll CanvasKit to 0.36.1 (attempt 2) (#35839)" (flutter/engine#35864)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants