feat(@StatusSmartIdenticon): Add support for color rings in StatusSmartIdenticon #553

noeliaSD · 2022-02-09T11:23:53Z

Closes #517

What does the PR do

It has been created a new control StatusIdenticonRing and added it into StatusSmartIdenticon component.

To test the component, the needed properties have been added in sandbox models to display the StatusIdenticonRing control.

NOTE: StatusIdenticonRing provides the following API as a first approach (since we don't have the back-end specs clearly defined yet, when they have it clear we can decide if it is better to manage the data in another format):

property var ringSpec: Expected ListModel object to iterate through ring segments. Each segment (ListElement) contains:
1. The colorId: from 0 to 31 corresponding with the 32 distinctive segment defined in figma and with the order defined there (it should be defined as well in specs).
2. The segmentType: from 1 to 5 being these integer values the number of units the segment contains.
property real totalRingUnits: Total number of units in the segment
property real initalAngleRad: Initial angle value in radians
property real ringPxSize: Ring thickness

Affected areas

StatusSmartIdenticon component

Screenshot of functionality

Added example in Sandbox:

alexandraB99

lovely PR @noeliaSD ! :) just left a few comment, mostly stuff related to beautification :)

alexandraB99 · 2022-02-09T20:34:51Z

src/StatusQ/Controls/StatusIdenticonRing.qml

+
+    implicitWidth: 40
+    implicitHeight: 40
+    Layout.alignment: Qt.AlignHCenter | Qt.AlignVCenter


wondering if this is needed here?

No needed! Removed!

alexandraB99 · 2022-02-09T20:41:12Z

sandbox/controls/ListItems.qml

@@ -357,6 +368,8 @@ CExPynn1gWf9bx498P7/nzPcxEzGExhBdJGYihtAYQlO+tUZvqrPbqeudo5iJGEJjCE15a3VtodH3q2I
                      nzPcxEzGExhBdJGYihtAYQlO+tUZvqrPbqeudo5iJGEJjCE15a3VtodH3q2ImYgiNITTlTdG1nUZ5a92VITQxITFiJmIIjSE0htAYQrMHAAD//+wwFVpz+yqXAAAAAElFTkSuQmCC"
        image.isIdenticon: true
        isOnline: true
+        statusListItemIcon.identiconRing.ringSpec: identiconRing1
+        statusListItemIcon.identiconRing.totalRingUnits: 25


just for the sake of beautifying: I imagine that statusListItemIcon is part of StatusListItem and not of StatusMemberListItem because of Mouse events and z order reasons? I was wondering, is there any case we could avoid 'dotdotting' like statusListItemIcon.identiconRing.totalRingUnits: and have something more straight forward like totalRingUnits: instead?

Yes, I agree with your point of view, however... I don't know which is the best point to expose these properties but giving as much flexibility as possible to the component. StatusListItem is a super flexible component that exposes, I don't know if all, but most of its components and allows you to design a super custom list item.

I'm open to proposals! Maybe it could have sense to add all StatusIdenticonRingproperties as well in StatusSmartIdenticon so in this particular case we'd have something like this:

statusListItemIcon.ringSpecs or statusListItemIcon.totalRingUnits

WDYT?

One other way to go about this is to redefine dedicated settings on that higher level comopnent.

For example, right now we do statusListItemIcon.identiconRing.[...], because the SmartIdenticon is exposed on StatusListItem as statusListItemIcon.
To avoid having that very first level required to dot into deeper properties, we could introduce:

// StatusMemberListItem StatusListItem { StatusIdenticonRingSettings identiconRingSettings: StatusIdenticonRingSettings { ... } }

Analogue to how we do it for StatusIconSettings and StatusImageSettings.

Inside StatusMemberListItem we then need to delegate the top level IdenticonRingSettings to the underlying statusListItemIcon.identiconRingSettings

^ This also implies that StatusSmartIdenticon would make use StatusIdenticonRingSettings as well.

We're doing this pattern in a few other components already where dotting into object members gets a bit cumbersome.

Ok! It sound like a good idea. Going for that! :)

alexandraB99 · 2022-02-09T20:43:25Z

sandbox/controls/ListItems.qml

@@ -347,6 +347,17 @@ CExPynn1gWf9bx498P7/nzPcxEzGExhBdJGYihtAYQlO+tUZvqrPbqeudo5iJGEJjCE15a3VtodH3q2I
        requestsCount: 3
    }

