Replicate element coloring with icons #305

AbsurdlySuspicious · 2020-08-18T03:34:23Z

Fixes #301

app/src/main/res/values/userpreferences.xml

Tunous · 2020-08-18T05:30:09Z

Do we actually need a setting for this feature? These icons look very nice and don't seem to clutter the view. It seems to me like they could be always on.

AbsurdlySuspicious · 2020-08-18T11:42:02Z

I think we can just rephrase and enable it by default, something like "More icons", because I'm sure some people like cleaner look more (like me :P)

Tunous · 2020-08-18T12:50:59Z

I'm personally more into "Less settings is better" thinking nowadays in aspect of UI and UX. The argument for it is based on the fact that growing amount of possible settings increases code and UX complexity. Not only we have more code paths to maintain, we make it harder (marginally but still) for the app to define its own distinguishing style. As settings like this will grow the app will become more and more confusing for the majority of users entering settings screen.

In this case I don't see huge value in a new setting and don't see how the interface becomes cluttered with new icons, but perhaps I'm missing something 😄

But if others disagree with me then let's go with enabling it by default with changed text.

AbsurdlySuspicious · 2020-08-21T01:18:01Z

I removed preference from this branch. When time comes for the next release, if nobody else will voice their opinion, let's merge it as is.

I also added some vertical offset for icons to center them properly. Span centers them between borders of tallest and lowest glyph so with the "common case" text they was visually skewed to the top. This offset should look identical on all devices

binocry · 2020-09-02T01:12:07Z

I hope the icons are the same as reddit so users have the same feeling when moving from reddit website to this app

…ture Replicate element coloring with icons * github.com:Tunous/Dawn: Update changelog entry Remove preference gates Revert c3598f8, 9d13eca (remove preference) Add vertical offset sp to px units conversion Fix tests Changelog Add preference use centering based on font metrics OP comment icon Vote icon for comments Vote direction icon CenterAlignedImageSpan fix invalid icon name

app/src/main/java/me/saket/dank/ui/submission/SubmissionCommentTreeUiConstructor.java

app/src/main/java/me/saket/dank/widgets/span/CenterAlignedImageSpan.kt

AbsurdlySuspicious · 2020-09-10T22:02:57Z

Span centers them between borders of tallest and lowest glyph

Looks like this approach is inconsistent between different fonts and current implementation is optimal exclusively for Roboto. On the contrary, ALIGN_CENTER implementation from api 29 sdk is inconsistent between single and multi-line textviews due to extra line spacing and I'm not sure which one is better. I also tried icon font approach (using TypefaceSpan) back then and it looks terrible. Any ideas?

Tunous · 2020-09-11T10:40:52Z

I've pushed a modification where I'm passing in the extra line spacing where it's used. Not the best solution but it appears to look correctly with the fonts I've tested (didn't check all).

AbsurdlySuspicious · 2020-09-11T22:23:30Z

app/src/main/java/me/saket/dank/widgets/span/CenterAlignedImageSpan.kt

-    val b = getCachedDrawable()
+    val drawable = getCachedDrawable()
+    val drawableHeight = drawable.bounds.height()
+    val translationY = (bottom - top - drawableHeight - lineSpacingExtra) / 2f


The thing is extra line spacing is not applied to the last line of text. For some reason, it's included in bottom value (unlike ordinary line spacing) and thus it's different for last line and all other lines. So this change just "inverts" vertical skew, making icon offset negative for single-line textviews

Good to know.

Sorry, I forgot to mark this as resolved, I was still thinking what to do with it at the moment of posting this :)

AbsurdlySuspicious · 2020-09-11T23:13:17Z

app/src/main/java/me/saket/dank/widgets/span/CenterAlignedImageSpan.kt

