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

Display verification status as a badge atop the user profile icon #244

Merged
merged 19 commits into from
Dec 18, 2024

Conversation

aaravlu
Copy link
Contributor

@aaravlu aaravlu commented Nov 8, 2024

@aaravlu aaravlu marked this pull request as ready for review November 9, 2024 12:05
@tyreseluo tyreseluo added the waiting-on-review This issue is waiting to be reviewed label Nov 11, 2024
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Nice job so far! Visually, the effect looks great, and that's almost exactly what I was hoping for.

Responding to your questions:

What should be transparent is not transparent, instead, what should not be transparent is transparent. What should I do to correct it?

Hmm, not exactly sure what you mean by this, because everything looks correct to me?

How can I centre the svg? I have debuged this for a long time but no idea.

The verification badge icon looks to be properly positioned, as it should ideally be on the upper right corner of the user profile image. So it looks like you're doing things correctly already, not sure why you would want to center it.


Now that I see the visual effect, I think we should probably switch to a different design for the icon. Instead of an outline of a shield that is transparent in the middle, we should switch to a shield icon that has a solid color fill (green/red/gray), with a solid-white inner checkmark / exclamation point / question mark, as that will be is easier for the user to read.


While the visual design is good, the code itself needs some improvement. I have left inline comments about that.
The main problem is that we should be using Makepad's Actions feature to inform the SDock widget about changes in Verification state. Actions are perfect for any scenario where you need to deliver a "message" to another widget. You definitely should not be using a shared RwLock for this purpose, as we do not want the main UI thread to ever have to acquire any locks.

src/home/spaces_dock.rs Outdated Show resolved Hide resolved
src/home/spaces_dock.rs Outdated Show resolved Hide resolved
src/verification.rs Outdated Show resolved Hide resolved
src/home/spaces_dock.rs Outdated Show resolved Hide resolved
src/verification.rs Outdated Show resolved Hide resolved
@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Nov 11, 2024
@aaravlu
Copy link
Contributor Author

aaravlu commented Nov 12, 2024

Now that I see the visual effect, I think we should probably switch to a different design for the icon. Instead of an outline of a shield that is transparent in the middle, we should switch to a shield icon that has a solid color fill (green/red/gray), with a solid-white inner checkmark / exclamation point / question mark, as that will be is easier for the user to read.

This is equal to What should be transparent is not transparent, instead, what should not be transparent is transparent., my english is not good.

@aaravlu aaravlu force-pushed the fix241 branch 2 times, most recently from 6da7642 to f4ffba5 Compare November 12, 2024 11:32
@aaravlu
Copy link
Contributor Author

aaravlu commented Nov 12, 2024

It seems that there is a bug:

#[rust(VerificationState::Unverified)]
verification_state: VerificationState,

The verification_state will be changed to its default value VerificationState::Unverified once app window was resized to mobile layout.
I also tried to set #[rust(VerificationState::Unverified)] to #[rust(VerificationState::Unknown)] , also in the expectation, the icon become the ? with gray.

Here is the screenshot:
Screencast from 2024-11-12 19-09-02.webm

I will send the log video to wechat cahnnel because it is a little large.

@kevinaboos
Copy link
Member

Now that I see the visual effect, I think we should probably switch to a different design for the icon. Instead of an outline of a shield that is transparent in the middle, we should switch to a shield icon that has a solid color fill (green/red/gray), with a solid-white inner checkmark / exclamation point / question mark, as that will be is easier for the user to read.

This is equal to What should be transparent is not transparent, instead, what should not be transparent is transparent., my english is not good.

oh ok, great, then we agree! Sorry for the misunderstanding.

@kevinaboos
Copy link
Member

It seems that there is a bug:

#[rust(VerificationState::Unverified)]
verification_state: VerificationState,

The verification_state will be changed to its default value VerificationState::Unverified once app window was resized to mobile layout.
I also tried to set #[rust(VerificationState::Unverified)] to #[rust(VerificationState::Unknown)] , also in the expectation, the icon become the ? with gray.
Here is the screenshot: Screencast from 2024-11-12 19-09-02.webm

I will send the log video to wechat cahnnel because it is a little large.

Well yes, this isn't a bug, it's just a missing part of your current implementation in this PR so far. This behavior is expected, since you haven't dealt with that case yet.

To fix this, you need to make sure the state of the verification badge is preserved when transitioning between the Desktop and Mobile views. If you're unsure how this works, I'd recommend reaching out to Julian, who wrote that code initially (or perhaps another intern can help you out).

src/home/spaces_dock.rs Outdated Show resolved Hide resolved
src/home/spaces_dock.rs Outdated Show resolved Hide resolved
src/home/spaces_dock.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
@alanpoon
Copy link
Contributor

Looks ok now. You can just do some code cleanup. Revert back all those non-related changed.

@alanpoon alanpoon requested a review from kevinaboos November 22, 2024 05:43
@alanpoon alanpoon added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Nov 22, 2024
@ZhangHanDong
Copy link
Contributor

ZhangHanDong commented Nov 22, 2024