+    ListModel {
+        id: identiconRing1


is this model used / going to be used elsewhere? If not, could we assign it directly to the component that's using it?

Yes, it could be assigned directly!

0x-r4bbit

Super cool @noeliaSD !

As @alexandraB99 there's probably room for improvement with regards to developer ergonomics. I've left some thoughts there on how we solved this in other components.

In addition, I think the color ring colors should live in Theme for a few reasons: a) that's just the place where colors are defined and b) we want to ensure these colors work across themes, so every theme should probably be able to define their own ring colors (needed to ensure things like contrast ratio)

0x-r4bbit · 2022-02-10T11:50:17Z

sandbox/controls/ListItems.qml

@@ -357,6 +368,8 @@ CExPynn1gWf9bx498P7/nzPcxEzGExhBdJGYihtAYQlO+tUZvqrPbqeudo5iJGEJjCE15a3VtodH3q2I
                      nzPcxEzGExhBdJGYihtAYQlO+tUZvqrPbqeudo5iJGEJjCE15a3VtodH3q2ImYgiNITTlTdG1nUZ5a92VITQxITFiJmIIjSE0htAYQrMHAAD//+wwFVpz+yqXAAAAAElFTkSuQmCC"
        image.isIdenticon: true
        isOnline: true
+        statusListItemIcon.identiconRing.ringSpec: identiconRing1
+        statusListItemIcon.identiconRing.totalRingUnits: 25


One other way to go about this is to redefine dedicated settings on that higher level comopnent.

For example, right now we do statusListItemIcon.identiconRing.[...], because the SmartIdenticon is exposed on StatusListItem as statusListItemIcon.
To avoid having that very first level required to dot into deeper properties, we could introduce:

// StatusMemberListItem StatusListItem { StatusIdenticonRingSettings identiconRingSettings: StatusIdenticonRingSettings { ... } }

Analogue to how we do it for StatusIconSettings and StatusImageSettings.

Inside StatusMemberListItem we then need to delegate the top level IdenticonRingSettings to the underlying statusListItemIcon.identiconRingSettings

^ This also implies that StatusSmartIdenticon would make use StatusIdenticonRingSettings as well.

We're doing this pattern in a few other components already where dotting into object members gets a bit cumbersome.

0x-r4bbit · 2022-02-10T11:51:41Z

sandbox/demoapp/StatusAppCommunityView.qml

