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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add visibility icons to statuses #901

Merged
merged 9 commits into from Nov 28, 2018

Conversation

Projects
None yet
5 participants
@wbrawner
Contributor

wbrawner commented Oct 26, 2018

Hi! 馃憢

I realize this isn't a part of standard Mastodon though I'm a fan of the features requested in #835 and the data is already there so it may as well be used, right? I'm open to any suggestions for improvements here. Screenshots below:

EDIT: I just realized that devices that have modified the system font size end up with really bad alignment with the icon/text. I'll work on fixing that but I'd appreciate any comments/concerns in the meantime :) Also CI doesn't seem to be happy so I'll fix whatever I broke :P

image
image

@connyduck

This comment has been minimized.

Member

connyduck commented Oct 26, 2018

Im fine with having this on the detailed status, but not in timelines. There is already not enough space for everything in this one line.

@charlag

This comment has been minimized.

Collaborator

charlag commented Oct 26, 2018

Same here

@wbrawner

This comment has been minimized.

Contributor

wbrawner commented Oct 27, 2018

Yeah, it does seem a bit cluttered. I went ahead and pulled it out of the timeline statuses. I also implemented dynamic scaling of the visibility icon based on the font size of the timestamp, so that should look a lot better now on devices where users have adjusted the system font size.

EDIT: Also, tests are passing for me locally, so I'm not sure what I've broken on CI. Any help with that would be appreciated.

@charlag

Hi! Thanks for this nice PR, I think it will be a really nice addition.

There are some minor code things I believe we can improve.
Also I think it would be better if visiblity icon had the same height as text.

screenshot_tusky_20181027-181952

@@ -70,6 +71,16 @@ data class Status(
}
}
fun icon(): Int {

This comment has been minimized.

@charlag

charlag Oct 27, 2018

Collaborator

I know that there's a similar fun above, but to me it look more idiomatic as computed property

This comment has been minimized.

@charlag

charlag Oct 27, 2018

Collaborator

Wait, I think it doesn't belong here at all. This is a model, it doesn't care about some Android view IDs. It could live it StatusViewData but if it's used only in holder, maybe it should be there.

@@ -303,20 +303,37 @@
</android.support.constraint.ConstraintLayout>
<TextView
android:id="@+id/status_timestamp_info"
<RelativeLayout

This comment has been minimized.

@charlag

charlag Oct 27, 2018

Collaborator

RelativeLayout is always a bad idea (they are very expensive and deprecated), when we need a complex layout we use a ConstraintLayout but here it would work just as a TextView with android:drawableStart.

@wbrawner

This comment has been minimized.

Contributor

wbrawner commented Oct 27, 2018

I moved the logic for selecting the icon into the setStatusVisibility() method, since it's most likely the only place it'll be used as far as I can tell. I also had to migrate setStatusVisibility() into the StatusDetailedViewHolder class since I no longer was able to check for the existence of the ImageView. I used TextView.getTextSize() for the height of the icon, and it seems to be closer, but still off by a couple of pixels. If you have any suggestions for how to get it pixel-perfect I'd appreciate it! Ah, and one more thing. I don't see any way to attach the contentDescription to the icon (though I'm also not sure if screen readers will pick up on it anymore). If there's something I'm missing, I'd love to know about it, otherwise I'll just take out the strings I added since they're not used anymore.

@charlag

This comment has been minimized.

Collaborator

charlag commented Oct 27, 2018

Thanks for the changes! I'll look up how to make drawable of the same size.

@wbrawner

This comment has been minimized.

Contributor

wbrawner commented Oct 31, 2018

I've been doing some research into this myself as well, and I think that EditText#getTextSize() is as close as we'll get with the sizing of the drawable. From my tinkering around with it, it seems that the little bit of extra space on the top that the drawable extends past is still considered as the text height, as accents and umlauts extend up into that space on capitalized letters.

@charlag

This comment has been minimized.

Collaborator

charlag commented Oct 31, 2018

@wbrawner extra space is not a problem, I think it's as intended.
Another option would be taking sp and calculating text size by hand.
Here are some options: https://stackoverflow.com/questions/2069810/how-to-assign-text-size-in-sp-value-using-java-code

@wbrawner

This comment has been minimized.

Contributor

wbrawner commented Oct 31, 2018

Another option would be taking sp and calculating text size by hand

I think that might be a bit of unnecessary extra work. According to the docs for getTextSize(), it should already be returning the size in pixels of the text, which is what I need to give to the drawable for its size. At any rate, I'll give it a shot and see if it gives me anything different from getTextSize()

@charlag

This comment has been minimized.

Collaborator

charlag commented Oct 31, 2018

It just may be easier than setting a listener etc but honestly I don't mind, it's not a timeline.

@wbrawner

This comment has been minimized.

Contributor

wbrawner commented Oct 31, 2018

Ah, I see. Yeah the listener was necessary to wait until the TextView had already been drawn, but I can see if the sp -> px conversion is any simpler without losing accuracy

@wbrawner

This comment has been minimized.

Contributor

wbrawner commented Nov 6, 2018

I was already using getTextSize(), and it seems to be the only way to get the size of the text (you can't get the sp directly, have to calculate it from getTextSize()), but I was mistaken in thinking that you needed to wait until the view had been layed out to use it, so I've removed the LayoutListener and it still seems to work just fine.

@mal0ki

This comment has been minimized.

Collaborator

mal0ki commented Nov 14, 2018

Are we still waiting for changes on this?

@charlag

This comment has been minimized.

Collaborator

charlag commented Nov 15, 2018

I may have a look again but I think that w need decision by @connyduck

@connyduck

looks good & works fine, I like it

}
if (visibilityIcon == 0) {
return;

This comment has been minimized.

@connyduck

connyduck Nov 15, 2018

Member

you could just default: return above and save these three lines

@wbrawner

This comment has been minimized.

Contributor

wbrawner commented Nov 27, 2018

Fixed. Sorry for the delay, been a bit busy lately :)

@connyduck

This comment has been minimized.

Member

connyduck commented Nov 28, 2018

thx!

@connyduck connyduck merged commit 95a656d into tuskyapp:master Nov 28, 2018

1 of 2 checks passed

ci/circleci: test Your tests failed on CircleCI
Details
ci/bitrise/a3e773c3c57a894c/pr Passed - Tusky
Details

kyori19 added a commit to accelforce/Tusky that referenced this pull request Dec 15, 2018

@kyori19

This comment has been minimized.

Contributor

kyori19 commented Dec 16, 2018

I think this is mistake.
It should be ic_lock_outline, not ic_lock_open.

visibilityIcon = R.drawable.ic_lock_open_24dp;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment