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

List tabs #1570

Merged
merged 16 commits into from Dec 2, 2019
Merged

List tabs #1570

merged 16 commits into from Dec 2, 2019

Conversation

@kyori19
Copy link
Contributor

kyori19 commented Nov 24, 2019

image
image
image
image

Add ability to add list timelines to tabs.

createTabDataFromId(data[0], data.drop(1))
createTabDataFromId(data[0], data.drop(1).map { s -> URLDecoder.decode(s, "UTF-8") })
}
}

@TypeConverter
fun tabDataToString(tabData: List<TabData>?): String? {
return tabData?.joinToString(";") { it.id + ":" + it.arguments.joinToString(":") }
return tabData?.joinToString(";") { it.id + ":" + it.arguments.joinToString(":") { s -> URLEncoder.encode(s, "UTF-8") } }
Comment on lines 64 to 72

This comment has been minimized.

Copy link
@kyori19

kyori19 Nov 24, 2019

Author Contributor

List name may includes ":".

This comment has been minimized.

Copy link
@connyduck

connyduck Nov 26, 2019

Member

put that into an comment in the code please

Copy link
Member

connyduck left a comment

Very nice 😍
This feature is so often requested

createTabDataFromId(data[0], data.drop(1))
createTabDataFromId(data[0], data.drop(1).map { s -> URLDecoder.decode(s, "UTF-8") })
}
}

@TypeConverter
fun tabDataToString(tabData: List<TabData>?): String? {
return tabData?.joinToString(";") { it.id + ":" + it.arguments.joinToString(":") }
return tabData?.joinToString(";") { it.id + ":" + it.arguments.joinToString(":") { s -> URLEncoder.encode(s, "UTF-8") } }

This comment has been minimized.

Copy link
@connyduck

connyduck Nov 26, 2019

Member

put that into an comment in the code please

if (list != null) {
val title = view.hashtag
title.text = list.title
val icon = context.getDrawable(R.drawable.ic_list)

This comment has been minimized.

Copy link
@connyduck

connyduck Nov 26, 2019

Member

The icon should have the same color as the text. You can use ThemeUtils.getTintedDrawable, or IconicsDrawable, see here
https://github.com/tuskyapp/Tusky/blob/develop/app/src/main/java/com/keylesspalace/tusky/ListsActivity.kt#L237-L238

@@ -61,13 +63,13 @@ class Converters {
return str?.split(";")
?.map {
val data = it.split(":")
createTabDataFromId(data[0], data.drop(1))
createTabDataFromId(data[0], data.drop(1).map { s -> URLDecoder.decode(s, "UTF-8") })

This comment has been minimized.

Copy link
@connyduck

connyduck Nov 26, 2019

Member

Can you try moving the url decoding somewhere else, so it doesn't run for tabs that don't need it? E.g. to TabData.createTabDataFromId


if (convertView == null) {
val layoutInflater = context.getSystemService(Context.LAYOUT_INFLATER_SERVICE) as LayoutInflater
view = layoutInflater.inflate(R.layout.item_autocomplete_hashtag, parent, false)

This comment has been minimized.

Copy link
@connyduck

connyduck Nov 26, 2019

Member

I think its better to use a dedicated layout here. No bold text, more spacing between the CompoundDrawable and the text.

@kyori19

This comment has been minimized.

Copy link
Contributor Author

kyori19 commented Nov 29, 2019

@connyduck All done!

@@ -67,7 +68,8 @@ class Converters {

@TypeConverter
fun tabDataToString(tabData: List<TabData>?): String? {
return tabData?.joinToString(";") { it.id + ":" + it.arguments.joinToString(":") }
// List name may include ":"
return tabData?.joinToString(";") { it.id + ":" + it.arguments.joinToString(":") { s -> URLEncoder.encode(s, "UTF-8") } }

This comment has been minimized.

Copy link
@connyduck

connyduck Nov 29, 2019

Member

nah, please remove the url encoding from here, it will run for all tabs.

kyori19 and others added 4 commits Nov 30, 2019
Co-Authored-By: Konrad Pozniak <connyduck@users.noreply.github.com>
Co-Authored-By: Konrad Pozniak <connyduck@users.noreply.github.com>
Copy link
Collaborator

charlag left a comment

I went through it and besides already mentioned things didn't find any issues. Great job, it's impressive that you could get away with so little code (and it's a good thing!)

kyori19 added 2 commits Nov 30, 2019
@kyori19

This comment has been minimized.

Copy link
Contributor Author

kyori19 commented Nov 30, 2019

@charlag Thanks!

@connyduck
I moved the decoder, but it's a bit ugly.
Do you have any other good ideas?

@connyduck

This comment has been minimized.

Copy link
Member

connyduck commented Dec 1, 2019

Hmm this sucks 🤔
You could make TabData an open class and override the arguments for lists, but thats also quite ugly. And there is probably no other delimiter we could use, so its propably best to prefer simpler code over performance. Can you move the url en/decoding back into the converter? Sorry for the back&forth

kyori19 added 4 commits Dec 2, 2019
This reverts commit fdc45aa.
This reverts commit 704b4e6.
This reverts commit d1cd207.
@kyori19

This comment has been minimized.

Copy link
Contributor Author

kyori19 commented Dec 2, 2019

@connyduck
Reverted all changes.

@connyduck connyduck merged commit d6ae071 into tuskyapp:develop Dec 2, 2019
1 check passed
1 check passed
ci/bitrise/a3e773c3c57a894c/pr Passed - Tusky
Details
@kyori19 kyori19 deleted the accelforce:list-tabs branch Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.