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.

Sign up for GitHub

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

Feature/aris/threads beta feedback #5647

Merged
merged 9 commits into from
Apr 5, 2022
Merged

Feature/aris/threads beta feedback #5647

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
58cc393
Add beta feedback for threads
ariskotsomitopoulos Mar 28, 2022
a2e2cdc
Add feature specific configurations
ariskotsomitopoulos Mar 28, 2022
9eccb9e
Enable threads beta feedback
ariskotsomitopoulos Mar 28, 2022
b996e0e
Add changelog
ariskotsomitopoulos Mar 28, 2022
3ba2419
Replace hardcoded string
ariskotsomitopoulos Mar 28, 2022
b5f8d2c
Update string resource
ariskotsomitopoulos Apr 5, 2022
aef4cce
Format code ordering
ariskotsomitopoulos Apr 5, 2022
858623b
Change default threads beta visibility
ariskotsomitopoulos Apr 5, 2022
a269aa5
Remove unused elevation
ariskotsomitopoulos Apr 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5647.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Users will be able to provide feedback for threads
9 changes: 9 additions & 0 deletions library/ui-styles/src/main/res/drawable/bg_shadow_divider.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?xml version="1.0" encoding="utf-8"?>
<shape xmlns:android="http://schemas.android.com/apk/res/android"
android:shape="rectangle" >
<gradient
android:angle="-90"
android:startColor="#00000000"
android:endColor="#1A000000"
/>
</shape>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separator on dark theme is maybe not visible enough:

image

11 changes: 11 additions & 0 deletions vector-config/src/main/res/values/config-features.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?xml version="1.0" encoding="utf-8"?>
<resources>

<!-- This file contains values to show or hide some features, that are not available from
settings & labs. Values here are mainly modified by developers and are not exposed
to the users.
-->

<bool name="feature_threads_beta_feedback_enabled">true</bool>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a temporary flag, it's OK to keep it like that, but a FeatureFlag would have been better here. I am still writting the doc about it)


</resources>
11 changes: 11 additions & 0 deletions ...r/src/main/java/im/vector/app/features/home/room/threads/list/views/ThreadListFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,16 @@ import im.vector.app.features.home.room.threads.arguments.ThreadTimelineArgs
import im.vector.app.features.home.room.threads.list.viewmodel.ThreadListController
import im.vector.app.features.home.room.threads.list.viewmodel.ThreadListViewModel
import im.vector.app.features.home.room.threads.list.viewmodel.ThreadListViewState
import im.vector.app.features.rageshake.BugReporter
import im.vector.app.features.rageshake.ReportType
import org.matrix.android.sdk.api.session.room.threads.model.ThreadSummary
import org.matrix.android.sdk.api.session.room.timeline.TimelineEvent
import org.matrix.android.sdk.api.util.MatrixItem
import javax.inject.Inject

class ThreadListFragment @Inject constructor(
private val avatarRenderer: AvatarRenderer,
private val bugReporter: BugReporter,
private val threadListController: ThreadListController,
val threadListViewModelFactory: ThreadListViewModel.Factory
) : VectorBaseFragment<FragmentThreadListBinding>(),
Expand Down Expand Up @@ -80,6 +83,7 @@ class ThreadListFragment @Inject constructor(
super.onViewCreated(view, savedInstanceState)
initToolbar()
initTextConstants()
initBetaFeedback()
views.threadListRecyclerView.configureWith(threadListController, TimelineItemAnimator(), hasFixedSize = false)
threadListController.listener = this
}
Expand All @@ -101,6 +105,13 @@ class ThreadListFragment @Inject constructor(
resources.getString(R.string.reply_in_thread))
}

