Skip to content

adds isAttached getter to DraggableScrollableController#100269

Merged
fluttergithubbot merged 3 commits intoflutter:masterfrom
maheshj01:draggablesheet
Apr 14, 2022
Merged

adds isAttached getter to DraggableScrollableController#100269
fluttergithubbot merged 3 commits intoflutter:masterfrom
maheshj01:draggablesheet

Conversation

@maheshj01
Copy link
Member

@maheshj01 maheshj01 commented Mar 17, 2022 •
edited
Loading

This PR adds isAttached getter to DraggableScrollableController similar to ScrollController.hasClients

Fixes #99994

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

@flutter-dashboard flutter-dashboard bot added f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. labels Mar 17, 2022
@maheshj01 maheshj01 changed the title adds isAttached getter to DraggableScrollController adds isAttached getter to DraggableScrollableController Mar 17, 2022
@HansMuller HansMuller requested a review from Piinks March 18, 2022 22:00
Piinks
Piinks reviewed Mar 22, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Hey @maheshmnj! Can this instead use the hasClients property that already exists on ScrollController?

@Piinks
Copy link
Contributor

Piinks commented Mar 22, 2022

For more context, just because _attachedController is not null, does not mean there is an attached position that is safe to access. :)

@maheshj01
Copy link
Member Author

maheshj01 commented Mar 23, 2022 •
edited
Loading

Can this instead use the hasClients property that already exists on ScrollController?

@Piinks, Oh Yes, We should use hasClients, But I was wondering to check for null too, Considering the added test

expect(controller.isAttached, false); ///  would throw Null check operator used on Null value

Instead of making isAttached nullable probably adding a null check would be right?

@goderbauer goderbauer requested a review from Piinks March 30, 2022 21:39
Piinks
Piinks reviewed Apr 5, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart Outdated
Copy link
Contributor

@Piinks Piinks Apr 5, 2022

Choose a reason for hiding this comment

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

Can you update the _assertAttached function to use this getter now?

Copy link
Member Author

@maheshj01 maheshj01 Apr 6, 2022

Choose a reason for hiding this comment

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

Done

@goderbauer
Copy link
Member

goderbauer commented Apr 6, 2022

From triage: This probably needs to be rebased to make the failing check happy.

@goderbauer goderbauer requested a review from Piinks April 6, 2022 21:31
@maheshj01
Copy link
Member Author

maheshj01 commented Apr 7, 2022

Thanks for checking @goderbauer, I have rebased it.

Piinks
Piinks approved these changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Flutter_LGTM

Thank you!

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Apr 7, 2022

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@goderbauer
Copy link
Member

goderbauer commented Apr 13, 2022

Restarted the Google Testing check. Hopefully it will pass now.

@fluttergithubbot fluttergithubbot merged commit c815aae into flutter:master Apr 14, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 14, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 14, 2022
@engine-flutter-autoroll
c815aae adds isAttached getter to DraggableScrollableController (fl…
47705a5 
…utter/flutter#100269)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 15, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 15, 2022
@engine-flutter-autoroll
c815aae adds isAttached getter to DraggableScrollableController (fl…
842f77b 
…utter/flutter#100269)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 15, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 15, 2022
@engine-flutter-autoroll
c815aae adds isAttached getter to DraggableScrollableController (fl…
0dc55b9 
…utter/flutter#100269)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 15, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 15, 2022
@engine-flutter-autoroll
c815aae adds isAttached getter to DraggableScrollableController (fl…
d15f414 
…utter/flutter#100269)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 15, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 15, 2022
@engine-flutter-autoroll
c815aae adds isAttached getter to DraggableScrollableController (fl…
a442576 
…utter/flutter#100269)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 15, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 15, 2022
@engine-flutter-autoroll
c815aae adds isAttached getter to DraggableScrollableController (fl…
4279678 
…utter/flutter#100269)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 15, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Apr 15, 2022
@engine-flutter-autoroll
c815aae adds isAttached getter to DraggableScrollableController (fl…
8117f66 
…utter/flutter#100269)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 21, 2022
@engine-flutter-autoroll
c815aae adds isAttached getter to DraggableScrollableController (fl…
2ae25e5 
…utter/flutter#100269)
@NatsuOnFire NatsuOnFire mentioned this pull request Aug 30, 2022
Closed
@huycozy huycozy mentioned this pull request Sep 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Piinks Piinks Piinks approved these changes

Assignees

No one assigned

Labels

f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Proposal] Add "isAttached" property to DraggableScrollableController

4 participants

@maheshj01 @Piinks @goderbauer @fluttergithubbot