[Feature] Summary/Show all of small events #469

tyreseluo · 2025-04-22T15:15:12Z

PR content

By default, a series of two or more adjacent small events should be collapsed into an abbreviated view that takes up much less vertical space. This will make it easier for the user to focus on important content like text and image messages from other users, rather than less important metadata.

Related PR

Collapsible/expandable sequence of small events #118

Copilot

Pull Request Overview

This PR implements a feature to collapse adjacent small events into a summarized view in the room timeline, improving vertical space usage and focusing the UI on more prominent messages. Key changes include updating imports to support new grouping logic, adding the live design element "SmallStateEventSummary", and refactoring the timeline population logic to handle grouped small events.

Copilot · 2025-05-12T02:37:57Z

src/home/room_screen.rs

-                                populate_message_view(
+                match group {
+                    GroupedTimelineItem::Single(timeline_item) => {
+                        let tl_idx = tl_items.iter().position(|x| Arc::ptr_eq(x, timeline_item));


The use of an iterative search (position on tl_items) for each grouped single event may introduce performance issues for large timelines. Consider caching or maintaining an index map to improve lookup efficiency.

Timeline item uses paged loading, so no this performance issue.

Copilot · 2025-05-12T02:37:57Z

src/home/room_screen.rs

+                buffer.push(item.clone());
+            }
+            _ => {
+                if buffer.len() >= 3 {


The grouping threshold of 3 is a magic number; consider defining it as a named constant to improve readability and facilitate future adjustments.

Suggested change

if buffer.len() >= 3 {

if buffer.len() >= GROUPING_THRESHOLD {

Copilot

Pull Request Overview

This PR collapses sequences of adjacent small state events into a compact summary view with a “Show all” button instead of rendering each event individually.

Defined a <SmallStateEventSummary> layout and “Show all” button in the live design.
Added group_small_state_events and summarize_small_events to cluster and describe adjacent small events.
Updated the main timeline loop to render grouped summaries (GroupedTimelineItem::GroupedSmallEvents) instead of single items.

Comments suppressed due to low confidence (1)

src/home/room_screen.rs:4123

Consider adding unit tests for group_small_state_events and summarize_small_events to verify that grouping and summary text are correct across event types.

fn group_small_state_events(

Copilot · 2025-05-14T06:25:47Z

src/home/room_screen.rs

+                buffer.push(item.clone());
+            }
+            _ => {
+                if buffer.len() >= 3 {


The PR description specifies collapsing two or more adjacent small events, but this condition uses >= 3. Change it to >= 2 to match the intended behavior.

Suggested change

if buffer.len() >= 3 {

if buffer.len() >= 2 {

Two small event is still easy to read.

Let's reguard 3 as the dividing corner to collapse.

Copilot · 2025-05-14T06:25:47Z

src/home/room_screen.rs

+    }
+
+    // handle any remaining items in the buffer
+    if buffer.len() >= 3 {


This second threshold also uses >= 3. It should be >= 2 so that runs of two small events are grouped as per spec.

Suggested change

if buffer.len() >= 3 {

if buffer.len() >= 2 {

Copilot · 2025-05-14T06:25:47Z

src/home/room_screen.rs


+#[derive(Clone, Debug)]
+enum GroupedTimelineItem {
+    Single(Arc<TimelineItem>),
+    GroupedSmallEvents(Vec<Arc<TimelineItem>>),
+}
+


[nitpick] Add doc comments to GroupedTimelineItem, group_small_state_events, and summarize_small_events to explain their roles and grouping logic.

Suggested change

#[derive(Clone, Debug)]

enum GroupedTimelineItem {

Single(Arc<TimelineItem>),

GroupedSmallEvents(Vec<Arc<TimelineItem>>),

}

/// Represents a timeline item that can either be a single item or a group of small events.

///

/// - `Single`: A single timeline item.

/// - `GroupedSmallEvents`: A collection of small events grouped together for display purposes.

#[derive(Clone, Debug)]

enum GroupedTimelineItem {

Single(Arc<TimelineItem>),

GroupedSmallEvents(Vec<Arc<TimelineItem>>),

}

/// Groups small state events from a list of timeline items into `GroupedTimelineItem` variants.

///

/// This function iterates through the provided timeline items and groups small state events

/// (e.g., membership changes, profile changes, redacted messages, and other state events)

/// into a `GroupedSmallEvents` variant if there are three or more consecutive small events.

/// Otherwise, it keeps them as individual `Single` items.

///

/// # Parameters

/// - `tl_items`: A vector of timeline items to be grouped.

///

/// # Returns

/// A vector of `GroupedTimelineItem` representing the grouped and individual timeline items.

@tyreseluo , How do you like copilot's doc?

Yep, I will keep it and make some small modifications.

Copilot · 2025-05-14T06:25:48Z

src/home/room_screen.rs

+        parts.push(text);
+    }
+    if redacted > 0 {
+        parts.push(format!("{} message(s) were redacted", redacted));


The text message(s) is ambiguous for singular vs. plural. Consider branching to message was redacted when redacted == 1 and messages were redacted otherwise.

Suggested change

parts.push(format!("{} message(s) were redacted", redacted));

if redacted == 1 {

parts.push("1 message was redacted".to_string());

} else {

parts.push(format!("{} messages were redacted", redacted));

}

Copilot · 2025-05-14T06:25:48Z

src/home/room_screen.rs

+        parts.push(format!("{} message(s) were redacted", redacted));
+    }
+    if state_changed > 0 {
+        parts.push(format!("{} state change(s)", state_changed));


Similar to the redacted count, state change(s) should handle singular/plural correctly (e.g., 1 state change vs. n state changes).

Suggested change

parts.push(format!("{} state change(s)", state_changed));

parts.push(format!(

"{} state change{}",

state_changed,

if state_changed == 1 { "" } else { "s" }

));

aaravlu requested a review from Copilot May 12, 2025 02:37

Copilot AI reviewed May 12, 2025

View reviewed changes

aaravlu requested a review from Copilot May 14, 2025 06:23

Copilot AI reviewed May 14, 2025

View reviewed changes

Refactor the PR

c0ef539

tyreseluo force-pushed the feature/collapsible_expandable_sequence_small_events branch from 4e9ddef to c0ef539 Compare May 22, 2025 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Summary/Show all of small events #469

[Feature] Summary/Show all of small events #469

tyreseluo commented Apr 22, 2025

Copilot AI left a comment

Copilot AI May 12, 2025

aaravlu May 12, 2025

Copilot AI May 12, 2025

aaravlu May 12, 2025

Copilot AI left a comment

Copilot AI May 14, 2025

aaravlu May 14, 2025

Copilot AI May 14, 2025

aaravlu May 14, 2025

tyreseluo May 22, 2025

Copilot AI May 14, 2025

aaravlu May 14, 2025

tyreseluo May 22, 2025

Copilot AI May 14, 2025

Copilot AI May 14, 2025

	if buffer.len() >= 3 {
	if buffer.len() >= GROUPING_THRESHOLD {

-#[derive(Clone, Debug)]
-enum GroupedTimelineItem {
-    Single(Arc<TimelineItem>),
-    GroupedSmallEvents(Vec<Arc<TimelineItem>>),
-}
+/// Represents a timeline item that can either be a single item or a group of small events.
+///
+/// - `Single`: A single timeline item.
+/// - `GroupedSmallEvents`: A collection of small events grouped together for display purposes.
+#[derive(Clone, Debug)]
+enum GroupedTimelineItem {
+    Single(Arc<TimelineItem>),
+    GroupedSmallEvents(Vec<Arc<TimelineItem>>),
+}
+/// Groups small state events from a list of timeline items into `GroupedTimelineItem` variants.
+///
+/// This function iterates through the provided timeline items and groups small state events
+/// (e.g., membership changes, profile changes, redacted messages, and other state events)
+/// into a `GroupedSmallEvents` variant if there are three or more consecutive small events.
+/// Otherwise, it keeps them as individual `Single` items.
+///
+/// # Parameters
+/// - `tl_items`: A vector of timeline items to be grouped.
+///
+/// # Returns
+/// A vector of `GroupedTimelineItem` representing the grouped and individual timeline items.

-        parts.push(format!("{} message(s) were redacted", redacted));
+        if redacted == 1 {
+            parts.push("1 message was redacted".to_string());
+        } else {
+            parts.push(format!("{} messages were redacted", redacted));
+        }

-        parts.push(format!("{} state change(s)", state_changed));
+        parts.push(format!(
+            "{} state change{}",
+            state_changed,
+            if state_changed == 1 { "" } else { "s" }
+        ));

[Feature] Summary/Show all of small events #469

Are you sure you want to change the base?

[Feature] Summary/Show all of small events #469

Conversation

tyreseluo commented Apr 22, 2025

PR content

Related PR

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Copilot AI May 12, 2025

Choose a reason for hiding this comment

aaravlu May 12, 2025

Choose a reason for hiding this comment

Copilot AI May 12, 2025

Choose a reason for hiding this comment

aaravlu May 12, 2025

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Copilot AI May 14, 2025

Choose a reason for hiding this comment

aaravlu May 14, 2025

Choose a reason for hiding this comment

Copilot AI May 14, 2025

Choose a reason for hiding this comment

aaravlu May 14, 2025

Choose a reason for hiding this comment

tyreseluo May 22, 2025

Choose a reason for hiding this comment

Copilot AI May 14, 2025

Choose a reason for hiding this comment

aaravlu May 14, 2025

Choose a reason for hiding this comment

tyreseluo May 22, 2025

Choose a reason for hiding this comment

Copilot AI May 14, 2025

Choose a reason for hiding this comment

Copilot AI May 14, 2025

Choose a reason for hiding this comment