@@ -13,7 +13,8 @@ class CenterAlignedImageSpan(drawable: Drawable, private val lineSpacingExtra: I
                    start: Int, end: Int, x: Float,
                    top: Int, y: Int, bottom: Int, paint: Paint) {
    val drawableHeight = drawable.bounds.height()
-    val translationY = (bottom - top - drawableHeight - lineSpacingExtra) / 2f
+    val realExtraSpacing = if (canvas.clipBounds.bottom != bottom) lineSpacingExtra else 0 // extra line spacing is not applied to last line


dirty but works

AbsurdlySuspicious · 2020-09-11T23:14:17Z

app/src/main/java/me/saket/dank/widgets/span/CenterAlignedImageSpan.kt

@@ -13,7 +13,8 @@ class CenterAlignedImageSpan(drawable: Drawable, private val lineSpacingExtra: I
                    start: Int, end: Int, x: Float,
                    top: Int, y: Int, bottom: Int, paint: Paint) {
    val drawableHeight = drawable.bounds.height()
-    val translationY = (bottom - top - drawableHeight - lineSpacingExtra) / 2f
+    val realExtraSpacing = if (canvas.clipBounds.bottom != bottom) lineSpacingExtra else 0 // extra line spacing is not applied to last line
+    val translationY = top + (bottom - top - drawableHeight - realExtraSpacing) / 2f


top + allows to use this span in lines other than first

Tunous

For me looks good

msfjarvis

I've been using it for a while and haven't found any obvious flaws with either the code or the actual functionality.

msfjarvis · 2020-09-13T17:08:36Z

app/src/main/java/me/saket/dank/utils/ColorReplicationIcons.kt

+
+object ColorReplicationIcons {
+  @JvmStatic fun pushIcon(context: Context, builder: Truss, sizeDimenResId: Int,
+                          drawableResId: Int, tintColor: Int, lineSpacingExtra: Int) {


not sure what the project policy is but annotating these parameters with @DrawableRes, @ColorRes and @DimenRes would probably not be a bad idea.

I'm voting for including these :)

Sorry for such a long delay, I'll address it in a separate PR

Minnona · 2020-09-23T09:28:47Z

Well, guess it's too late to comment on this. To me these arrows seem like an unnecessary duplication.

AbsurdlySuspicious commented Aug 18, 2020

View reviewed changes

app/src/main/res/values/userpreferences.xml Outdated Show resolved Hide resolved

Tunous reviewed Sep 9, 2020

View reviewed changes

AbsurdlySuspicious and others added 14 commits September 11, 2020 11:17

fix invalid icon name

b9845ae

CenterAlignedImageSpan

b36d811

Vote direction icon

f95cca3

Vote icon for comments

c7fc7e5

OP comment icon

c8d2115

use centering based on font metrics

fa1917e

Add preference

03a7d40

Fix tests

60031ac

sp to px units conversion

0d3168e

Add vertical offset

9a7114c

Revert c3598f8, 9d13eca (remove preference)

1d28e43

Remove preference gates

db98525

Update changelog entry

766327d

Improve icon centering for different fonts

688993b

Tunous force-pushed the color_icons/feature branch from 50f9a59 to 688993b Compare September 11, 2020 10:39

AbsurdlySuspicious commented Sep 11, 2020

View reviewed changes

AbsurdlySuspicious added 3 commits September 12, 2020 02:11

Use dimension resource for line spacing

dc8c05f

Remove drawable caching

a6d0d51

Don't apply extra line spacing correction to last line

05626c1

AbsurdlySuspicious commented Sep 11, 2020

View reviewed changes

Tunous approved these changes Sep 12, 2020

View reviewed changes

msfjarvis approved these changes Sep 13, 2020

View reviewed changes

Tunous added this to the 0.11.0 milestone Sep 22, 2020

Tunous merged commit 1592ad5 into Tunous:master Sep 22, 2020

AbsurdlySuspicious deleted the color_icons/feature branch January 23, 2021 05:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replicate element coloring with icons #305

Replicate element coloring with icons #305

AbsurdlySuspicious commented Aug 18, 2020

Tunous commented Aug 18, 2020

AbsurdlySuspicious commented Aug 18, 2020 •

edited

Loading

Tunous commented Aug 18, 2020

AbsurdlySuspicious commented Aug 21, 2020

binocry commented Sep 2, 2020

AbsurdlySuspicious commented Sep 10, 2020

Tunous commented Sep 11, 2020

AbsurdlySuspicious Sep 11, 2020

Tunous Sep 12, 2020

AbsurdlySuspicious Sep 12, 2020

AbsurdlySuspicious Sep 11, 2020

AbsurdlySuspicious Sep 11, 2020

Tunous left a comment

msfjarvis left a comment

msfjarvis Sep 13, 2020

Tunous Sep 15, 2020

AbsurdlySuspicious Oct 4, 2020

Minnona commented Sep 23, 2020

Replicate element coloring with icons #305

Replicate element coloring with icons #305

Conversation

AbsurdlySuspicious commented Aug 18, 2020

Tunous commented Aug 18, 2020

AbsurdlySuspicious commented Aug 18, 2020 • edited Loading

Tunous commented Aug 18, 2020

AbsurdlySuspicious commented Aug 21, 2020

binocry commented Sep 2, 2020

AbsurdlySuspicious commented Sep 10, 2020

Tunous commented Sep 11, 2020

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

Tunous left a comment

Choose a reason for hiding this comment

msfjarvis 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

Minnona commented Sep 23, 2020

AbsurdlySuspicious commented Aug 18, 2020 •

edited

Loading