@@ -266,6 +266,8 @@ StatusAppThreePanelLayout {
                image.source: model.source
                image.isIdenticon: model.isIdenticon
                isOnline: model.isOnline
+                statusListItemIcon.identiconRing.ringSpec: model.ringSpec
+                statusListItemIcon.identiconRing.totalRingUnits: model.totalRingUnits


This can be reduced to something like

identiconRingSettings.totalRingUnits // or identicon.totalRingUnits

Depending on how you wanna call that property, using the pattern described in the comment above.

0x-r4bbit · 2022-02-10T11:53:32Z

src/StatusQ/Components/StatusSmartIdenticon.qml


    // Badge color properties must be set if badgeItem.visible = true
    property alias badge: statusBadge
+    property alias identiconRing: statusIdenticonRing


As mentioned in the first comment, how about we introduce StatusIdenticonRIngSettings or, alternatively, we extend StatusIconSettings (if we do that, we need to ensure existing usages of StatusIconSettings don't break)

Then we don't need to alias statusIdenticonRing and we can reuse StatusIdenticonRingSettings in StatusMemberListItem and other component that want to leverage those properties.

0x-r4bbit · 2022-02-10T11:55:00Z

src/StatusQ/Controls/StatusIdenticonRing.qml

+                                              "#FFFFB0", "#FF5733", "#FF0000", "#9A0000", "#FF9D9D", "#FF0099",
+                                              "#C80078", "#FF00FF", "#900090", "#FFB0FF", "#9E00FF", "#0000FF",
+                                              "#000086", "#9B81FF", "#3FAEF9", "#9A6600", "#00FFFF", "#008694",
+                                              "#C2FFFF", "#00F0B6"]


How do these work across different themes? Have things like contrast ratio been taken into account?

If we know there'll be a max of 32 colors, it probably makes sense for each individual theme to define those, so it's ensured they work well in the theme.

Once that is done, these color would need to be moved to Core.Theme

Oh and they should be added to the design system figma then. We already have a color palette system there. Could introduce an additional section for "verified" colors.

Yes, it is something we should ask @John-44 because as far as I know, colored ring should be unique so don't know if it could be changed, in the perspective of identification, depending on the theme. But surely, these colores are not correct for both views, so yes, it makes more sense to have a pair for each.

@noeliaSD the colours should be the exactly the same across themes e.g. the ring colours shouldn't change when the user changes themes.

Oh and they should be added to the design system figma then. We already have a color palette system there. Could introduce an additional section for "verified" colors.

I think @davholik is working on introducing these colours into the design system file. We can keep it now just as it is and when he finishes, we can create a new task for doing the refactor! WDYT?

And he is working as well in updating these 32 colours to work througth themes without altering them to keep the same identicon ring.

If we really want to tweak the properties by theme it should be done by appropriate client's transform function, e.g. Qt.darker(identiconRingColors[0], 1.15).

So the thing with this is that it really depends on what's the impact of tweaks needed to have those colors work in different themes. Lemme elaborate:

Often times, a color in one theme can't be mapped to another color in a different theme using calculated rules (.e.g Qt.darker(...)). In those cases, the color red in light mode isn't just the same red in a different shade in darkmode, it's actually a different red. We have a good amount of such cases in our color palette which is why our Theme system is so explicit.

I think there's plans to eventually support custom themes, so users will be able to install themes for Status Desktop, in which case the colors provided by status-go might not go well with the theme at all.

I think we have a handful of options with their trade-offs:

We make color rings look exactly the same regardless of what the theme looks like (proposed by @John-44), at the cost of it possibly not being pleasant to the eye in any other theme that wasn't designed with those colors in mind (even in darkmode already those colors look too sharp, which is probably why @davholik wants to tweak them on a per theme basis).

We let the theme decide what those ring colors are, ensuring all colors work well in every theme, however, at the cost of the rings to be hard to recognize/verify across themes (as they might look different)

We drop the idea of supporting custom themes in the future entirely, at which point we'll only need to support light and darkmode colors which can then be either defined in our themes on the client side, or come from status-go (doesn't really matter)

The argument that this can cause inconsistencies across apps (desktop, mobile, etc..) is a good one. Which probably means we'll have to go with 1) or 3)

Often times, a color in one theme can't be mapped to another color in a different theme using calculated rules (.e.g Qt.darker(...)).

Yes, that's obvious, but it is not what I meant. I put it in the context, what I wanted to state is:
All colors should come from status-go (and so be the same for every theme) ONLY THEN

If we really want to tweak the properties by theme it should be done by appropriate client's transform function, e.g. Qt.darker(identiconRingColors[0], 1.15).

EDIT:
I am aware that transform functions are not ideal and will bring complexity/platform inconsistency, but I simply see no other way to keep base colors the same and yet make them look good on every theme. Because of that, I would avoid any transformations on identicons' colors palette. Maybe we could come up with a universal color set that will look acceptable on every theme.

We make color rings look exactly the same regardless of what the theme looks like (proposed by @John-44), at the cost of it possibly not being pleasant to the eye in any other theme that wasn't designed with those colors in mind (even in darkmode already those colors look too sharp, which is probably why @davholik wants to tweak them on a per theme basis).

I opt for this one. Identicon ring is a mechanism for visual Identity representation, it is meant to be consumed subconsciously by the Users. In my opinion, if colors will differ per Theme human brain will not handle it.

Taking into account all your comments and since we still have some solutions open, I suggest the following:

To make StatusIdenticonRing the most flexible StatusQ component, we can expose the distinctiveColors property as a required property to be set by the consumer. Then we can close the component itself as it is.

To manage colors/themes, we can now add the list of colors (all 32 defined in figma) in the Themes (now the same for light and dark) and when we use the component in the demo app, just like we are configuring the model in case of ringSpecsModel property, we can set up an array of the 32 colors (using Themes) we've now defined. So, we will have prepared the definition of colors in different themes (just waiting for the dark mode updates) and we will leave the door open to give the responsibility to the backend to decide which colors are passed to the ring.

WDYT?

Sounds like a good "best-of-both-worlds" solution - I like it

