-
Notifications
You must be signed in to change notification settings - Fork 754
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
Feature/fga/message bubbles #4937
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thanks! Will do some test
|
||
abstract fun getViewStubId(): Int | ||
|
||
// Szudzik function |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, nice to see that here! But is there a risk of overflow since Android ids can be big integers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I changed to Long and then normalize by substracting Int.MAX_VALUE
* | ||
* @return true to show emoji keyboard button. | ||
*/ | ||
fun useMessageBubblesLayout(): Boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the comment above :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
(Could be nice to add some screenshots in the PR) |
<im.vector.app.core.preference.VectorSwitchPreference | ||
android:defaultValue="false" | ||
android:key="SETTINGS_INTERFACE_BUBBLE_KEY" | ||
android:title="@string/message_bubbles" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to move this in the timeline
section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Quick remark, maybe increase the font size of the messages when bubbles are enabled. |
… "modern" layout.
Matrix SDKIntegration Tests Results:
|
❌ Error: 'layout_(.*)="@+id' detected 1 time, in file './vector/src/main/res/layout/view_message_bubble.xml', at line 203. Also have you handled all my remarks above? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work.
Maybe the changes regarding code block rendering, and URL preview management (with width and heigth) could have been handled in dedicated PR.
Still some small remarks and existing one not yet handle.
After we could merge on develop if the PR is ready!
private val vectorPreferences: VectorPreferences) { | ||
|
||
companion object { | ||
// Can't be rendered in bubbles, so get back to default layout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
private val isRTL: Boolean by lazy { | ||
val currentLocale = localeProvider.current() | ||
TextUtils.getLayoutDirectionFromLocale(currentLocale) == View.LAYOUT_DIRECTION_RTL | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do this on develop now: resources.getBoolean(R.bool.is_rtl)
, simplier to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
R.bool.is_rtl
is not working properly if the user change the language inside the app.
This have to be rework. I create an issue from here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I'll close the related issue then.
@@ -76,6 +81,22 @@ class PreviewUrlView @JvmOverloads constructor( | |||
} | |||
} | |||
|
|||
override fun render(messageLayout: TimelineMessageLayout) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird to have to render
fun in this class now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to renderMessageLayout
@@ -55,5 +55,5 @@ | |||
app:layout_constraintHorizontal_bias="0" | |||
app:layout_constraintStart_toEndOf="@id/messageThreadSummaryAvatarImageView" | |||
app:layout_constraintTop_toTopOf="parent" | |||
tools:text="@sample/messages.json/data/message" /> | |||
tools:text="Pouet" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) (maybe revert)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. Can you add a file for the changelog please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the update!
@@ -225,7 +223,7 @@ class MessageItemFactory @Inject constructor( | |||
.locationUrl(locationUrl) | |||
.mapWidth(width) | |||
.mapHeight(height) | |||
.userId(informationData.senderId) | |||
.userId(userId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @onurays
private val isRTL: Boolean by lazy { | ||
val currentLocale = localeProvider.current() | ||
TextUtils.getLayoutDirectionFromLocale(currentLocale) == View.LAYOUT_DIRECTION_RTL | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I'll close the related issue then.
This PR introduces message bubbles in Timeline.
It allows to switch Layout at runtime from the Preferences.
It also improves a bit Code Block rendering and Url previews.