private fun initBetaFeedback() {
views.threadsFeedBackConstraintLayout.isVisible = resources.getBoolean(R.bool.feature_threads_beta_feedback_enabled)
views.threadFeedbackDivider.isVisible = resources.getBoolean(R.bool.feature_threads_beta_feedback_enabled)
views.threadsFeedBackConstraintLayout.debouncedClicks {
bugReporter.openBugReportScreen(requireActivity(), reportType = ReportType.THREADS_BETA_FEEDBACK)
}
}
override fun invalidate() = withState(threadListViewModel) { state ->
renderEmptyStateIfNeeded(state)
threadListController.update(state)
Expand Down
9 changes: 9 additions & 0 deletions vector/src/main/java/im/vector/app/features/rageshake/BugReportActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,15 @@ class BugReportActivity : VectorBaseActivity<ActivityBugReportBinding>() {

hideBugReportOptions()
}
ReportType.THREADS_BETA_FEEDBACK -> {
supportActionBar?.setTitle(R.string.send_feedback_threads_title)

views.bugReportFirstText.setText(R.string.send_feedback_threads_info)
views.bugReportTextInputLayout.hint = getString(R.string.feedback)
views.bugReportButtonContactMe.isVisible = true

hideBugReportOptions()
}
else -> {
// other types not supported here
}
Expand Down
10 changes: 6 additions & 4 deletions vector/src/main/java/im/vector/app/features/rageshake/BugReporter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,11 +255,12 @@ class BugReporter @Inject constructor(

if (!mIsCancelled) {
val text = when (reportType) {
ReportType.BUG_REPORT -> "[Element] $bugDescription"
ReportType.SUGGESTION -> "[Element] [Suggestion] $bugDescription"
ReportType.SPACE_BETA_FEEDBACK -> "[Element] [spaces-feedback] $bugDescription"
ReportType.BUG_REPORT -> "[Element] $bugDescription"
ReportType.SUGGESTION -> "[Element] [Suggestion] $bugDescription"
ReportType.SPACE_BETA_FEEDBACK -> "[Element] [spaces-feedback] $bugDescription"
ReportType.AUTO_UISI_SENDER,
ReportType.AUTO_UISI -> bugDescription
ReportType.AUTO_UISI -> bugDescription
ReportType.THREADS_BETA_FEEDBACK -> "[Element] [threads-feedback] $bugDescription"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker but I would have added this case just below SPACE_BETA_FEEDBACK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored!

}

// build the multi part request
Expand Down Expand Up @@ -350,6 +351,7 @@ class BugReporter @Inject constructor(
builder.addFormDataPart("label", "android")
builder.addFormDataPart("label", "uisi-sender")
}
ReportType.THREADS_BETA_FEEDBACK -> builder.addFormDataPart("label", "threads-feedback")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same remark about ordering cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

}

if (getCrashFile().exists()) {
Expand Down
3 changes: 2 additions & 1 deletion vector/src/main/java/im/vector/app/features/rageshake/ReportType.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2021 New Vector Ltd
* Copyright (c) 2022 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -20,6 +20,7 @@ enum class ReportType {
BUG_REPORT,
SUGGESTION,
SPACE_BETA_FEEDBACK,
THREADS_BETA_FEEDBACK,
AUTO_UISI,
AUTO_UISI_SENDER,
}
66 changes: 60 additions & 6 deletions vector/src/main/res/layout/fragment_thread_list.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
android:layout_width="0dp"
android:layout_height="0dp"
android:background="?android:colorBackground"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintBottom_toTopOf="@id/threadsFeedBackConstraintLayout"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@id/threadListAppBarLayout"
Expand Down Expand Up @@ -75,13 +75,13 @@
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_marginBottom="20dp"
android:gravity="center"
android:text="@string/thread_list_empty_title"
android:textColor="?vctr_content_primary"
app:layout_constraintBottom_toTopOf="@id/threadListEmptySubtitleTextView"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
android:gravity="center"
app:layout_constraintTop_toBottomOf="@id/threadListEmptyImageView"
android:text="@string/thread_list_empty_title" />
app:layout_constraintTop_toBottomOf="@id/threadListEmptyImageView" />

<TextView
android:id="@+id/threadListEmptySubtitleTextView"
Expand All @@ -90,12 +90,12 @@
android:layout_height="wrap_content"
android:layout_marginBottom="20dp"
android:gravity="center"
android:text="@string/thread_list_empty_subtitle"
android:textColor="?vctr_content_secondary"
app:layout_constraintBottom_toTopOf="@id/threadListEmptyNoticeTextView"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@id/threadListEmptyTitleTextView"
android:text="@string/thread_list_empty_subtitle" />
app:layout_constraintTop_toBottomOf="@id/threadListEmptyTitleTextView" />

<TextView
android:id="@+id/threadListEmptyNoticeTextView"
Expand All @@ -112,4 +112,58 @@


</androidx.constraintlayout.widget.ConstraintLayout>

<View
android:id="@+id/threadFeedbackDivider"
android:layout_width="0dp"
android:layout_height="4dp"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintEnd_toEndOf="parent"
android:background="@drawable/bg_shadow_divider"
app:layout_constraintBottom_toTopOf="@id/threadsFeedBackConstraintLayout"
/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid possible blink, it's better to hide those new views by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case it's the opposite, to avoid blinking we should turn them on, they are not dynamic. Feedback will be on by default

Copy link
Member

@bmarty bmarty Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. But it's worth to see the View to quickly vanish when opening the screen (if the bool was set to false), than to see the View to quickly appear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, I have something in mind that will be a solution for both

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bmarty A solution would be to use data binding conversions such as:

@BindingConversion
fun booleanToVisibility(visible: Boolean) = if(visible) View.VISIBLE else View.GONE

So then we will be able to do in our xml:
android:visibility="@{@bool/myBooleanFlag}"

However this requires to enable data binding for the whole project and introduces some boilerplate, so I will change the default to false !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that does not worth it. Also in the XML config we could have view style, defining a visibility...

<androidx.constraintlayout.widget.ConstraintLayout
android:id="@+id/threadsFeedBackConstraintLayout"
android:layout_width="0dp"
android:layout_height="wrap_content"
android:elevation="30dp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need an elevation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the shadow, however I enhanced it by adding the divider view so we don't really need that

android:padding="6dp"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent">
<TextView
android:id="@+id/threadsBetaFeedbackIcon"
style="@style/Widget.Vector.TextView.Caption"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_gravity="center"
android:background="@drawable/notification_badge"
android:backgroundTint="@color/palette_azure"
android:gravity="center"
android:paddingStart="10dp"
android:paddingTop="3dp"
android:paddingEnd="10dp"
android:paddingBottom="3dp"
android:text="@string/beta"
android:textColor="@color/palette_white"
app:layout_constraintBottom_toBottomOf="@id/threadsBetaFeedbackButton"
app:layout_constraintEnd_toStartOf="@id/threadsBetaFeedbackButton"
app:layout_constraintHorizontal_chainStyle="packed"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="@id/threadsBetaFeedbackButton" />

<TextView
android:id="@+id/threadsBetaFeedbackButton"
style="@style/Widget.Vector.TextView.Body"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:gravity="center"
android:padding="8dp"
android:text="@string/give_feedback_threads"
android:textColor="@color/vector_info_color"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toEndOf="@id/threadsBetaFeedbackIcon" />
</androidx.constraintlayout.widget.ConstraintLayout>

</androidx.constraintlayout.widget.ConstraintLayout>
4 changes: 4 additions & 0 deletions vector/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1648,6 +1648,10 @@
<string name="feedback_sent">Thanks, your feedback has been successfully sent</string>
<string name="feedback_failed">The feedback failed to be sent (%s)</string>
<string name="give_feedback">Give Feedback</string>
<string name="give_feedback_threads">Give Feedback on threads</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F -> f ?

Suggested change
<string name="give_feedback_threads">Give Feedback on threads</string>
<string name="give_feedback_threads">Give feedback on threads</string>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its with capital "F" in the designs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the current strategy is rather to avoid capitalization. Can you double check with the product please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They still want that with Capital F for that specific case, new string resource will be Give Feedback instead of Give Feedback on threads

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks for double checking.

<string name="send_feedback_threads_title">Threads Beta feedback</string>
<string name="send_feedback_threads_info">Threads are a work in progress with new, exciting upcoming features, such as improved notifications. We’d love to hear your feedback!</string>
<string name="beta">BETA</string>

<string name="settings_labs_show_hidden_events_in_timeline">Show hidden events in timeline</string>
<string name="settings_labs_show_complete_history_in_encrypted_room">"Show complete history in encrypted rooms"</string>
Expand Down