iurimatias · 2022-02-10T22:36:09Z

it looks like blurred, is this an artifact of the screenshot?

noeliaSD · 2022-02-11T09:44:27Z

Pushed some updates adding a ring settings object. Please review @alexandraB99 and @PascalPrecht!

noeliaSD · 2022-02-11T10:02:00Z

it looks like blurred, is this an artifact of the screenshot?

I'm not sure.. It is true that I can see a considerable difference between looking at it in my laptop screen (that it is perfectly defined) but, in the other support screen I have, it seems the pixels are more visible. What about you @alexandraB99 @PascalPrecht ?

Here a test with a ring px size of 2 (instead of the previous one that is 1.5):

Do you appreciate any improvement?

alexandraB99

Looks good to me! (don't hate me for this please :) ) should we already add documentation?

noeliaSD · 2022-02-14T08:20:14Z

Looks good to me! (don't hate me for this please :) ) should we already add documentation?

:) I think it is the best way! Yes I'll check how to do it and add docu, as well, here!

0x-r4bbit · 2022-02-14T08:52:42Z

src/StatusQ/Components/StatusSmartIdenticon.qml

@@ -20,6 +22,13 @@ Loader {
        height: 40
    }

+    property StatusIdenticonRingSettings ringSettings: StatusIdenticonRingSettings {
+        ringSpec: undefined


Just a nitpic: There's no need to define it as undefined here. Leave it out and it'll be undefined as well.

0x-r4bbit · 2022-02-14T08:54:45Z

Changes look good to me now! Just need to clarify #553 (comment)

osmaczko

Looks great, good job! There are only few small things that may be improved, see comments below.

osmaczko · 2022-02-14T13:50:57Z

sandbox/controls/ListItems.qml

@@ -357,6 +357,17 @@ CExPynn1gWf9bx498P7/nzPcxEzGExhBdJGYihtAYQlO+tUZvqrPbqeudo5iJGEJjCE15a3VtodH3q2I
                      nzPcxEzGExhBdJGYihtAYQlO+tUZvqrPbqeudo5iJGEJjCE15a3VtodH3q2ImYgiNITTlTdG1nUZ5a92VITQxITFiJmIIjSE0htAYQrMHAAD//+wwFVpz+yqXAAAAAElFTkSuQmCC"
        image.isIdenticon: true
        isOnline: true
+        ringSettings.ringSpec:
+            ListModel {
+                ListElement {colorId: 13; segmentType: 5}


I would rename it to segmentLength, because it is more readable than segmentType, which yells - "I am enum value" 😛

osmaczko · 2022-02-14T13:53:21Z

sandbox/controls/ListItems.qml

@@ -357,6 +357,17 @@ CExPynn1gWf9bx498P7/nzPcxEzGExhBdJGYihtAYQlO+tUZvqrPbqeudo5iJGEJjCE15a3VtodH3q2I
                      nzPcxEzGExhBdJGYihtAYQlO+tUZvqrPbqeudo5iJGEJjCE15a3VtodH3q2ImYgiNITTlTdG1nUZ5a92VITQxITFiJmIIjSE0htAYQrMHAAD//+wwFVpz+yqXAAAAAElFTkSuQmCC"
        image.isIdenticon: true
        isOnline: true
+        ringSettings.ringSpec:
+            ListModel {


A identicon ring MUST be composed of 5 color segments

According to feature spec, it should contain 5 elements - please consider updating models in demoapp to better reflect the future. Btw. it is great that StatusIdenticonRing is universal and can handle any number of elements!

osmaczko · 2022-02-14T14:02:24Z

src/StatusQ/Core/StatusIdenticonRingSettings.qml

+QtObject {
+    id: statusIdenticonRingSettings
+
+    property var ringSpec // Expected ListModel object to iterate through ring segments


Suggested change

property var ringSpec // Expected ListModel object to iterate through ring segments

property var ringSpecModel // [ListElement {colorId: 0; segmentLength: 1}, ...]

Wouldn't it be more readable than a plain text comment? Also added Model postfix to the name of the variable.

I like it! :)

osmaczko · 2022-02-14T14:08:37Z

src/StatusQ/Controls/StatusIdenticonRing.qml

+
+    readonly property real segmentRadLen: 2 * Math.PI / settings.totalRingUnits
+    // 32 distinctive segment: They are expected to be used using the array index from 0 to 31 and with the order defined in figma (it should be defined as well in specs.)
+    readonly property var distinctiveColors: ["#000000", "#726F6F", "#C4C4C4", "#E7E7E7", "#FFFFFF", "#00FF00",


I would NOT make it read-only. Clients of StatusQ may want to define their own colors.

osmaczko · 2022-02-14T14:12:10Z

src/StatusQ/Controls/StatusIdenticonRing.qml

+                                              "#000086", "#9B81FF", "#3FAEF9", "#9A6600", "#00FFFF", "#008694",
+                                              "#C2FFFF", "#00F0B6"]
+
+    function printArcSegment(context, xPos, yPos, radius, color, startAngle, arcAngleLen, lineWidth) {


It does not make much sense to expose it as a public API. Please consider putting this private function into:

QtObject { id: d function printArcSegment(...) {...} }

id: d - detail

or defining it inside Canvas component.

noeliaSD · 2022-02-15T14:57:24Z

Pushed all changes following your comments. I've also added documentation for the new components. Please @alexandraB99, @PascalPrecht and @osmaczko, take a look!!

0x-r4bbit

Very cool, also nice that it has docs!

osmaczko

Great docs!

…tIdenticon Created new control `StatusIdenticonRing` and used in `StatusSmartIdenticon` component. Added property assignments in sandbox models to display the `StatusIdenticonRing` when needed. Added first documentation approach for `StatusIdenticonRing` and `StatusIdenticonRingSettings`. Closes #517

noeliaSD requested review from 0x-r4bbit, alexandraB99 and osmaczko February 9, 2022 11:23

alexandraB99 reviewed Feb 9, 2022

View reviewed changes

noeliaSD force-pushed the feat/issue-517 branch from f13ad65 to f89881a Compare February 10, 2022 07:34

0x-r4bbit suggested changes Feb 10, 2022

View reviewed changes

iurimatias mentioned this pull request Feb 10, 2022

Use Emoji Hash & Identicon Ring in Desktop status-im/status-desktop#4782

Closed

3 tasks

noeliaSD force-pushed the feat/issue-517 branch from f89881a to 6f1f372 Compare February 11, 2022 09:43

alexandraB99 reviewed Feb 11, 2022

View reviewed changes

0x-r4bbit reviewed Feb 14, 2022

View reviewed changes

osmaczko suggested changes Feb 14, 2022

View reviewed changes

noeliaSD force-pushed the feat/issue-517 branch 3 times, most recently from bf42f87 to 5d8bd38 Compare February 15, 2022 14:53

0x-r4bbit approved these changes Feb 16, 2022

View reviewed changes

osmaczko approved these changes Feb 16, 2022

View reviewed changes

noeliaSD force-pushed the feat/issue-517 branch from 5d8bd38 to 83c2082 Compare February 17, 2022 08:18

noeliaSD merged commit 3b86d13 into master Feb 17, 2022

noeliaSD deleted the feat/issue-517 branch February 17, 2022 08:20

	property var ringSpec // Expected ListModel object to iterate through ring segments
	property var ringSpecModel // [ListElement {colorId: 0; segmentLength: 1}, ...]

feat(@StatusSmartIdenticon): Add support for color rings in StatusSmartIdenticon #553

feat(@StatusSmartIdenticon): Add support for color rings in StatusSmartIdenticon #553

Conversation

noeliaSD commented Feb 9, 2022

What does the PR do

Affected areas

Screenshot of functionality

alexandraB99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x-r4bbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noeliaSD Feb 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osmaczko Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iurimatias commented Feb 10, 2022

noeliaSD commented Feb 11, 2022

noeliaSD commented Feb 11, 2022

alexandraB99 left a comment

Choose a reason for hiding this comment

noeliaSD commented Feb 14, 2022

Choose a reason for hiding this comment

0x-r4bbit commented Feb 14, 2022

osmaczko left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noeliaSD commented Feb 15, 2022

0x-r4bbit left a comment

Choose a reason for hiding this comment

osmaczko left a comment

Choose a reason for hiding this comment

noeliaSD Feb 14, 2022 •

edited

Loading

osmaczko Feb 15, 2022 •

edited

Loading

osmaczko left a comment •

edited

Loading