From 4be75253cde57aead1b5e8328cfac18c60d06649 Mon Sep 17 00:00:00 2001 From: Ren Amamiya Date: Sun, 13 Sep 2026 17:18:54 +0700 Subject: [PATCH] update --- crates/workspace/src/views/issues/mod.rs | 37 +++++--------- crates/workspace/src/views/mod.rs | 1 + .../workspace/src/views/pull_requests/mod.rs | 49 ++++++------------- .../src/views/sidebar/grasp_servers.rs | 24 +++------ crates/workspace/src/views/sidebar/mod.rs | 21 ++++++++ .../src/views/sidebar/settings_dialog.rs | 22 +++------ crates/workspace/src/views/status_list.rs | 45 +++++++++++++++++ docs/over-engineering-cleanup-plan.md | 40 ++++++++++----- 8 files changed, 136 insertions(+), 103 deletions(-) create mode 100644 crates/workspace/src/views/status_list.rs diff --git a/crates/workspace/src/views/issues/mod.rs b/crates/workspace/src/views/issues/mod.rs index a3741ed..5100175 100644 --- a/crates/workspace/src/views/issues/mod.rs +++ b/crates/workspace/src/views/issues/mod.rs @@ -24,6 +24,7 @@ use utils::relative_time; pub(super) mod detail; use self::detail::IssueDetailView; +use super::status_list::{StatusCounts, filter_by_status}; const ISSUE_ROW_HEIGHT: f32 = 73.; @@ -52,7 +53,7 @@ pub struct IssuesView { filter: IssueFilter, item_sizes: Rc>>, visible_issues: Vec, - counts: (usize, usize, usize), + counts: StatusCounts, // A filter change notifies even when the visible rows are unchanged, // e.g. switching between two empty filters. synced_filter: IssueFilter, @@ -85,7 +86,7 @@ impl IssuesView { filter: IssueFilter::Open, item_sizes: Rc::new(Vec::new()), visible_issues: Vec::new(), - counts: (0, 0, 0), + counts: StatusCounts::default(), synced_filter: IssueFilter::Open, scroll_handle: VirtualListScrollHandle::new(), _subscription: subscription, @@ -97,25 +98,11 @@ impl IssuesView { let (visible_issues, counts) = { let store = self.store.read(cx); - let mut counts = (0usize, 0usize, 0usize); - - let visible_issues: Vec = store - .issues - .iter() - .enumerate() - .filter_map(|(ix, issue)| { - let status = store.status_of(issue); - counts.0 += 1; - match status { - RepoStatus::Open => counts.1 += 1, - RepoStatus::Closed => counts.2 += 1, - RepoStatus::Draft | RepoStatus::Applied => {} - } - filter.matches(status).then_some(ix) - }) - .collect(); - - (visible_issues, counts) + filter_by_status( + &store.issues, + |issue| store.status_of(issue), + |status| filter.matches(status), + ) }; let filter_changed = self.synced_filter != filter; @@ -218,7 +205,7 @@ impl IssuesView { } fn render_header(&self, cx: &mut Context) -> AnyElement { - let (total, open, closed) = self.counts; + let counts = self.counts; h_flex() .px_4() @@ -234,7 +221,7 @@ impl IssuesView { .child( SegmentButton::new("all", "All") .icon(Icon::new(CustomIconName::GitIssueDone)) - .count(total) + .count(counts.total) .selected(self.filter == IssueFilter::All) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = IssueFilter::All; @@ -244,7 +231,7 @@ impl IssuesView { .child( SegmentButton::new("open", "Open") .icon(Icon::new(CustomIconName::GitIssueOpen)) - .count(open) + .count(counts.open) .selected(self.filter == IssueFilter::Open) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = IssueFilter::Open; @@ -254,7 +241,7 @@ impl IssuesView { .child( SegmentButton::new("closed", "Closed") .icon(Icon::new(CustomIconName::GitIssueClosed)) - .count(closed) + .count(counts.closed) .selected(self.filter == IssueFilter::Closed) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = IssueFilter::Closed; diff --git a/crates/workspace/src/views/mod.rs b/crates/workspace/src/views/mod.rs index ba027ad..6c8caa1 100644 --- a/crates/workspace/src/views/mod.rs +++ b/crates/workspace/src/views/mod.rs @@ -8,6 +8,7 @@ mod repo; mod repo_list; mod send_patch; pub(crate) mod sidebar; +mod status_list; pub(crate) mod tree; pub use inbox::InboxView; diff --git a/crates/workspace/src/views/pull_requests/mod.rs b/crates/workspace/src/views/pull_requests/mod.rs index a1a4949..f27af31 100644 --- a/crates/workspace/src/views/pull_requests/mod.rs +++ b/crates/workspace/src/views/pull_requests/mod.rs @@ -25,6 +25,7 @@ pub(super) mod new; use self::detail::PullRequestDetailView; use self::new::open_new_pull_panel; use super::send_patch::open_send_patch_panel; +use super::status_list::{StatusCounts, filter_by_status}; use crate::views::repo::RepoAction; const ROW_HEIGHT: f32 = 73.; @@ -59,8 +60,7 @@ pub struct PullRequestsView { item_sizes: Rc>>, /// Indices into the store's `pull_requests` matching [`Self::filter`]. visible_prs: Vec, - /// Header counts `(total, open, closed, draft, merged)`. - counts: (usize, usize, usize, usize, usize), + counts: StatusCounts, // A filter change notifies even when the visible rows are unchanged, // e.g. switching between two empty filters. synced_filter: PullRequestFilter, @@ -93,7 +93,7 @@ impl PullRequestsView { filter: PullRequestFilter::Open, item_sizes: Rc::new(Vec::new()), visible_prs: Vec::new(), - counts: (0, 0, 0, 0, 0), + counts: StatusCounts::default(), synced_filter: PullRequestFilter::Open, scroll_handle: VirtualListScrollHandle::new(), _subscription: subscription, @@ -105,32 +105,15 @@ impl PullRequestsView { let (visible_prs, counts) = { let store = self.store.read(cx); - let mut counts = (0usize, 0usize, 0usize, 0usize, 0usize); - - let visible_prs: Vec = store + let roots = store .pull_requests .iter() - .enumerate() - .filter_map(|(ix, pr)| { - if pr.kind != Kind::GitPullRequest { - return None; - } - - let status = store.status_of(pr); - counts.0 += 1; - - match status { - RepoStatus::Open => counts.1 += 1, - RepoStatus::Closed => counts.2 += 1, - RepoStatus::Draft => counts.3 += 1, - RepoStatus::Applied => counts.4 += 1, - } - - filter.matches(status).then_some(ix) - }) - .collect(); - - (visible_prs, counts) + .filter(|pr| pr.kind == Kind::GitPullRequest); + filter_by_status( + roots, + |pr| store.status_of(pr), + |status| filter.matches(status), + ) }; let filter_changed = self.synced_filter != filter; @@ -236,7 +219,7 @@ impl PullRequestsView { } fn render_header(&self, cx: &mut Context) -> AnyElement { - let (total, open, closed, draft, merged) = self.counts; + let counts = self.counts; h_flex() .px_4() @@ -252,7 +235,7 @@ impl PullRequestsView { .child( SegmentButton::new("all", "All") .icon(Icon::new(CustomIconName::GitPullRequest)) - .count(total) + .count(counts.total) .selected(self.filter == PullRequestFilter::All) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::All; @@ -262,7 +245,7 @@ impl PullRequestsView { .child( SegmentButton::new("open", "Open") .icon(Icon::new(CustomIconName::GitPullRequest)) - .count(open) + .count(counts.open) .selected(self.filter == PullRequestFilter::Open) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::Open; @@ -272,7 +255,7 @@ impl PullRequestsView { .child( SegmentButton::new("closed", "Closed") .icon(Icon::new(CustomIconName::GitPullRequestClosed)) - .count(closed) + .count(counts.closed) .selected(self.filter == PullRequestFilter::Closed) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::Closed; @@ -282,7 +265,7 @@ impl PullRequestsView { .child( SegmentButton::new("draft", "Draft") .icon(Icon::new(CustomIconName::GitPullRequestDraft)) - .count(draft) + .count(counts.draft) .selected(self.filter == PullRequestFilter::Draft) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::Draft; @@ -292,7 +275,7 @@ impl PullRequestsView { .child( SegmentButton::new("merged", "Merged") .icon(Icon::new(CustomIconName::GitPullRequestMerged)) - .count(merged) + .count(counts.applied) .selected(self.filter == PullRequestFilter::Merged) .on_click(cx.listener(|this, _event, _window, cx| { this.filter = PullRequestFilter::Merged; diff --git a/crates/workspace/src/views/sidebar/grasp_servers.rs b/crates/workspace/src/views/sidebar/grasp_servers.rs index 84cd0cf..2776b4c 100644 --- a/crates/workspace/src/views/sidebar/grasp_servers.rs +++ b/crates/workspace/src/views/sidebar/grasp_servers.rs @@ -8,6 +8,8 @@ use nostr::prelude::*; use settings::{DEFAULT_GRASP_SERVERS, GraspServersSettings}; use signed_state::Backend; +use super::{normalize_server, server_host}; + /// State of the grasp-server section of a publish dialog, so async results can be rendered. #[derive(Default)] pub struct GraspServersState { @@ -155,7 +157,7 @@ fn render_server_row( .text_color(cx.theme().muted_foreground) .text_sm() .rounded(cx.theme().radius) - .child(display_server(relay)), + .child(server_host(relay)), ) .child( Button::new(format!("remove-relay:{ix}")) @@ -174,14 +176,6 @@ fn render_server_row( ) } -/// Shows only the host, since grasp servers are entered without a scheme. -fn display_server(relay: &RelayUrl) -> SharedString { - relay - .domain() - .map(SharedString::from) - .unwrap_or_else(|| SharedString::from(relay.to_string())) -} - /// Accepts a bare host as well as a full URL. fn add_relay( state: &Entity, @@ -194,14 +188,8 @@ fn add_relay( return; } - let normalized = if value.contains("://") { - value.clone() - } else { - format!("wss://{value}") - }; - - match RelayUrl::parse(&normalized) { - Ok(relay) => { + match normalize_server(&value) { + Some((_, relay)) => { state.update(cx, |state, _| { state.error = None; if !state.grasp_servers.contains(&relay) { @@ -210,7 +198,7 @@ fn add_relay( }); input.update(cx, |input, cx| input.set_value("", window, cx)); } - Err(_) => { + None => { state.update(cx, |state, _| { state.error = Some(format!("Invalid grasp server URL: {value}").into()); }); diff --git a/crates/workspace/src/views/sidebar/mod.rs b/crates/workspace/src/views/sidebar/mod.rs index 6dbb3e9..2da0a3e 100644 --- a/crates/workspace/src/views/sidebar/mod.rs +++ b/crates/workspace/src/views/sidebar/mod.rs @@ -17,6 +17,7 @@ use gpui_base::Button as BaseButton; use gpui_component::button::{Button, ButtonVariants}; use gpui_component::input::InputState; use gpui_component::{ActiveTheme, Icon, IconName, Sizable, StyledExt, h_flex, v_flex}; +use nostr::prelude::RelayUrl; use signed_core::{Announcement, RepoAddr, identifier_from_name}; use signed_state::{ Backend, BackendEvent, CheckoutsStore, LocalReposStore, Profile, ProfileStore, RepoListStore, @@ -542,6 +543,26 @@ impl SidebarPanel { } } +/// Normalize a user-typed grasp server, adding a `wss://` scheme when none is given. +/// +/// Returns the text to store and the parsed relay URL, or `None` when it is not a valid relay URL. +pub(super) fn normalize_server(input: &str) -> Option<(String, RelayUrl)> { + let text = if input.contains("://") { + input.to_owned() + } else { + format!("wss://{input}") + }; + RelayUrl::parse(&text).ok().map(|relay| (text, relay)) +} + +/// The host of a relay URL, which is what the server lists show; the scheme is implied. +pub(super) fn server_host(relay: &RelayUrl) -> SharedString { + relay + .domain() + .map(SharedString::from) + .unwrap_or_else(|| SharedString::from(relay.to_string())) +} + fn pick_banner() -> SharedString { let num = SystemTime::now() .duration_since(UNIX_EPOCH) diff --git a/crates/workspace/src/views/sidebar/settings_dialog.rs b/crates/workspace/src/views/sidebar/settings_dialog.rs index a0822f5..51e4e9c 100644 --- a/crates/workspace/src/views/sidebar/settings_dialog.rs +++ b/crates/workspace/src/views/sidebar/settings_dialog.rs @@ -18,10 +18,11 @@ use gpui_component::{ ActiveTheme, IconName, IndexPath, Sizable, Theme, ThemeMode, ThemeRegistry, WindowExt, h_flex, v_flex, }; -use nostr::prelude::RelayUrl; use settings::{AppearanceMode, Settings, SettingsStore}; use signed_ui::{SelectOption, setting_block, setting_row}; +use super::{normalize_server, server_host}; + /// Looks up the option index used to seed a [`SelectState`]. fn selected_index(options: &[SelectOption], value: &str) -> Option { options @@ -454,13 +455,11 @@ fn grasp_server_editor( } /// Shows only the host, since grasp servers are entered without a scheme. -/// Matches how the publish dialogs display servers. fn display_server(server: &str) -> SharedString { - RelayUrl::parse(server) - .ok() - .and_then(|relay| relay.domain().map(|domain| domain.to_owned())) - .map(SharedString::from) - .unwrap_or_else(|| SharedString::from(server.to_owned())) + match normalize_server(server) { + Some((_, relay)) => server_host(&relay), + None => SharedString::from(server.to_owned()), + } } fn repositories_section( @@ -568,14 +567,9 @@ fn add_server(input: &Entity, window: &mut Window, cx: &mut App) { if value.is_empty() { return; } - let normalized = if value.contains("://") { - value - } else { - format!("wss://{value}") - }; - if RelayUrl::parse(&normalized).is_err() { + let Some((normalized, _)) = normalize_server(&value) else { return; - } + }; let store = SettingsStore::global(cx); store.update(cx, |store, cx| { diff --git a/crates/workspace/src/views/status_list.rs b/crates/workspace/src/views/status_list.rs new file mode 100644 index 0000000..83badd4 --- /dev/null +++ b/crates/workspace/src/views/status_list.rs @@ -0,0 +1,45 @@ +use nostr::prelude::Event; +use signed_core::RepoStatus; + +/// Root events counted by their resolved status. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub(crate) struct StatusCounts { + pub(crate) total: usize, + pub(crate) open: usize, + pub(crate) closed: usize, + pub(crate) draft: usize, + pub(crate) applied: usize, +} + +impl StatusCounts { + fn record(&mut self, status: RepoStatus) { + self.total += 1; + match status { + RepoStatus::Open => self.open += 1, + RepoStatus::Closed => self.closed += 1, + RepoStatus::Draft => self.draft += 1, + RepoStatus::Applied => self.applied += 1, + } + } +} + +/// Indices of `roots` whose status `keep` accepts, counting every root's status. +pub(crate) fn filter_by_status<'a>( + roots: impl IntoIterator, + status_of: impl Fn(&Event) -> RepoStatus, + keep: impl Fn(RepoStatus) -> bool, +) -> (Vec, StatusCounts) { + let mut counts = StatusCounts::default(); + + let visible = roots + .into_iter() + .enumerate() + .filter_map(|(index, root)| { + let status = status_of(root); + counts.record(status); + keep(status).then_some(index) + }) + .collect(); + + (visible, counts) +} diff --git a/docs/over-engineering-cleanup-plan.md b/docs/over-engineering-cleanup-plan.md index 7a3940a..d15b11a 100644 --- a/docs/over-engineering-cleanup-plan.md +++ b/docs/over-engineering-cleanup-plan.md @@ -199,30 +199,44 @@ Verify the duplication before extracting; each could be intentional. Files: `crates/workspace/src/views/pull_requests/mod.rs`, `crates/workspace/src/views/issues/mod.rs` -- [ ] Confirm the filter enum, visible-index rebuild, counts tuple, and - virtual-list resize are the same shape. -- [ ] If so, extract one small helper for the filtered index + counts + notify - decision and use it in both. +- [x] Confirm the shape. The two `rebuild`s are the same mechanic: one pass over a + root list, per-status counts, keep matching indices, early-return when + filter/indices/counts are unchanged, resize the item sizes, notify. +- [x] Extract `crates/workspace/src/views/status_list.rs` with `StatusCounts` and + `filter_by_status`. Both views now use it; the tuple counts were replaced by + `StatusCounts`. The notify decision stays local because it would need a trait + over the two filter enums. ### 3.2 Relay URL normalize/display Files: `crates/workspace/src/views/sidebar/settings_dialog.rs`, `crates/workspace/src/views/sidebar/grasp_servers.rs` -- [ ] Confirm both pairs do prepend-scheme, parse, dedupe, and host-display. -- [ ] Extract one normalize helper and one display helper. Decide the crate - (check whether `signed_ui` may depend on `nostr`). +- [x] Confirm both pairs do prepend-scheme, parse, dedupe, and host-display. +- [x] Extract `normalize_server` and `server_host` into `sidebar/mod.rs`. They live + in `workspace`, not `signed_ui`: `signed_ui` does not depend on `nostr`, and + these are used only by the two sidebar modules. Dedupe differs per caller + (`Vec` vs persisted `Vec`) and stays at the call site. ### 3.3 `crates/dock` vs the pinned `gpui_component` dock renderer Files: `crates/dock/src/*` vs the pinned rev's `crates/ui/src/dock/*` -- [ ] Spike only: pick one part (`SignedTabGroupSkin` or `SignedTilesSkin`) and - determine whether it can delegate to the upstream `DockSkin` trait - implementation and keep only the Signed deltas (window controls in the tab - bar, plain-sidebar detection, prev/next, i18n). -- [ ] Report effort and risk before doing any replacement. Do not start a - rewrite of this crate in this cleanup. +- [x] Spike: `SignedTabGroupSkin` cannot delegate to the pinned upstream skin. + - `TabGroupSkin`, `TilesSkin` and `SkinShared` are `pub(crate)` in + `gpui_component::ui`; only the opaque `DockSkin` renderer is public, and it + holds that private shared state. + - `TabGroupRenderer`/`TilesRenderer` are all-or-nothing per method. The + Signed deltas (window controls, prev/next, plain-sidebar detection, i18n) + live *inside* `render_tab_bar` and `frame`. There is no hook below the whole + method, so "delegate and keep only the deltas" has no seam to hang on. + - Composing `Rc` would still leave `render_tab_bar` a near-full + reimplementation while adding a dependency on upstream internals, for no + line reduction. +- [x] Effort/risk: high effort, high churn, no achievable reduction on this rev. + `SignedTilesSkin` is the same shape. Recommend keeping the fork as-is. A + future upstream change (public `DockSkin` with per-part hooks) would be the + precondition for any delegation. ---