Implement Ready tasks: sync scrolling, document map, auto-close

[hybridmachine]

Summary

Implemented the current Ready tasks from project board:

• P1-13: Synchronized scrolling (split views) #31 P1-13 Synchronized scrolling (split views)
• P1-14: Document Map (minimap) #33 P1-14 Document Map (minimap)
• P2-10: Auto-close brackets and quotes #38 P2-10 Auto-close brackets and quotes

What changed

• Added macos/platform/sync_scroll.h/.mm and integrated SCN_UPDATEUI handling in main/split views.
• Added macos/platform/document_map.h/.mm minimap with overlay viewport + click/drag navigation.
• Added macos/platform/auto_close.h/.mm (pair insertion, wrap-selection, skip-over, backspace pair delete via SCN_MODIFIED).
• Added command IDs and menu items for Synchronize Scrolling, Document Map, and Auto-close Brackets.
• Added state + persistence fields in app_state and settings_manager.
• Wired command dispatch and menu-state refresh in wndproc and split-view state updates.
• Updated macos/CMakeLists.txt to compile new modules.

Validation

• cmake --build macos/build --target MacOSNotePP
• cmake --build macos/build --target npp_macos_compile_test

Notes

• Sync scrolling is guarded against re-entrant notifications.
• Horizontal sync is skipped when wrap modes differ to avoid jitter.
• Document Map uses SCI_GETDOCPOINTER/SCI_SETDOCPOINTER to share active document state.

[hybridmachine]

PR Review: Implement Ready tasks — sync scrolling, document map, auto-close

Overall

Clean, well-structured PR that follows the codebase's modular pattern (separate .h/.mm per feature). All three features are properly integrated into the notification handlers, menu system, settings persistence, and split-view lifecycle. The plan review feedback about using SCI_SETDOCPOINTER for the document map and creating dedicated modules was addressed.

---

Bug: Quote auto-pairing is broken

File: auto_close.mm, lines 569–584

