Poll Feature - Timeline #4624

onurays · 2021-12-03T08:50:26Z

Design: https://www.figma.com/file/tepA3ELWBE5v4O2WB8PqWn/Polls?node-id=247%3A62813

Users cannot see votes until they vote. But the total casted vote count is visible
Users can end poll if they have enough power level
Multiple options can win if they have the same amount of votes (bigger than zero)
Creating and rendering polls are under Labs and disabled by default

* feature/ons/poll: Design review fixes. Code review fixes. Make the poll option visible so that it can be tested from the PR Limit maximum number of poll options. Code review fixes. Fix UI issues. Remove poll command. Use unstable types. Create poll event content. Create poll UI implementation. Create poll fragment with a title. Create poll screen components implemented. Add poll icon to attachment type selector.

* feature/ons/poll: Hide poll icon from attachment type selector for now. Code review fixes. Lint fixes. Design review fixes.

* develop: (88 commits) Fix "PendingIntents attached to actions with remote inputs must be mutable" Room notifications are now working on Android 12 emulator Move P1 issues to app team and crypto boards Quick fix on this file Fix warning after rebase (it's for test, so OK to suppress warning) Update changelog, since the feature is not visible yet. Create file for Toast style (more generic) And improve fragment_create_poll.xml preview rendering Create CallToAction button style Create dedicated file for TextInputLayout styles And follow naming convention Lint fix. Add comment to run on Android 12 Use correct value, but I do not see any effect on emulator with API 12 Fix crash on Android 12. I guess we accept only images coming from the keyboard. ktlint Fix lint issue "NullSafeMutableLiveData" Fix lint issue "Incorrect constant" Fix lint issue "Outside Range" Ensure that column index is not -1 Make the Cursor extensions public And make the code more efficient, since we call getColumnIndexOrNull only once and not on each cursor iteration Fix crash on Android 12: PendingIntent.FLAG_IMMUTABLE has to be set Fix crash on Android 12 java.lang.SecurityException: To use the sampling rate of 0 microseconds, app needs to declare the normal permission HIGH_SAMPLING_RATE_SENSORS. InputConnectionCompat.createWrapper is deprecated Permission should be granted, according to https://developer.android.com/reference/android/view/OnReceiveContentListener#uri-permissions ...

DoM1niC · 2021-12-03T21:39:35Z

Circle need be colored but so far works perfectly for me :)

onurays · 2021-12-07T12:38:08Z

Circle need be colored but so far works perfectly for me :)

Good catch, fixed.

* develop: (319 commits) Bump dagger from 2.40.4 to 2.40.5 More debouncing Changelog I need the view here clicks() already has debouncing with conflate(), so throttleFirst is not necessary Use debouncedClicks where applicable Use `observeViewEvents` facility private use != rather that is Fix waring for state is not dialing or connected state can also be connected Add script to compress video and convert to gif file removing boolean notification version usage adding changelog entry removing unused imports and increasing enum allowance porting the notifications setting version to the vector features Add warning if unexpected state. setting login version via typed build config field instead of resources separating the SSO redirection from the login activities - adds a dedicated routing activity to proxy the uri to the login selected by the feature flags Removing trailing space ... # Conflicts: # vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailAction.kt # vector/src/main/java/im/vector/app/features/home/room/detail/timeline/format/DisplayableEventFormatter.kt # vector/src/main/res/layout/item_timeline_event_option_buttons_stub.xml # vector/src/main/res/xml/vector_settings_labs.xml

github-actions · 2021-12-07T16:23:17Z

Unit Test Results

  66 files ±0   66 suites ±0 49s ⏱️ -7s
135 tests ±0 135 ✔️ ±0 0 💤 ±0 0 ❌ ±0
418 runs ±0 418 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit db81ec2. ± Comparison against base commit 5ea7f3c.

♻️ This comment has been updated with latest results.

bmarty

Nice work!
The PR needs some cleanup before it can be merged, but there is not much to do.
Thanks!

bmarty · 2021-12-07T20:15:03Z

...dk-android/src/main/java/org/matrix/android/sdk/api/session/room/model/PollSummaryContent.kt

