notes-android icon indicating copy to clipboard operation
notes-android copied to clipboard

Issue #1441 - Smaller radius for single note widget

Open abdusalamApps opened this issue 2 years ago • 15 comments

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

abdusalamApps avatar Oct 08 '22 16:10 abdusalamApps

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

jancborchardt avatar Jan 12 '23 14:01 jancborchardt

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...

stefan-niedermann avatar Jan 12 '23 15:01 stefan-niedermann

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

jancborchardt avatar Jan 16 '23 16:01 jancborchardt

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

abdusalamApps avatar Jan 20 '23 11:01 abdusalamApps

@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

abdusalamApps avatar Jan 25 '23 15:01 abdusalamApps

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. :\

jancborchardt avatar May 15 '23 16:05 jancborchardt

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

abdusalamApps avatar Jun 03 '23 09:06 abdusalamApps

what guide lines

As linked in the issue:

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

stefan-niedermann avatar Jun 03 '23 11:06 stefan-niedermann

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

AndyScherzinger avatar Jan 21 '24 15:01 AndyScherzinger

@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 avatar Apr 01 '24 13:04 newhinton

I just checked it: https://github.com/nextcloud/notes-android/blob/cc110fcfe4f72d437c7bcd90f68a1102f68de831/app/src/main/res/values/dimens.xml#L18

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

newhinton avatar Apr 01 '24 14:04 newhinton

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.

stefan-niedermann avatar Apr 01 '24 14:04 stefan-niedermann

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 avatar Apr 01 '24 14:04 newhinton

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

newhinton avatar Apr 01 '24 14:04 newhinton

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.

newhinton avatar Apr 01 '24 14:04 newhinton