Quotes (`, ', "") satisfy both `isCloser()` and `isOpener()`. The code checks `isCloser(ch)` first and always returns from that block, so the opener auto-pair logic below is never reached for quote characters.

Result: typing `"` never auto-inserts a matching `"`. Only bracket pairs (`()`, `[]`, `{}`) work.

Fix: For quotes, check whether the next character is the same quote before entering the skip-over path. If not, fall through to the opener logic instead of returning:

```cpp
if (isCloser(ch) && !isQuote(ch))
{
// existing skip-over logic for brackets only
...
return;
}

// For quotes: check skip-over, then fall through to opener if no match
if (isQuote(ch))
{
intptr_t nextPos = pos;
int nextChar = static_cast(ScintillaBridge_sendMessage(sci, SCI_GETCHARAT, nextPos, 0));
if (nextChar == ch)
{
// skip-over logic
...
return;
}
// fall through to opener pairing
}
```

Bug: Single-character selection wrapping broken

File: auto_close.mm, `handleAutoCloseModified`, lines 656–683

Two `SC_MOD_BEFOREDELETE` checks exist:

1. Lines 656–663: catches `length == 1` for pair-delete tracking, then returns
2. Lines 666–684: captures deleted selection text for wrap behavior

When a 1-character selection is replaced by typing an opener, the first check matches (length 1) and returns early, so the selection-wrap detection never runs. Single-character selection wrapping is broken.

Fix: Don't return early from the first check — let it fall through. Restructure so both paths can run, or merge the two blocks with an `if/else` that distinguishes selection-delete from single-char backspace (check `SCI_GETSELECTIONEMPTY` first).

---

Medium Issues

Hardcoded layout constants in `document_map.mm`

`relayoutDocumentMap()` hardcodes `tabHeight = 28` and `statusHeight = 22`. If these change (e.g., toolbar hidden, retina scaling, system font size), the document map layout will be misaligned. Consider reading actual frame values from the existing tab bar and status bar views, or at minimum defining these as shared constants.

Redundant state toggle in `wndproc.mm` (minor)

The `IDM_VIEW_SYNCHRONIZE_SCROLLING` handler does:
```cpp
ctx().syncScrolling = !ctx().syncScrolling;
setSyncScrollingEnabled(ctx().syncScrolling);
```
`setSyncScrollingEnabled` just sets `ctx().syncScrolling = enabled` — identical to what was already done. Either call only `setSyncScrollingEnabled` with the toggled value, or remove the call.

Duplicate macro (pre-existing but now at two locations)

`INDIC_ROUNDBOX` is defined twice in `npp_constants.h` (lines 383 and 447). This is pre-existing on `macos-port` but worth cleaning up while you're editing this file.

---

Style/Minor

Magic style numbers in `isLikelyStringOrCommentStyle` — The long lists of numeric style IDs (`style == 1 || style == 2 || ...`) are hard to verify. Consider adding inline comments mapping to SciLexer.h constant names (e.g., `1 /* SCE_C_COMMENT */`), or at least a header comment explaining the source.

Whitespace-only diffs — Many hunks in this PR are whitespace normalization (tab/space alignment) with no functional change. This makes the diff ~2x larger than needed and harder to review. Consider separating whitespace fixes from functional changes in future PRs, or using `git diff --ignore-space-change` to verify.

---

What looks good

• sync_scroll.mm: Clean reentrancy guard, correct wrap-mode check for horizontal sync, properly handles source/target view identification
• document_map.mm: Correct use of `SCI_SETDOCPOINTER` for document sharing, nice overlay implementation with click/drag navigation, proper lifecycle management (init/destroy/relayout)
• Split view integration: All three features correctly wired into both main and split view notification callbacks
• Settings persistence: All new settings properly added to `AppSettings`, serialized in JSON, loaded/saved at startup/exit
• Menu state management: `updateSplitMenuState()` correctly grays out sync scrolling when not split, `WM_INITMENUPOPUP` refreshes check states

[Copilot]

Pull request overview

Implements three “Ready” editor features in the macOS port (split-view synchronized scrolling, a minimap-style Document Map, and auto-close brackets/quotes) and wires them through commands, menus, settings persistence, and Scintilla notifications.

Changes:

• Added synchronized scrolling support for split views (hooks SCN_UPDATEUI to mirror vertical/horizontal scroll offsets with re-entrancy guard).
• Added Document Map (minimap) view with viewport overlay and click/drag navigation, bound to the active editor’s document.
• Added auto-close brackets/quotes handling (pair insertion, skip-over, wrap-selection, backspace pair delete) plus toggles and persistence.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
macos/platform/wndproc.mm	Adds command handling + menu state updates for the new toggles/features.
macos/platform/sync_scroll.mm	Implements split-view scroll synchronization logic.
macos/platform/sync_scroll.h	Declares sync-scrolling API used by notification handlers.
macos/platform/split_view.mm	Hooks new behaviors into split-view notification flow and updates split menu state.
macos/platform/split_view.h	Exposes updateSplitMenuState() for menu enable/disable logic.
macos/platform/settings_manager.mm	Persists new settings keys for auto-close, sync scrolling, and document map.
macos/platform/settings_manager.h	Adds new AppSettings fields for the implemented features.
macos/platform/npp_constants.h	Introduces new command IDs and additional Scintilla constants/notification codes.
macos/platform/menu_builder.mm	Adds View/Edit menu items for Document Map, Sync Scrolling, Auto-close Brackets.
macos/platform/document_map.mm	Implements minimap view, overlay viewport rendering, and navigation + binding logic.
macos/platform/document_map.h	Declares document map lifecycle + update hooks.
macos/platform/document_manager.mm	Ensures document map is rebound/updated when switching/adding/closing tabs.
macos/platform/auto_close.mm	Implements auto-close behavior for openers/closers, selection wrapping, and pair delete.
macos/platform/auto_close.h	Declares auto-close entry points for SCN_CHARADDED / SCN_MODIFIED.
macos/platform/app_state.h	Adds app-context state for new feature toggles and document map view pointers.
macos/platform/app_delegate.mm	Initializes/wires new modules on startup; persists new settings on terminate.
macos/CMakeLists.txt	Adds new modules to the macOS build target.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

handleAutoCloseModified() returns early for any SC_MOD_BEFOREDELETE with length == 1, which also occurs when replacing a 1-character selection (typing an opener over a single selected character). That prevents the selection-wrap path from capturing the deleted selection, so wrapping a 1-char selection won’t work. Consider only arming the backspace pair-delete logic when the selection is empty (e.g., SCI_GETSELECTIONEMPTY != 0) and/or when the delete is clearly a caret backspace/delete, not a selection replacement.

Suggested change

	char deleted = static_cast<char>(ScintillaBridge_sendMessage(sci, SCI_GETCHARAT, scn->position, 0));
	info.deleted = deleted;
	info.pos = scn->position;
	info.armed = true;
	return;
	// Only arm pair-delete logic when there is no active selection (caret backspace/delete),
	// so that selection replacements (including 1-char selections) can still be wrapped.
	intptr_t selectionEmpty = ScintillaBridge_sendMessage(sci, SCI_GETSELECTIONEMPTY, 0, 0);
	if (selectionEmpty != 0)
	{
	char deleted = static_cast<char>(ScintillaBridge_sendMessage(sci, SCI_GETCHARAT, scn->position, 0));
	info.deleted = deleted;
	info.pos = scn->position;
	info.armed = true;
	}

[Copilot]

The per-view state caches (s_lastDeleteByView / s_pendingWrapByView) are never cleared when a Scintilla view is destroyed (e.g., when unsplitting and destroying scintillaView2). Over time this can leak memory and may also cause incorrect behavior if a pointer address gets reused for a new view. Provide a small cleanup API (e.g., autoCloseOnViewDestroyed(void* sci)), and call it from view teardown paths.

Suggested change

	} // anonymous namespace
	void autoCloseOnViewDestroyed(void *sci)
	{
	if (!sci)
	{
	return;
	}
	// Remove any cached per-view state associated with this Scintilla view.
	s_lastDeleteByView.erase(sci);
	s_pendingWrapByView.erase(sci);
	}
	namespace
	{

[Copilot]

If ScintillaBridge_createView fails, initializeDocumentMap() returns leaving documentMapContainer added to the window and ctx().documentMapContainer set. That leaks the container view and also prevents later retries because the early guard (ctx().documentMapContainer) will keep returning. On failure, clean up the container (removeFromSuperview + nil) and reset any partially-initialized state.

Suggested change

	if (!ctx().documentMapScintilla)
	return;
	if (!ctx().documentMapScintilla)
	{
	[ctx().documentMapContainer removeFromSuperview];
	ctx().documentMapContainer = nil;
	ctx().documentMapScintilla = nullptr;
	return;
	}

[Copilot]

bindDocumentMapToActiveView() shares a Scintilla document via SCI_SETDOCPOINTER without managing the document reference count (Scintilla expects SCI_ADDREFDOCUMENT/SCI_RELEASEDOCUMENT when multiple views share a document). This can lead to use-after-free/double-free when tabs close or views are destroyed while the minimap still points at the document. Track the currently-bound doc pointer for the minimap, addref the new doc before setting it, and release the old doc when switching/destroying the map view.

[Copilot]

initializeDocumentMap() is called unconditionally at startup, which creates and attaches the minimap Scintilla view even when the feature is disabled in settings. This adds extra views/resources for all users and makes “disabled” effectively just “hidden”. Consider lazy-creating the minimap only when documentMapEnabled becomes true (and optionally destroying it when toggled off) to avoid unnecessary overhead.

Suggested change

	initializeDocumentMap();
	if (ctx().documentMapEnabled)
	initializeDocumentMap();

[Copilot]

prevChar reads SCI_GETCHARAT(pos - 2, ...) even when pos == 1 (typing an opener at the start of the document), which passes a negative position to Scintilla. Guard pos < 2 and treat the previous character as a boundary (e.g., '\0') in that case.

Suggested change

	const char prevChar = static_cast<char>(ScintillaBridge_sendMessage(sci, SCI_GETCHARAT, pos - 2, 0));
	char prevChar = '\0';
	if (pos >= 2)
	{
	prevChar = static_cast<char>(ScintillaBridge_sendMessage(sci, SCI_GETCHARAT, pos - 2, 0));
	}

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

setDocumentMapEnabled() calls initializeDocumentMap() whenever the container is missing, even when disabling (enabled == false). That can create the minimap view as a side effect of turning the feature off (or before first enable), which is surprising and defeats lazy-init. Consider changing this to only initialize when enabling (e.g., if (enabled && !ctx().documentMapContainer) initializeDocumentMap()).

Suggested change

	if (!ctx().documentMapContainer)
	if (enabled && !ctx().documentMapContainer)

[Copilot]

Toggling Auto-close Brackets only flips ctx().autoCloseBrackets but does not clear the per-view pending state held in auto_close.mm (pendingWrap / lastDelete). If the feature is disabled between a SC_MOD_BEFOREDELETE and the next SCN_CHARADDED, that stale state can remain armed and cause an unexpected wrap or pair-delete after re-enabling. Consider clearing the auto-close caches for the active view(s) when turning this off (e.g., call a reset helper such as autoCloseOnViewDestroyed(view) or add a dedicated reset function).