@@ -24,8 +24,7 @@ import com.squareup.moshi.JsonClass
 */
 @JsonClass(generateAdapter = true)
 data class PollSummaryContent(
-        // Index of my vote
-        var myVote: Int? = null,
+        var myVote: String? = null,


Strange to see var in a data class. Also there it no @Json annotations on the fields? Is it a Content of a Matrix message? If not if should be renamed to remove the Content suffix.

EDIT: I think it is not the case, so @JsonClass(generateAdapter = true) can also be removed.

Right, nice catch. Done.

Not done. We need it to be able to (de)serialize in DB as String.

bmarty · 2021-12-07T20:23:51Z

...k-android/src/main/java/org/matrix/android/sdk/api/session/room/model/message/MessageType.kt

-    const val MSGTYPE_RESPONSE = "org.matrix.response"
-    const val MSGTYPE_POLL_CLOSED = "org.matrix.poll_closed"
+    const val MSGTYPE_POLL_START = "org.matrix.msc3381.poll.start"
+    const val MSGTYPE_POLL_RESPONSE = "org.matrix.msc3381.poll.response"


Those 2 strings are not msgType, but EventType. And they are already defined in EventType. So please remove.

As we discussed, I changed it as we did for sticker events.

bmarty · 2021-12-07T20:25:49Z

...ain/java/org/matrix/android/sdk/api/session/room/model/message/MessagePollResponseContent.kt

 @JsonClass(generateAdapter = true)
 data class MessagePollResponseContent(
-        @Json(name = MessageContent.MSG_TYPE_JSON_KEY) override val msgType: String = MessageType.MSGTYPE_RESPONSE,
-        @Json(name = "body") override val body: String,
+        override val msgType: String = MessageType.MSGTYPE_POLL_RESPONSE,


This is not correct. MessagePollResponseContent must not inherit from MessageContent, since the type is not "m.room.message".
When I answer a poll from ElementWeb I got this FTR:

As we discussed, I changed it as we did for sticker events.

bmarty · 2021-12-07T20:28:54Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/send/SendService.kt

     * @return a [Cancelable]
     */
-    fun sendOptionsReply(pollEventId: String, optionIndex: Int, optionValue: String): Cancelable
+    fun registerVoteToPoll(pollEventId: String, optionKey: String): Cancelable


why register prefix here and not just voteToPoll? This is maybe coming from the spec?

It was from task title :) Changed as suggested.

bmarty · 2021-12-07T20:32:11Z

...main/java/org/matrix/android/sdk/internal/session/room/EventRelationsAggregationProcessor.kt

@@ -107,7 +111,7 @@ internal class EventRelationsAggregationProcessor @Inject constructor(
                        Timber.v("###REPLACE in room $roomId for event ${event.eventId}")
                        // A replace!
                        handleReplace(realm, event, content, roomId, isLocalEcho)
-                    } else if (content?.relatesTo?.type == RelationType.RESPONSE) {
+                    } else if (content is MessagePollResponseContent) {


event.type is not EventType.MESSAGE. This code can be removed, it`s handled at line 205

Of course. Good catch, removed.

bmarty · 2021-12-07T20:57:38Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/PollOptionItem.kt

+        context: Context,
+        attrs: AttributeSet? = null,
+        defStyleAttr: Int = 0
+) : ConstraintLayout(context, attrs, defStyleAttr) {


item_poll_option.xml root view is also a ConstraintLayout, so the view hierarchy is too deep. You may remove a level.

I didn't get it. It doesn't add a new level, just represents the current level, no?

It seems that you are right.

bmarty · 2021-12-07T20:58:58Z

vector/src/main/res/drawable/poll_option_progressbar_checked.xml

+            </shape>
+        </clip>
+    </item>
+</layer-list>


Nice solution!

bmarty · 2021-12-07T21:00:06Z

vector/src/main/res/layout/item_poll_option.xml

+        app:layout_constraintEnd_toEndOf="parent"
+        app:layout_constraintStart_toStartOf="parent"
+        app:layout_constraintTop_toTopOf="parent"
+        tools:ignore="ContentDescription" />


Please remove tools:ignore="ContentDescription" from several place in this file (and maybe in other files). You should either add a contentDescription or use importantForAccessibility attribute.

Right. This is the only file related to polls. Done.

bmarty · 2021-12-07T21:04:40Z

vector/src/main/res/values/strings.xml

+    </plurals>
+    <plurals name="poll_total_vote_count_after_ended">
+        <item quantity="one">Final result based on %1$s vote</item>
+        <item quantity="other">Final result based on %1$s votes</item>


I prefer that we use %1$d instead of %1$s when integers will be used to build the string. To be changed at 8 places.

Of course. Done.

bmarty · 2021-12-07T21:08:41Z

vector/src/main/res/values/strings.xml

+    <string name="poll_end_action">End poll</string>
+    <string name="a11y_poll_winner_option">winner option</string>
+    <string name="end_poll_confirmation_title">End poll</string>
+    <string name="end_poll_confirmation_description">Are you sure you want to end this poll? This will stop people from being able to vote and will display the final results of the poll.</string>


Saying "Are you sure" is bad for the user experience (too stressful).
The dialog title should be:

End this poll?

And the content:

Ending the poll will stop people from being able to vote and will display the final results of the poll.

I agree. We may consider checking other places we have this. Done.

bmarty · 2021-12-09T12:01:25Z

When enabling "show hidden event" I see that:

Could be nice to add a better notice display here.

bmarty

Thanks for the update. One more remark. Will do some test too.

bmarty · 2021-12-10T16:01:12Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/PollItem.kt

+                            render(PollOptionViewState.EnabledOptionWithVisibleVotes(optionName, voteCount, votePercentage, isMyVote), callback)
+                        } else {
+                            // User didn't voted yet and poll is not ended yet. Enable options, hide votes.
+                            render(PollOptionViewState.EnabledOptionWithInvisibleVotes(optionName), callback)


That's better but the Epoxy item should have a field with a list of PollOptionViewState now, so that all those computation are done outside the binding.

Your suggestion is to calculate state outside of epoxy item and pass PollOptionViewState to PollItem as EpoxyAttribute, right?

Yes, or rather a List<PollOptionViewState>

gaelledel

Please change the copy on the Remove dialog so it matches the copy provided in Figma/below

Can we have the same amount of padding on each side of the option frame? 8px should do. (currently it seems we have larger padding on the right).

We still have a problem with the option sizes. They need to reach the edge of the timestamp. See example here

gaelledel · 2021-12-13T12:17:12Z

@gaelledel it has been decided with @amshakal to add a right margin to all messages in the timeline, to avoid a blink effect because the last message has a send status which take some room on the right.

@bmarty Yes for text you might need a margin but it shouldn't affect other media types such as poll. See Andy's Read receipt in the image below:

We're considerably wasting real estate for each option text length by doing this where the RR is actually not obstructing anything being under the options.

bmarty · 2021-12-13T13:45:49Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/PollItem.kt

+
+        holder.questionTextView.text = pollContent?.pollCreationInfo?.question?.question
+
+        holder.optionsContainer.removeAllViews()


Should be better to avoid to do that each time, but rather get the existing views and recycle them.

Done. We always need to remove & add views but I suggest a way to prevent the recreation of option views.

bmarty · 2021-12-13T13:48:52Z

@gaelledel read receipt are below the messages, but send status are on the right of the message. We want to avoid the blink which can occur when the send status goes from shown to hidden (which occur as soon as another message appear in the timeline.

gaelledel

Approved by me 🥳

* develop: (49 commits) Update changelog.d/4592.bugfix Remove jcenter from here, let's see what the CI will say Common struct for each maven repo Rename the file to group them Add changelog file Revert PR 4592 for devtools Enhance include groups implementation by decoupling them to a separate file Cleanup Legals: improve API to get homeserver terms Legals: update setting icon Auto-review Add a help section in the settings. Changelog Color for links Use same height than the loading item to avoid dynamic resizing Legals: only display external URLs. Legals: Move the 2 copyrights items to the new legal screen Add some space between the 2 TextViews, and improve the layout Legals: Move the 3 element links to the new legal screen Rename some classes ... # Conflicts: # vector/src/main/res/layout/item_timeline_event_option_buttons_stub.xml # vector/src/main/res/layout/item_timeline_event_poll_stub.xml

bmarty

We are nearly there, some more comments!

bmarty · 2021-12-13T14:47:50Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/PollItem.kt

-        val didUserVoted = pollResponseSummary?.myVote?.isNotEmpty().orFalse()
-        val totalVotes = pollResponseSummary?.totalVotes ?: 0
-        val winnerVoteCount = pollResponseSummary?.winnerVoteCount
+        holder.optionsContainer.removeAllViews()


Maybe avoid to remove (and re-add later) all the View? The number of views may differ.

I just tested it and it is not working to remove items one by one before adding. Ends up with duplicated views.

Say you have 3 options to display. If there is:

0 existing views: create 3 views and add it to the parent (this will be the most cases)

4 existing views: just remove one, and do not add any

3 existing views: do nothing

2 existing views: create 1 view and add it to the parent

I see your point now. It will be an interesting experiment for me.

bmarty · 2021-12-13T14:48:52Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/PollItem.kt

-            val isMyVote = pollResponseSummary?.myVote == option.id
-            val voteCount = voteSummary?.total ?: 0
-            val votePercentage = voteSummary?.percentage ?: 0.0
+        pollContent?.pollCreationInfo?.answers?.forEachIndexed { index, option ->


I would not iterate on pollContent?.pollCreationInfo?.answers? anymore, but on optionViewStates now.

pollContent can be removed I think.

bmarty · 2021-12-13T14:51:26Z

...r/src/main/java/im/vector/app/features/home/room/detail/timeline/action/EventSharedAction.kt

@@ -60,7 +60,7 @@ sealed class EventSharedAction(@StringRes val titleRes: Int,
    data class Remove(val eventId: String) :
            EventSharedAction(R.string.remove, R.drawable.ic_trash, true)

-    data class Redact(val eventId: String, val askForReason: Boolean) :
+    data class Redact(val eventId: String, val askForReason: Boolean, val dialogTitleRes: Int, val dialogDescriptionRes: Int) :


Do we want to update the action text too?
Currently this is:

(I think, not tested on your branch though)

Yes, design team agrees to use the existing item.

bmarty

Some minor final remarks

bmarty · 2021-12-13T19:51:18Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/PollItem.kt

+    var pollQuestion: String? = null
+
+    @EpoxyAttribute
+    var pollResponseSummary: PollResponseData? = null


This can be removed.

bmarty · 2021-12-13T19:52:32Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/PollOptionItem.kt

+import im.vector.app.core.extensions.setAttributeTintedImageResource
+import im.vector.app.databinding.ItemPollOptionBinding
+
+class PollOptionItem @JvmOverloads constructor(


Class with Item suffix is reserved for Epoxy items. Please rename to PollOptionView

bmarty · 2021-12-13T19:55:30Z

vector/src/main/java/im/vector/app/features/home/room/detail/timeline/item/PollOptionItem.kt

+            is PollOptionViewState.DisabledOptionWithInvisibleVotes -> renderDisabledOptionWithInvisibleVotes()
+            is PollOptionViewState.DisabledOptionWithVisibleVotes   -> renderDisabledOptionWithVisibleVotes(state)
+            is PollOptionViewState.EnabledOptionWithInvisibleVotes  -> renderEnabledOptionWithInvisibleVotes()
+            is PollOptionViewState.EnabledOptionWithVisibleVotes    -> renderEnabledOptionWithVisibleVotes(state)


The code is much cleaner now, thanks!

6543 · 2021-12-13T20:04:13Z

hey just stumbled over this pull - a smal question about this feature pr:

is there a option to add new options to choose (not sure how ui should look like (to toggle other to allow that or deny ....))

onurays · 2021-12-13T20:16:06Z

hey just stumbled over this pull - a smal question about this feature pr:

is there a option to add new options to choose (not sure how ui should look like (to toggle other to allow that or deny ....))

Editing polls is not a part of this PR but it is planned/designed as post MVP.

bmarty

Approved, I have added some commits, you tell me if you agree with the changes.

Onuray Sahin added 13 commits November 9, 2021 13:19

Merge branch 'feature/ons/poll' into feature/ons/poll_timeline

94ae3c6

* feature/ons/poll: Hide poll icon from attachment type selector for now. Code review fixes. Lint fixes. Design review fixes.

Implement new poll content.

ebc131f

Allow sending vote and ending poll.

7c26930

Allow removing poll event.

a3b11b2

Aggregate votes and poll end event.

2a3a558

Implement poll in timeline ui.

06485cf

Remove legacy poll ui.

32e8a7e

Implement poll actions bottom sheet.

c62028d

Add confirmation dialog to end poll.

0a7df44

Fix room list preview.

435dc9f

Remove legacy implementation classes.

23ad4e5

Onuray Sahin added 5 commits December 7, 2021 11:53

Add labs flag for polls.

1df6b33

Fix poll option checkbox color.

0d3444b

Support push notification for poll creation event.

75b544a

Add room list preview for poll response and end events.

71d7270

Set max length for poll options.

566f633

Changelog added.

0f11e49

onurays linked an issue Dec 7, 2021 that may be closed by this pull request

Poll Feature - Render in timeline #4653

Closed

8 tasks

onurays requested review from bmarty and gaelledel December 7, 2021 17:14

onurays marked this pull request as ready for review December 7, 2021 17:16

bmarty requested changes Dec 7, 2021

View reviewed changes

Code review fixes.

04a7590

onurays requested a review from gaelledel December 10, 2021 14:58

bmarty reviewed Dec 10, 2021

View reviewed changes

gaelledel suggested changes Dec 13, 2021

View reviewed changes

bmarty reviewed Dec 13, 2021

View reviewed changes

Code and design review fixes.

c7ad50a

gaelledel approved these changes Dec 13, 2021

View reviewed changes

Code review fixes.

f6dcda6

onurays requested a review from bmarty December 13, 2021 14:42

bmarty reviewed Dec 13, 2021

View reviewed changes

Code review fixes.

e2bbc3f

bmarty reviewed Dec 13, 2021

View reviewed changes

Reorder classes so that it follows the poll status logical order

c1ea653

bmarty added 2 commits December 13, 2021 21:04

Do some renaming

eac06d5

Do some renaming

10b39cc

bmarty added 3 commits December 13, 2021 21:16

Avoid lateinit

da407ef

Rename class

f29e14f

Remove unused attribute

0981af3

bmarty force-pushed the feature/ons/poll_timeline branch from 8dff863 to 0981af3 Compare December 13, 2021 20:33

Recycle View a bit more

db81ec2

bmarty approved these changes Dec 13, 2021

View reviewed changes

bmarty added the A-Polls label Dec 13, 2021

This was referenced Dec 13, 2021

[Poll] Close poll #991

Closed

I can't see total votes, also no secret voting #1942

Closed

bmarty merged commit 38e7e2f into develop Dec 13, 2021

bmarty deleted the feature/ons/poll_timeline branch December 13, 2021 21:29


		holder.questionTextView.text = pollContent?.pollCreationInfo?.question?.question

		holder.optionsContainer.removeAllViews()

Poll Feature - Timeline #4624

Poll Feature - Timeline #4624

Conversation

onurays commented Dec 3, 2021 • edited Loading

DoM1niC commented Dec 3, 2021

onurays commented Dec 7, 2021

github-actions bot commented Dec 7, 2021 • edited Loading

Unit Test Results

bmarty left a comment

Choose a reason for hiding this comment

bmarty Dec 7, 2021 • edited Loading

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

onurays Dec 8, 2021 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty commented Dec 9, 2021 • edited by onurays Loading

bmarty 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

Choose a reason for hiding this comment

gaelledel left a comment • edited by onurays Loading

Choose a reason for hiding this comment

gaelledel commented Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty commented Dec 13, 2021

gaelledel left a comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty Dec 13, 2021 • edited Loading

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

Choose a reason for hiding this comment

bmarty 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

6543 commented Dec 13, 2021

onurays commented Dec 13, 2021

bmarty left a comment

Choose a reason for hiding this comment

onurays commented Dec 3, 2021 •

edited

Loading

github-actions bot commented Dec 7, 2021 •

edited

Loading

bmarty Dec 7, 2021 •

edited

Loading

onurays Dec 8, 2021 •

edited

Loading

bmarty commented Dec 9, 2021 •

edited by onurays

Loading

gaelledel left a comment •

edited by onurays

Loading

gaelledel commented Dec 13, 2021 •

edited

Loading

bmarty Dec 13, 2021 •

edited

Loading