Skip to content

Add all system cursors (framework) #60931

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

Jump to bottom
Merged
fluttergithubbot merged 7 commits into from
Jul 9, 2020
Merged

Add all system cursors (framework) #60931

fluttergithubbot merged 7 commits into from
Jul 9, 2020

Conversation

@dkwingsmt
Copy link
Contributor

@dkwingsmt dkwingsmt commented Jul 6, 2020 •
edited
Loading

Description

This PR adds support to a number of system cursors to the framework.

With PR, Flutter now supports all system cursors from the following platforms:

  • Android
  • CSS
  • macOS
  • Linux (GTK)

Breakage

This PR renames the following cursors:

  • verticalDoubleArrow is renamed to resizeUpDown
  • horizontalDoubleArrow is renamed to resizeLeftRight

Technically a breaking change, however since I've found no reference of these values in g3 or flutter/tests, and these values have not been cut into a dev build, it should cause no breakage.

These renames are decided for the following reasons:

  • For the consistency of naming cursors based on their uses instead of their shapes
  • For the convenience of naming the renaming cursors (the single arrow cursors, the col/row resizing cursors)

Related Issues

Tests

I added the following tests:

  • No tests are added. They're simply new enum values, and testing them would be literally writing all code a second time.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Jul 6, 2020

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jul 6, 2020
@dkwingsmt dkwingsmt mentioned this pull request Jul 6, 2020
Merged
8 tasks
This was referenced Jul 7, 2020
Merged
Merged
@goderbauer goderbauer requested a review from Piinks July 8, 2020 21:47
@Piinks Piinks added a: desktop Running on desktop a: mouse Issues related to using a mouse or mouse support labels Jul 8, 2020
Piinks
Piinks reviewed Jul 8, 2020
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.

All these docs are really great.

packages/flutter/lib/src/rendering/mouse_cursor.dart
///
/// * Android: TYPE_WAIT
/// * Web: wait
///
Copy link
Contributor

@Piinks Piinks Jul 8, 2020

Choose a reason for hiding this comment

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

Is there no mac equivalent? Like the spinning beach ball? 😜

Copy link
Contributor Author

@dkwingsmt dkwingsmt Jul 8, 2020

Choose a reason for hiding this comment

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

I probably should doc this, but mac decides when to show the spinning beach ball by itself (when the application does not respond a blocked callback for a while), and does not expose a way to manually trigger it.

packages/flutter/lib/src/rendering/mouse_cursor.dart Outdated
Comment on lines 558 to 559
/// * Web: crosshair
/// * Web: crosshairCursor
Copy link
Contributor

@Piinks Piinks Jul 8, 2020

Choose a reason for hiding this comment

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

Are these equivalent? Or specific to certain browsers?

Copy link
Contributor Author

@dkwingsmt dkwingsmt Jul 8, 2020

Choose a reason for hiding this comment

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

Good catch, this is actually macOS.

packages/flutter/lib/src/rendering/mouse_cursor.dart
/// A cursor indicating moving something.
///
/// Typically the shape of four-way arrow. May fall back to [allScroll].
Copy link
Contributor

@Piinks Piinks Jul 8, 2020

Choose a reason for hiding this comment

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

allScroll mentions falling back on move, and here it is vice versa. Is there a specific condition or precedence for one or the other?

Copy link
Contributor Author

@dkwingsmt dkwingsmt Jul 8, 2020

Choose a reason for hiding this comment

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

Yeah, I thought move should be ubiquitous so that allScroll might fall back to it, but it turns out no platforms have move but no allScroll at all. I'll remove this fallback.

@dkwingsmt dkwingsmt requested a review from Piinks July 8, 2020 23:07
Piinks
Piinks approved these changes Jul 8, 2020
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.

LGTM!

@fluttergithubbot fluttergithubbot merged commit d4b4eee into flutter:master Jul 9, 2020
@dkwingsmt dkwingsmt deleted the complete-system-cursors branch July 9, 2020 22:17
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@dkwingsmt @mingwandroid
Add all system cursors (framework) (flutter#60931)
8d41caa
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Piinks Piinks Piinks approved these changes

Assignees

No one assigned

Labels

a: desktop Running on desktop a: mouse Issues related to using a mouse or mouse support 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.

4 participants

@dkwingsmt @fluttergithubbot @Piinks @googlebot