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

Considering Color.fromRGBA instead of fromARGB #5205

Closed
sethladd opened this issue Aug 3, 2016 · 12 comments
Closed

Considering Color.fromRGBA instead of fromARGB #5205

sethladd opened this issue Aug 3, 2016 · 12 comments
Assignees
Labels
c: API break Backwards-incompatible API changes c: new feature Nothing broken; request for a new capability engine flutter/engine repository. See also e: labels.

Comments

@sethladd
Copy link
Contributor

sethladd commented Aug 3, 2016

Feedback from a developer familiar with the web. They noticed that Flutter uses "ARGB" (for example, https://docs.flutter.io/flutter/rendering/Color/Color.fromARGB.html) but the web uses "RGBA" (for example, in CSS).

To more easily leverage the color converters out there, and existing docs around color and the web, Flutter could consider thinking in "RGBA" instead of "ARGB".

(not sure if there was a reason we went with ARGB?)

@sethladd sethladd added the framework flutter/packages/flutter repository. See also f: labels. label Aug 3, 2016
@sethladd
Copy link
Contributor Author

sethladd commented Aug 3, 2016

cc @Hixie

@abarth
Copy link
Contributor

abarth commented Aug 3, 2016

Is the "instead" part important or would Color.fromRGBA in addition to Color.fromARGB be acceptable?

not sure if there was a reason we went with ARGB?

It's how Skia prefers to pack the bits.

@a14n
Copy link
Contributor

a14n commented Aug 4, 2016

Having two really close names (fromRGBA and fromARGB) with the same types for parameters could be error prone IMHO.

@Hixie
Copy link
Contributor

Hixie commented Aug 4, 2016

CSS doesn't have a way to provide ARGB as a single hex code, does it? Just rgba(.., .., .., ...).

In which case we could have const Color(0xAARRGGBB) and new Color.fromRGBA(rr, gg, bb, aa), where the former is significantly preferred but the latter is available when you need to switch from CSS to Flutter.

@eseidelGoogle
Copy link
Contributor

The same developer also commented that our handling of Alpha as a byte instead of a float felt odd coming from a CSS background. So fromRGBA might need to be (byte, byte, byte, float) to feel like CSS.

@Hixie
Copy link
Contributor

Hixie commented Aug 4, 2016

Agreed.

@Hixie Hixie added affects: dev experience c: new feature Nothing broken; request for a new capability engine flutter/engine repository. See also e: labels. and removed framework flutter/packages/flutter repository. See also f: labels. labels Aug 4, 2016
@Hixie Hixie added this to the Flutter 1.0 milestone Aug 4, 2016
@abarth
Copy link
Contributor

abarth commented Aug 4, 2016

We're inconsistent about where we use a byte or a float. We should make sure we're consistent. (Typically we call the float "opacity" and the byte "alpha" but I'm not sure how consistent we are about that.)

@Hixie
Copy link
Contributor

Hixie commented Aug 4, 2016

I think we should use that convention in general but in this specific case I think we should still call it fromRGBA (not fromRGBO or fromRgbAndOpacity) because then it matches CSS, which is the point.

The documentation for that method should only refer to the fourth argument as the "opacity" though.

@sethladd sethladd added the c: API break Backwards-incompatible API changes label Aug 8, 2016
@Hixie
Copy link
Contributor

Hixie commented Aug 8, 2016

(Looking at this with fresh eyes, I must admit that I'm not sure I feel as strongly about my last comment as I did when I wrote it. Maybe Color.fromRGBO is the way to go, for internal consistency.)

@Hixie Hixie modified the milestones: Flutter 1.0, Flutter 0.8 Sep 16, 2016
@abarth abarth self-assigned this Sep 19, 2016
@abarth
Copy link
Contributor

abarth commented Sep 19, 2016

@Hixie says Color.fromRGBO is the one true name.

@abarth
Copy link
Contributor

abarth commented Sep 19, 2016

Not sure how to get from opacity (which is a double) to the internal representation (which is an int) as a const expression...

abarth added a commit to abarth/engine that referenced this issue Sep 20, 2016
Sometimes it is convenient to construct a color from an opacity rather than an
alpha value. Unfortunately, this constructor cannot be const because there's no
const way to convert from a double to the internal representation of a Color,
which is an int.

Fixes flutter/flutter#5205
abarth added a commit to abarth/engine that referenced this issue Sep 20, 2016
Sometimes it is convenient to construct a color from an opacity rather than an
alpha value.

Fixes flutter/flutter#5205
abarth added a commit to abarth/engine that referenced this issue Sep 20, 2016
Sometimes it is convenient to construct a color from an opacity rather than an
alpha value.

Fixes flutter/flutter#5205
abarth added a commit to flutter/engine that referenced this issue Sep 20, 2016
Sometimes it is convenient to construct a color from an opacity rather than an
alpha value.

Fixes flutter/flutter#5205
@github-actions
Copy link

github-actions bot commented Sep 6, 2021

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: API break Backwards-incompatible API changes c: new feature Nothing broken; request for a new capability engine flutter/engine repository. See also e: labels.
Projects
None yet
Development

No branches or pull requests

5 participants