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

Issue #1441 - Smaller radius for single note widget #1588

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abdusalamApps
Copy link

@abdusalamApps abdusalamApps commented Oct 8, 2022

This is a pull request for the change requested in issue 1441 in the original repo

@jancborchardt
Copy link
Member

@abdusalamApps could you add before/after screenshots for easier design review? Thank you! :)

@stefan-niedermann
Copy link
Member

I am afraid that this only reduces the border radius ignoring that newer Android versions will hardly crop the border radius furth - leading to unoredictable states...

@jancborchardt
Copy link
Member

@abdusalamApps could you also make sure to test on newer Android versions as @stefan-niedermann mentioned? Thanks! :)

@abdusalamApps
Copy link
Author

@jancborchardt I'll do that the first chance I get!

@abdusalamApps
Copy link
Author

abdusalamApps commented Jan 25, 2023

@jancborchardt @stefan-niedermann
It looks like my change doesn't affect Android 13.0 API 33. See screenshots below:

Android 13.0 API 33, before and after

Android 13 0 API 33 - Before Android 13 0 API 33 - After

Android 9.0 API 28, before and after

Androdi 9 0 API 28 - Before Android 9 0 API 28 - After

@jancborchardt
Copy link
Member

So then considering @stefan-niedermann’s feedback I’m not sure we should merge this. It’s better if we just follow the guidelines as closely as possible. :\

@abdusalamApps
Copy link
Author

@jancborchardt Could you elaborate on what guide lines to which you're referring?

@stefan-niedermann
Copy link
Member

what guide lines

As linked in the issue:

https://developer.android.com/about/versions/12/features/widgets

@AndyScherzinger AndyScherzinger force-pushed the 1441-signle-note-widget-card-radius-too-big branch from e8689dd to bb74ff1 Compare January 21, 2024 15:51
@AndyScherzinger
Copy link
Member

Rebased due to major technical updates in terms of libraries, especially AGP and SSO

@AndyScherzinger AndyScherzinger force-pushed the 1441-signle-note-widget-card-radius-too-big branch from bb74ff1 to c92cc53 Compare March 7, 2024 19:55
@newhinton
Copy link
Contributor

newhinton commented Apr 1, 2024

@stefan-niedermann But the linked docs state that system_app_widget_background_radius should be used. I have multiple devices (GrapheneOS, LineageOS) where the notes-widget uses a different radius than every other tested app.

As you can see the the notes app uses a wider radius. This becomes especially clear when you try to move the widget and the boundaries are shown. (second notes-widget)

The KDE one is also slightly wider than the boundary, but it is imperceptible except for "moving" it where a tiny gap is shown. The wikipedia-one is completely off the rails anyway, but it still uses the correct radius.

I don't know if the proposed solution is the right one, but the current choosen radius is most certainly not the proper one.

@newhinton
Copy link
Contributor

newhinton commented Apr 1, 2024

I just checked it:

<dimen name="spacer_4x">36dp</dimen>

This clearly violates the guideline set by google: "[...] the corner radius of the widget background, which is never larger than 28 dp."

@stefan-niedermann
Copy link
Member

I agree, but spacer_2x is only 16dp. I would set 28dp as hard coded value for widget_outer_radius and put a link to the link above as comment above the entry.

@newhinton
Copy link
Contributor

Why not follow the suggestion by the docs?

Use a default value (28dp like you suggested) for skd 30 and below, and

<item name="backgroundRadius">@android:dimen/system_app_widget_background_radius</item>

for sdk 31. This should always use the proper radius.

If you like i can create a pr for it.

@newhinton
Copy link
Contributor

Wait, sorry. It seems it is already using the proper constant. I need to investigate.

@newhinton
Copy link
Contributor

newhinton commented Apr 1, 2024

Huh?

This is weird. To me it seems that @android:dimen/system_app_widget_background_radius is creating the too-big radius in v31.

In sdk 30 and below, 28dp create the currently known radius. There is no (visible) difference between 36dp and 28dp and system_app_widget_background_radius.

The 16dp as suggested by this pr seems to be working like every other app.

KDE-Connect doesn't seem to have a radius at all. (0dp also results in a 16dp like radius cutoff)

So the question is if we want to stick out, or ignore the guideline. And it also seems that 16dp is the correct value if we want to ignore the guideline. Either way, the overall behaviour of the corners are weird.

Personally, i'd prefer the 16dp corner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants