
Release Notes: - Added channel reordering for administrators (use `cmd-up` and `cmd-down` on macOS or `ctrl-up` `ctrl-down` on Linux to move channels up or down within their parent) ## Summary This PR introduces the ability for channel administrators to reorder channels within their parent context, providing better organizational control over channel hierarchies. Users can now move channels up or down relative to their siblings using keyboard shortcuts. ## Problem Previously, channels were displayed in alphabetical order with no way to customize their arrangement. This made it difficult for teams to organize channels in a logical order that reflected their workflow or importance, forcing users to prefix channel names with numbers or special characters as a workaround. ## Solution The implementation adds a persistent `channel_order` field to channels that determines their display order within their parent. Channels with the same parent are sorted by this field rather than alphabetically. ## Implementation Details ### Database Schema Added a new column and index to support efficient ordering: ```sql -- crates/collab/migrations/20250530175450_add_channel_order.sql ALTER TABLE channels ADD COLUMN channel_order INTEGER NOT NULL DEFAULT 1; CREATE INDEX CONCURRENTLY "index_channels_on_parent_path_and_order" ON "channels" ("parent_path", "channel_order"); ``` ### RPC Protocol Extended the channel proto with ordering support: ```proto // crates/proto/proto/channel.proto message Channel { uint64 id = 1; string name = 2; ChannelVisibility visibility = 3; int32 channel_order = 4; repeated uint64 parent_path = 5; } message ReorderChannel { uint64 channel_id = 1; enum Direction { Up = 0; Down = 1; } Direction direction = 2; } ``` ### Server-side Logic The reordering is handled by swapping `channel_order` values between adjacent channels: ```rust // crates/collab/src/db/queries/channels.rs pub async fn reorder_channel( &self, channel_id: ChannelId, direction: proto::reorder_channel::Direction, user_id: UserId, ) -> Result<Vec<Channel>> { // Find the sibling channel to swap with let sibling_channel = match direction { proto::reorder_channel::Direction::Up => { // Find channel with highest order less than current channel::Entity::find() .filter( channel::Column::ParentPath .eq(&channel.parent_path) .and(channel::Column::ChannelOrder.lt(channel.channel_order)), ) .order_by_desc(channel::Column::ChannelOrder) .one(&*tx) .await? } // Similar logic for Down... }; // Swap the channel_order values let temp_order = channel.channel_order; channel.channel_order = sibling_channel.channel_order; sibling_channel.channel_order = temp_order; } ``` ### Client-side Sorting Optimized the sorting algorithm to avoid O(n²) complexity: ```rust // crates/collab/src/db/queries/channels.rs // Pre-compute sort keys for efficient O(n log n) sorting let mut channels_with_keys: Vec<(Vec<i32>, Channel)> = channels .into_iter() .map(|channel| { let mut sort_key = Vec::with_capacity(channel.parent_path.len() + 1); // Build sort key from parent path orders for parent_id in &channel.parent_path { sort_key.push(channel_order_map.get(parent_id).copied().unwrap_or(i32::MAX)); } sort_key.push(channel.channel_order); (sort_key, channel) }) .collect(); channels_with_keys.sort_by(|a, b| a.0.cmp(&b.0)); ``` ### User Interface Added keyboard shortcuts and proper context handling: ```json // assets/keymaps/default-macos.json { "context": "CollabPanel && not_editing", "bindings": { "cmd-up": "collab_panel::MoveChannelUp", "cmd-down": "collab_panel::MoveChannelDown" } } ``` The CollabPanel now properly sets context to distinguish between editing and navigation modes: ```rust // crates/collab_ui/src/collab_panel.rs fn dispatch_context(&self, window: &Window, cx: &Context<Self>) -> KeyContext { let mut dispatch_context = KeyContext::new_with_defaults(); dispatch_context.add("CollabPanel"); dispatch_context.add("menu"); let identifier = if self.channel_name_editor.focus_handle(cx).is_focused(window) { "editing" } else { "not_editing" }; dispatch_context.add(identifier); dispatch_context } ``` ## Testing Comprehensive tests were added to verify: - Basic reordering functionality (up/down movement) - Boundary conditions (first/last channels) - Permission checks (non-admins cannot reorder) - Ordering persistence across server restarts - Correct broadcasting of changes to channel members ## Migration Strategy Existing channels are assigned initial `channel_order` values based on their current alphabetical sorting to maintain the familiar order users expect: ```sql UPDATE channels SET channel_order = ( SELECT ROW_NUMBER() OVER ( PARTITION BY parent_path ORDER BY name, id ) FROM channels c2 WHERE c2.id = channels.id ); ``` ## Future Enhancements While this PR provides basic reordering functionality, potential future improvements could include: - Drag-and-drop reordering in the UI - Bulk reordering operations - Custom sorting strategies (by activity, creation date, etc.) ## Checklist - [x] Database migration included - [x] Tests added for new functionality - [x] Keybindings work on macOS and Linux - [x] Permissions properly enforced - [x] Error handling implemented throughout - [x] Manual testing completed - [x] Documentation updated --------- Co-authored-by: Mikayla Maki <mikayla.c.maki@gmail.com>
132 lines
8.7 KiB
Text
132 lines
8.7 KiB
Text
# Rust coding guidelines
|
|
|
|
* Prioritize code correctness and clarity. Speed and efficiency are secondary priorities unless otherwise specified.
|
|
* Do not write organizational or comments that summarize the code. Comments should only be written in order to explain "why" the code is written in some way in the case there is a reason that is tricky / non-obvious.
|
|
* Prefer implementing functionality in existing files unless it is a new logical component. Avoid creating many small files.
|
|
* Avoid using functions that panic like `unwrap()`, instead use mechanisms like `?` to propagate errors.
|
|
* Be careful with operations like indexing which may panic if the indexes are out of bounds.
|
|
* Never silently discard errors with `let _ =` on fallible operations. Always handle errors appropriately:
|
|
- Propagate errors with `?` when the calling function should handle them
|
|
- Use `.log_err()` or similar when you need to ignore errors but want visibility
|
|
- Use explicit error handling with `match` or `if let Err(...)` when you need custom logic
|
|
- Example: avoid `let _ = client.request(...).await?;` - use `client.request(...).await?;` instead
|
|
* When implementing async operations that may fail, ensure errors propagate to the UI layer so users get meaningful feedback.
|
|
* Never create files with `mod.rs` paths - prefer `src/some_module.rs` instead of `src/some_module/mod.rs`.
|
|
|
|
# GPUI
|
|
|
|
GPUI is a UI framework which also provides primitives for state and concurrency management.
|
|
|
|
## Context
|
|
|
|
Context types allow interaction with global state, windows, entities, and system services. They are typically passed to functions as the argument named `cx`. When a function takes callbacks they come after the `cx` parameter.
|
|
|
|
* `App` is the root context type, providing access to global state and read and update of entities.
|
|
* `Context<T>` is provided when updating an `Entity<T>`. This context dereferences into `App`, so functions which take `&App` can also take `&Context<T>`.
|
|
* `AsyncApp` and `AsyncWindowContext` are provided by `cx.spawn` and `cx.spawn_in`. These can be held across await points.
|
|
|
|
## `Window`
|
|
|
|
`Window` provides access to the state of an application window. It is passed to functions as an argument named `window` and comes before `cx` when present. It is used for managing focus, dispatching actions, directly drawing, getting user input state, etc.
|
|
|
|
## Entities
|
|
|
|
An `Entity<T>` is a handle to state of type `T`. With `thing: Entity<T>`:
|
|
|
|
* `thing.entity_id()` returns `EntityId`
|
|
* `thing.downgrade()` returns `WeakEntity<T>`
|
|
* `thing.read(cx: &App)` returns `&T`.
|
|
* `thing.read_with(cx, |thing: &T, cx: &App| ...)` returns the closure's return value.
|
|
* `thing.update(cx, |thing: &mut T, cx: &mut Context<T>| ...)` allows the closure to mutate the state, and provides a `Context<T>` for interacting with the entity. It returns the closure's return value.
|
|
* `thing.update_in(cx, |thing: &mut T, window: &mut Window, cx: &mut Context<T>| ...)` takes a `AsyncWindowContext` or `VisualTestContext`. It's the same as `update` while also providing the `Window`.
|
|
|
|
Within the closures, the inner `cx` provided to the closure must be used instead of the outer `cx` to avoid issues with multiple borrows.
|
|
|
|
Trying to update an entity while it's already being updated must be avoided as this will cause a panic.
|
|
|
|
When `read_with`, `update`, or `update_in` are used with an async context, the closure's return value is wrapped in an `anyhow::Result`.
|
|
|
|
`WeakEntity<T>` is a weak handle. It has `read_with`, `update`, and `update_in` methods that work the same, but always return an `anyhow::Result` so that they can fail if the entity no longer exists. This can be useful to avoid memory leaks - if entities have mutually recursive handles to eachother they will never be dropped.
|
|
|
|
## Concurrency
|
|
|
|
All use of entities and UI rendering occurs on a single foreground thread.
|
|
|
|
`cx.spawn(async move |cx| ...)` runs an async closure on the foreground thread. Within the closure, `cx` is an async context like `AsyncApp` or `AsyncWindowContext`.
|
|
|
|
When the outer cx is a `Context<T>`, the use of `spawn` instead looks like `cx.spawn(async move |handle, cx| ...)`, where `handle: WeakEntity<T>`.
|
|
|
|
To do work on other threads, `cx.background_spawn(async move { ... })` is used. Often this background task is awaited on by a foreground task which uses the results to update state.
|
|
|
|
Both `cx.spawn` and `cx.background_spawn` return a `Task<R>`, which is a future that can be awaited upon. If this task is dropped, then its work is cancelled. To prevent this one of the following must be done:
|
|
|
|
* Awaiting the task in some other async context.
|
|
* Detaching the task via `task.detach()` or `task.detach_and_log_err(cx)`, allowing it to run indefinitely.
|
|
* Storing the task in a field, if the work should be halted when the struct is dropped.
|
|
|
|
A task which doesn't do anything but provide a value can be created with `Task::ready(value)`.
|
|
|
|
## Elements
|
|
|
|
The `Render` trait is used to render some state into an element tree that is laid out using flexbox layout. An `Entity<T>` where `T` implements `Render` is sometimes called a "view".
|
|
|
|
Example:
|
|
|
|
```
|
|
struct TextWithBorder(SharedString);
|
|
|
|
impl Render for TextWithBorder {
|
|
fn render(&mut self, _window: &mut Window, _cx: &mut Context<Self>) -> impl IntoElement {
|
|
div().border_1().child(self.0.clone())
|
|
}
|
|
}
|
|
```
|
|
|
|
Since `impl IntoElement for SharedString` exists, it can be used as an argument to `child`. `SharedString` is used to avoid copying strings, and is either an `&'static str` or `Arc<str>`.
|
|
|
|
UI components that are constructed just to be turned into elements can instead implement the `RenderOnce` trait, which is similar to `Render`, but its `render` method takes ownership of `self`. Types that implement this trait can use `#[derive(IntoElement)]` to use them directly as children.
|
|
|
|
The style methods on elements are similar to those used by Tailwind CSS.
|
|
|
|
If some attributes or children of an element tree are conditional, `.when(condition, |this| ...)` can be used to run the closure only when `condition` is true. Similarly, `.when_some(option, |this, value| ...)` runs the closure when the `Option` has a value.
|
|
|
|
## Input events
|
|
|
|
Input event handlers can be registered on an element via methods like `.on_click(|event, window, cx: &mut App| ...)`.
|
|
|
|
Often event handlers will want to update the entity that's in the current `Context<T>`. The `cx.listener` method provides this - its use looks like `.on_click(cx.listener(|this: &mut T, event, window, cx: &mut Context<T>| ...)`.
|
|
|
|
## Actions
|
|
|
|
Actions are dispatched via user keyboard interaction or in code via `window.dispatch_action(SomeAction.boxed_clone(), cx)` or `focus_handle.dispatch_action(&SomeAction, window, cx)`.
|
|
|
|
Actions which have no data inside are created and registered with the `actions!(some_namespace, [SomeAction, AnotherAction])` macro call.
|
|
|
|
Actions that do have data must implement `Clone, Default, PartialEq, Deserialize, JsonSchema` and can be registered with an `impl_actions!(some_namespace, [SomeActionWithData])` macro call.
|
|
|
|
Action handlers can be registered on an element via the event handler `.on_action(|action, window, cx| ...)`. Like other event handlers, this is often used with `cx.listener`.
|
|
|
|
## Notify
|
|
|
|
When a view's state has changed in a way that may affect its rendering, it should call `cx.notify()`. This will cause the view to be rerendered. It will also cause any observe callbacks registered for the entity with `cx.observe` to be called.
|
|
|
|
## Entity events
|
|
|
|
While updating an entity (`cx: Context<T>`), it can emit an event using `cx.emit(event)`. Entities register which events they can emit by declaring `impl EventEmittor<EventType> for EntityType {}`.
|
|
|
|
Other entities can then register a callback to handle these events by doing `cx.subscribe(other_entity, |this, other_entity, event, cx| ...)`. This will return a `Subscription` which deregisters the callback when dropped. Typically `cx.subscribe` happens when creating a new entity and the subscriptions are stored in a `_subscriptions: Vec<Subscription>` field.
|
|
|
|
## Recent API changes
|
|
|
|
GPUI has had some changes to its APIs. Always write code using the new APIs:
|
|
|
|
* `spawn` methods now take async closures (`AsyncFn`), and so should be called like `cx.spawn(async move |cx| ...)`.
|
|
* Use `Entity<T>`. This replaces `Model<T>` and `View<T>` which no longer exist and should NEVER be used.
|
|
* Use `App` references. This replaces `AppContext` which no longer exists and should NEVER be used.
|
|
* Use `Context<T>` references. This replaces `ModelContext<T>` which no longer exists and should NEVER be used.
|
|
* `Window` is now passed around explicitly. The new interface adds a `Window` reference parameter to some methods, and adds some new "*_in" methods for plumbing `Window`. The old types `WindowContext` and `ViewContext<T>` should NEVER be used.
|
|
|
|
|
|
## General guidelines
|
|
|
|
- Use `./script/clippy` instead of `cargo clippy`
|