Replace selection-driven browser blocklist with checkboxes - #48
Open
marcpbailey wants to merge 4 commits into
Open
Replace selection-driven browser blocklist with checkboxes#48marcpbailey wants to merge 4 commits into
marcpbailey wants to merge 4 commits into
Conversation
Blocklist membership was previously driven by table selection itself (selecting a row blocklisted it), which fought the list's own scroll-into-view behavior on every refresh — updateBlocklistTable() reselected rows from the blocklist on every reload, so an unrelated menu update mid-scroll would visibly jump the list back to the last blocklisted row. Discontiguous ⌘-click selection was also awkward since it doubled as both "browse" and "toggle membership". Each row now has its own checkbox as the source of truth for blocklist membership, fully decoupled from table selection: - BlocklistDelegate builds an icon + text + checkbox cell per row (view recycled via the standard makeView reuse pool) instead of relying on an IB-provided plain text cell plus selectionIndexesForProposedSelection. - checkboxToggled(sender:) reads/writes defaults.browserBlocklist directly; the primary browser's checkbox is disabled and its label greyed out, matching the previous primary-browser treatment. - updateBlocklistTable() is now a plain reload — no more reselecting rows to reflect blocklist state. - The "show blocklist contents if it's being used" check on launch now reads defaults.browserBlocklist.isEmpty instead of table selection, since selection no longer carries that meaning. - Added alternating row backgrounds and .plain table style for readability with the extra checkbox column. - Updated the explanatory label to describe checking/unchecking instead of selecting.
The explanatory label promises "check or uncheck multiple items by selecting more than one," but checkboxToggled only ever acted on the single clicked row (sender.tag) — a plain NSButton in a view-based table row has no built-in multi-row propagation the way a cell-based checkbox column does, so the label was describing behavior that didn't exist. checkboxToggled now checks whether the clicked row is part of the current selection; if so, it applies the same resulting state to every selected row (skipping the primary browser, which stays fixed) and reloads the table so all affected checkboxes reflect it.
Two issues found in hands-on testing: - The hand-built NSTableCellView had no autoresizing mask, so it never tracked the row's actual width as the column resized — it just kept whatever frame it had when first created, leaving the checkbox pinned near its original position regardless of window width. Setting autoresizingMask = [.width, .height] makes it stretch with the row the same way an IB-authored cell view would. - Clicking the checkbox made it (and its row) first responder, which reset the table's selection to just that row — clobbering a multi-row selection built up specifically to toggle several checkboxes at once via the fix in 48be286. refusesFirstResponder = true stops the checkbox from taking that focus while still firing its action and toggling normally.
Found via hands-on testing: - The blocklist column never actually resized with the table despite resizeWithTable="YES" / columnAutoresizingStyle="lastColumnOnly" in the XIB — confirmed by instrumenting column width across a live window resize, it stayed pinned at its initial XIB value the whole time, leaving a growing dead zone of table background to the right of the real content (and the checkbox looking "stuck"). Added a windowResized handler that calls sizeToFit() to force the column to track the table's actual width. - Toggling a checkbox cleared the table's current selection: setting browserBlocklist triggers a KVO observer (resetBrowsers -> updateBlocklistTable -> reloadData()), and reloadData() clears selection as a side effect. Capture/restore selectedRowIndexes around that reload. Also removed checkboxToggled's own redundant reloadData() call, which ran before the KVO-driven reload had a chance to preserve anything. - refusesFirstResponder on the checkbox (an earlier attempt at the selection-clearing bug above) turned out to fix nothing real, so removed it — it only cost keyboard/VoiceOver accessibility on the checkbox for no benefit. - The primary browser's row can now not be selected at all (shouldSelectRow), since its checkbox is always disabled — being selectable was just confusing when part of a multi-row toggle that silently skipped it.
Contributor
Author
|
I meant to post a screenshot. This one is from my fork where I've added Markdown Editor support in a tab (see related discusssion but fundamentally this change is confined to the blocklist only, so the tab is irrelevant for this PR. 
Improvements:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Blocklist membership was driven by table selection itself (selecting a row blocklisted it), which fought the list's own scroll-into-view behavior on every refresh and made discontiguous ⌘-click selection awkward. Each row now has its own checkbox as the source of truth, decoupled from selection — selecting a range and toggling one checkbox applies to the whole selection, the primary browser's row can't be selected or blocklisted, and the column now actually tracks the table's width on resize (it never did, despite the XIB's resizeWithTable/columnAutoresizingStyle settings).