Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor NotebookCellOutline layers to remove target + breadcrumb fix #212002

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Yoyokrazy
Copy link
Contributor

Fixes: #211998

This is conveniently part of the ongoing outline refactor to pull filters out of the primary outline provider, in favor of filtering at higher layers.

Previously, the recomputeActive() call within NotebookCellOutlineProvider was only resetting the outline activeElement according to passing the three outlinePane filters for headers, code cells, and symbols. This activeElement was also used by the breadcrumbsDataSource, which meant that if you have code cells filtered out of your outline pane, 🐛 you will never see your breadcrumbs update as long as you click through code cells (because the active element is never getting set).

The fix:

  • remove filters from the provider. NotebookCellOutlineProvider.activeElement will now always hold the active entry, even if it will be filtered down the line.
  • add in a new getter for activeBreadcrumbsElement. This allows us to filter code cells out of the outline pane while still showing them in breadcrumbs (if that's something someone would ever do... regardless.)

New behavior to the outline pane:
This is only really relevant if you have Follow Cursor enabled for the view. Previously, if you were clicking around in cells that weren't directly displayed in the outline, the highlight in the outline just wouldn't move. Now, the selection will shift to the nearest valid parent OutlineEntry of the cell you selected. Makes this feature a touch more useful.

Current behavior:

20240504-0007-03.2021220.mp4

New Proposed behavior:

20240504-0010-56.7367964.mp4

@Yoyokrazy Yoyokrazy requested a review from rebornix May 4, 2024 00:19
@Yoyokrazy Yoyokrazy self-assigned this May 4, 2024
@VSCodeTriageBot VSCodeTriageBot added this to the May 2024 milestone May 4, 2024
@Yoyokrazy Yoyokrazy enabled auto-merge (squash) May 4, 2024 00:22
@Yoyokrazy Yoyokrazy disabled auto-merge May 8, 2024 00:07
@Yoyokrazy
Copy link
Contributor Author

Latest commit includes more refactor work:

  • OutlineTarget is removed from all layers except the class NotebookCellOutline
    • target here only controls config and options for the outline trees, the outline renderer, and setFullSymbols (which will need to be moved anyway in a further refactor that addresses more perf work)
  • the 3 data sources for the IOutline now just hold a reference to the primary NotebookCellOutlineDataSource
  • bit of fixing for testing to reflect shape changes of the NotebookCellOutline and it's view providers/data sources

@Yoyokrazy Yoyokrazy changed the title Remove some filters from NotebookCellOutlineProvider + breadcrumb fix Refactor NotebookCellOutline layers to remove target + breadcrumb fix May 8, 2024
// find a valid parent
let parent = newActive.parent;
while (parent) {
if (this.filterEntry(parent)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some potential perf optimization:

  • we are reading 3 settings each time when we read filterEntry, they can probably be cached as they won't change in this while loop.
  • minor: maybe we can cache the active entry so we don't need to recompute if activeElement doesn't change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notebook breadcrumbs are tied to OutlinePane filters
4 participants