From the code, it seems that the feature of displaying corresponding text on mouse hover has not been implemented yet ?

Then, when the user hovers over the badge/profile picture, we should display a tooltip that states the verification status, which is either "Verification unknown", "Unverified", or "Verified".

@ZhangHanDong ZhangHanDong added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Nov 22, 2024
src/verification.rs Outdated Show resolved Hide resolved
src/verification.rs Outdated Show resolved Hide resolved
@ZhangHanDong ZhangHanDong added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Nov 25, 2024
src/home/spaces_dock.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/home/spaces_dock.rs Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/app.rs Outdated Show resolved Hide resolved
src/home/spaces_dock.rs Outdated Show resolved Hide resolved
src/home/spaces_dock.rs Outdated Show resolved Hide resolved
src/verification.rs Outdated Show resolved Hide resolved
src/home/spaces_dock.rs Outdated Show resolved Hide resolved
src/home/spaces_dock.rs Outdated Show resolved Hide resolved
@kevinaboos kevinaboos removed the waiting-on-review This issue is waiting to be reviewed label Nov 25, 2024
@aaravlu
Copy link
Contributor Author

aaravlu commented Dec 17, 2024

Is this the latest behavior? You just posted a video with no comment, so I'm not sure what the point is.

If you're trying to illustrate that changing the adaptive view layout causes the verification state to be reset, then yes, you're correct. That just means you haven't properly handled the transfer of that widget state from the desktop UI instance of that widget to the mobile UI instance of that widget, or vice versa.

Thanks for reviewing.
The screenshot above wants to show the tooltip when user cursor is hovering the icon.
Verification state being resetted is a bug indeed, but the bug is not belong to this pr, it seems that some people is fixing it in makepad.
Now I have added some docs & done some little fixs.
Lmt if any style of the tooltip is not suitable.

@aaravlu aaravlu added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Dec 17, 2024
@aaravlu aaravlu requested a review from kevinaboos December 17, 2024 02:40
@kevinaboos
Copy link
Member

The screenshot above wants to show the tooltip when user cursor is hovering the icon.
Lmt if any style of the tooltip is not suitable.

Ok, if that's the point, then yes, I'd say to not use a transparent tooltip (that's very unusual) and just keep it simple and standard with a solid background. Also, the tooltip should be shown to the right of the verification badge icon, vertically aligned with the badge icon itself.

Verification state being resetted is a bug indeed, but the bug is not belong to this pr, is seems that some people is fixing it in makepad.

While it is technically possible for AdaptiveView to deal with this, I don't know if it ever will do that. Instead of relying on Makepad, you should explicitly obtain the current verification state when the widget is first loaded. You can do this by implementing the after_new_from_doc function from the LiveHook trait, which gets called by Makepad right after a Widget has been created.

You can get the verification state by doing:

    let current_verification_state = client.encryption().verification_state.get();

@aaravlu aaravlu added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Dec 17, 2024
@aaravlu
Copy link
Contributor Author

aaravlu commented Dec 17, 2024

Screencast from 2024-12-17 13-44-40.webm
As you can see, for the mobile layout, I found the the widget Profile(U) is not a linear change at the bottom spacedock when resizing the window while keeping app in mobile layout.
I try my best to simulate a relative position to "follow the the widget Profile(U)".
Lmk if there are any other better way to make the tooltip follow widget Profile(U).

@aaravlu aaravlu added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Dec 17, 2024
@kevinaboos
Copy link
Member

Screencast from 2024-12-17 13-44-40.webm As you can see, for the mobile layout, I found the the widget Profile(U) is not a linear change at the bottom spacedock when resizing the window while keeping app in mobile layout. I try my best to simulate a relative position to "follow the the widget Profile(U)". Lmk if there are any other better way to make the tooltip follow widget Profile(U).

I'm not sure how to fix that, but it looks quite good for now!

Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Things are looking quite good! Just left a few small comments.

src/shared/verification.rs Outdated Show resolved Hide resolved
src/home/spaces_dock.rs Outdated Show resolved Hide resolved
src/shared/verification.rs Outdated Show resolved Hide resolved
src/home/spaces_dock.rs Outdated Show resolved Hide resolved
src/shared/verification.rs Outdated Show resolved Hide resolved
@kevinaboos kevinaboos added waiting-on-author This issue is waiting on the original author for a response and removed waiting-on-review This issue is waiting to be reviewed labels Dec 17, 2024
@aaravlu aaravlu requested a review from kevinaboos December 18, 2024 02:18
@aaravlu aaravlu added waiting-on-review This issue is waiting to be reviewed and removed waiting-on-author This issue is waiting on the original author for a response labels Dec 18, 2024
Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

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

Looks great now, thanks very much!

By the way, in the future it'd be nice if you could provide more detailed commit messages than just "Fix", as it makes it easier for me to understand what changed in each commit. See some of my commit messages if you need examples/inspiration. Thanks!

@kevinaboos kevinaboos merged commit db88316 into project-robius:main Dec 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-review This issue is waiting to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants