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

Core 617 orphan collection #215

Merged
merged 4 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ $ poet shrink /path/to/osbooks-college-algebra-bundle precalculus-2e:0,3 algebra

# Publishing

We rely on [this Concourse pipeline](https://github.com/openstax/concourse-pipelines/tree/master/release-poet) to publish a new version to the [VSCode Marketplace](https://marketplace.visualstudio.com/items?itemName=openstax.editor) and [OpenVSX.org marketplace](https://open-vsx.org/extension/openstax/editor). That pipeline uses the `build-for-release.sh`, `publish-openvsx.sh`, and `publish-vsce.sh` scripts in the `scripts` directory for publishing. It's not recommended that you run these scripts yourself, but you can if you have the correct credentials.
We rely on [this Concourse pipeline](https://github.com/openstax/ce-pipelines/tree/main/pipelines/release-vscode-extension/) to publish a new version to the [VSCode Marketplace](https://marketplace.visualstudio.com/items?itemName=openstax.editor) and [OpenVSX.org marketplace](https://open-vsx.org/extension/openstax/editor). That pipeline uses the `build-for-release.sh`, `publish-openvsx.sh`, and `publish-vsce.sh` scripts in the `scripts` directory for publishing. It's not recommended that you run these scripts yourself, but you can if you have the correct credentials.

Example running publish from repo root:
```sh
Expand Down
39 changes: 37 additions & 2 deletions client/specs/__snapshots__/book-tocs.spec.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -139,15 +139,50 @@ exports[`Toc Provider returns tree items for children 3`] = `

exports[`Toc Provider returns tree items for children 4`] = `
{
"collapsibleState": "Collapsed",
"collapsibleState": "None",
"command": {
"arguments": [
{
"$mid": 1,
"path": "/path/to/ancillary",
"scheme": "file",
},
],
"command": "vscode.open",
"title": "open",
},
"contextValue": "rename",
"iconPath": "Folder",
"description": "fileId",
"iconPath": "File",
"label": "title",
"resourceUri": {
"$mid": 1,
"path": "/path/to/ancillary",
"scheme": "file",
},
}
`;

exports[`Toc Provider returns tree items for children 5`] = `
{
"collapsibleState": "Collapsed",
"iconPath": "Folder",
"label": "Orphaned Modules",
}
`;

exports[`filtering toggleTocTreesFilteringHandler 1`] = `
[
[
{
"children": [],
"type": "ClientOnlyTocKinds.OrphanCollection",
"value": undefined,
},
{
"expand": 3,
},
],
[
{
"children": [],
Expand Down
14 changes: 10 additions & 4 deletions client/specs/book-tocs.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type vscode from 'vscode'
import { expect } from '@jest/globals'

import { BookRootNode, type BookToc, type ClientTocNode, TocNodeKind } from '../../common/src/toc'
import { type BookOrTocNode, TocsTreeProvider, toggleTocTreesFilteringHandler } from '../src/book-tocs'
import { type BookOrTocNode, ClientOnlyTocKinds, TocsTreeProvider, toggleTocTreesFilteringHandler } from '../src/book-tocs'

const testTocPage: ClientTocNode = {
type: TocNodeKind.Page,
Expand Down Expand Up @@ -38,13 +38,18 @@ const testToc: BookToc = {
licenseUrl: 'licenseUrl',
tocTree: [testTocSubbook]
}
const testOrphanCollection = {
type: ClientOnlyTocKinds.OrphanCollection,
children: []
}
describe('Toc Provider', () => {
const p = new TocsTreeProvider()
it('returns tree items for children', () => {
expect(p.getTreeItem(testToc)).toMatchSnapshot()
expect(p.getTreeItem(testTocSubbook)).toMatchSnapshot()
expect(p.getTreeItem(testTocPage)).toMatchSnapshot()
expect(p.getTreeItem(testTocAncillary)).toMatchSnapshot()
expect(p.getTreeItem(testOrphanCollection)).toMatchSnapshot()
})
it('filters fileids when filtering is set', () => {
const p = new TocsTreeProvider()
Expand All @@ -64,7 +69,7 @@ describe('Toc Provider', () => {
})
it('gets children and parents', () => {
p.update([testToc], [])
expect(p.getChildren()).toEqual([testToc])
expect(p.getChildren()).toEqual([testToc, { children: [], type: ClientOnlyTocKinds.OrphanCollection, value: undefined }])
expect(p.getParent(testToc)).toBe(undefined)
expect(p.getChildren(testToc)).toEqual(testToc.tocTree)
expect(p.getParent(testToc.tocTree[0])).toBe(testToc)
Expand Down Expand Up @@ -92,15 +97,16 @@ describe('filtering', () => {
} as unknown as TocsTreeProvider
const fakeChildren = [
{ type: BookRootNode.Singleton, tocTree: [{ type: TocNodeKind.Subbook, label: 'unit1', children: [{ type: TocNodeKind.Subbook, label: 'subcol1', children: [{ type: TocNodeKind.Page, label: 'm2', children: [] }] }] }] },
{ type: BookRootNode.Singleton, tocTree: [{ label: 'm1', children: [] }] }
{ type: BookRootNode.Singleton, tocTree: [{ label: 'm1', children: [] }] },
{ type: ClientOnlyTocKinds.OrphanCollection, children: [], value: undefined }
]
getChildrenStub.returns(fakeChildren)

const handler = toggleTocTreesFilteringHandler(view, provider)
await handler()
expect(toggleFilterStub.callCount).toBe(1)
expect(getChildrenStub.callCount).toBe(1)
expect(revealStub.callCount).toBe(2)
expect(revealStub.callCount).toBe(3)
expect(revealStub.getCalls().map(c => c.args)).toMatchSnapshot()
expect(refreshStub.callCount).toBe(0)
})
Expand Down
7 changes: 7 additions & 0 deletions client/specs/tocs-event-handler.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -438,5 +438,12 @@ describe('TocsEventHandler', () => {
expect(removeNodeStub).not.toHaveBeenCalled()
expect(moveNodeStub).toHaveBeenCalledWith(orphanedPage, testTocPage)
})
it('calls removeNode when the target is an orphan collection', async () => {
const dt = { get: () => ({ value: testTocPage }) } as unknown as DataTransfer
const orphanCollection = tocsTreeProvider.getChildren().pop()
await withSubbedEvents.handleDrop(orphanCollection, dt)
expect(removeNodeStub).toHaveBeenCalledWith(testTocPage)
expect(moveNodeStub).not.toHaveBeenCalled()
})
})
})
84 changes: 51 additions & 33 deletions client/src/book-tocs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,15 @@ import { EventEmitter, type TreeItem, TreeItemCollapsibleState, Uri, type TreeDa
import { type BookToc, type ClientTocNode, BookRootNode, TocNodeKind, type ClientPageish } from '../../common/src/toc'
import type vscode from 'vscode'

export type BookOrTocNode = BookToc | ClientTocNode
export enum ClientOnlyTocKinds {
OrphanCollection = 'ClientOnlyTocKinds.OrphanCollection'
}

interface OrphanCollection {
type: ClientOnlyTocKinds.OrphanCollection
children: BookOrTocNode[]
}
export type BookOrTocNode = BookToc | ClientTocNode | OrphanCollection

const toClientTocNode = (n: ClientPageish): ClientTocNode => ({ type: TocNodeKind.Page, value: n })

Expand All @@ -19,12 +27,12 @@ export class TocsTreeProvider implements TreeDataProvider<BookOrTocNode> {

public includeFileIdsForFilter = false
private bookTocs: BookToc[]
private orphans: BookOrTocNode[]
private readonly orphanCollection: OrphanCollection
private readonly parentsMap = new Map<BookOrTocNode, BookOrTocNode>()

constructor() {
this.bookTocs = []
this.orphans = []
this.orphanCollection = { type: ClientOnlyTocKinds.OrphanCollection, children: [] }
}

public toggleFilterMode() {
Expand All @@ -36,7 +44,7 @@ export class TocsTreeProvider implements TreeDataProvider<BookOrTocNode> {
this.bookTocs = n
this.parentsMap.clear()
this.bookTocs.forEach(n => { this.recAddParent(n) })
this.orphans = o.map(toClientTocNode)
this.orphanCollection.children = o.map(toClientTocNode)
this._onDidChangeTreeData.fire()
}

Expand All @@ -55,42 +63,52 @@ export class TocsTreeProvider implements TreeDataProvider<BookOrTocNode> {
if (this.getParent(node) !== undefined) {
capabilities.push('delete')
}
if (node.type === BookRootNode.Singleton) {
const uri = Uri.parse(node.absPath)
return {
iconPath: TocItemIcon.Book,
collapsibleState: TreeItemCollapsibleState.Collapsed,
label: node.title,
description: node.slug,
resourceUri: uri,
command: { title: 'open', command: 'vscode.open', arguments: [uri] }
switch (node.type) {
case BookRootNode.Singleton: {
const uri = Uri.parse(node.absPath)
return {
iconPath: TocItemIcon.Book,
collapsibleState: TreeItemCollapsibleState.Collapsed,
label: node.title,
description: node.slug,
resourceUri: uri,
command: { title: 'open', command: 'vscode.open', arguments: [uri] }
}
}
} else if (node.type === TocNodeKind.Page) {
const uri = Uri.parse(node.value.absPath)
const title = node.value.title ?? 'Loading...'
const ret = this.includeFileIdsForFilter ? { label: `${title} (${node.value.fileId})` } : { label: title, description: node.value.fileId }
return {
...ret,
iconPath: TocItemIcon.Page,
collapsibleState: TreeItemCollapsibleState.None,
resourceUri: uri,
command: { title: 'open', command: 'vscode.open', arguments: [uri] },
contextValue: capabilities.join(',')
}
} else {
return {
iconPath: TocItemIcon.Subbook,
collapsibleState: TreeItemCollapsibleState.Collapsed,
label: node.value.title,
contextValue: capabilities.join(',')
case TocNodeKind.Ancillary:
case TocNodeKind.Page: {
const uri = Uri.parse(node.value.absPath)
const title = node.value.title ?? 'Loading...'
const ret = this.includeFileIdsForFilter ? { label: `${title} (${node.value.fileId})` } : { label: title, description: node.value.fileId }
return {
...ret,
iconPath: TocItemIcon.Page,
collapsibleState: TreeItemCollapsibleState.None,
resourceUri: uri,
command: { title: 'open', command: 'vscode.open', arguments: [uri] },
contextValue: capabilities.join(',')
}
}
case TocNodeKind.Subbook:
return {
iconPath: TocItemIcon.Subbook,
collapsibleState: TreeItemCollapsibleState.Collapsed,
label: node.value.title,
contextValue: capabilities.join(',')
}
case ClientOnlyTocKinds.OrphanCollection:
return {
iconPath: TocItemIcon.Subbook,
collapsibleState: TreeItemCollapsibleState.Collapsed,
label: 'Orphaned Modules'
}
}
}

public getChildren(node?: BookOrTocNode) {
let kids: BookOrTocNode[] = []
if (node === undefined) {
return [...this.bookTocs, ...this.orphans]
kids = [...this.bookTocs, this.orphanCollection]
} else if (node.type === BookRootNode.Singleton) {
kids = node.tocTree
} else if (node.type === TocNodeKind.Page || node.type === TocNodeKind.Ancillary) {
Expand Down Expand Up @@ -136,7 +154,7 @@ export function toggleTocTreesFilteringHandler(view: vscode.TreeView<BookOrTocNo
// is fully expanded. This approach is used since attempting to simply call
// reveal on root notes with the max expand value of 3 doesn't seem to always
// fully expose leaf nodes for large trees.
function leafFinder(acc: ClientTocNode[], elements: BookOrTocNode[]) {
function leafFinder(acc: Array<ClientTocNode | OrphanCollection>, elements: BookOrTocNode[]) {
for (const el of elements) {
if (el.type === BookRootNode.Singleton) {
leafFinder(acc, el.tocTree)
Expand Down
80 changes: 46 additions & 34 deletions client/src/tocs-event-handler.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import vscode from 'vscode'
import { type TocsTreeProvider, type BookOrTocNode } from './book-tocs'
import { type TocModification, TocModificationKind, type TocModificationParams, TocNodeKind, BookRootNode, type CreatePageEvent, type CreateSubbookEvent, type CreateAncillaryEvent } from '../../common/src/toc'
import { type TocsTreeProvider, type BookOrTocNode, ClientOnlyTocKinds } from './book-tocs'
import { type TocModification, TocModificationKind, type TocModificationParams, TocNodeKind, BookRootNode, type CreatePageEvent, type CreateSubbookEvent, type CreateAncillaryEvent, isClientTocNode } from '../../common/src/toc'
import { ExtensionServerRequest } from '../../common/src/requests'
import { expect, getRootPathUri } from './utils'
import { type ExtensionHostContext } from './panel'
Expand All @@ -17,6 +17,8 @@ export const validateTitle = (title: string) => {

export const XFER_ITEM_ID = 'application/vnd.code.tree.tocTrees'

type TocEvent = TocModification | CreateSubbookEvent | CreatePageEvent | CreateAncillaryEvent

export class TocsEventHandler implements vscode.TreeDragAndDropController<BookOrTocNode> {
constructor(
private readonly tocTreesProvider: TocsTreeProvider,
Expand All @@ -30,7 +32,7 @@ export class TocsEventHandler implements vscode.TreeDragAndDropController<BookOr
return expect(getRootPathUri(), 'Could not get root path uri').toString()
}

private async fireEvent(event: TocModification | CreateSubbookEvent | CreatePageEvent | CreateAncillaryEvent) {
private async fireEvent(event: TocEvent) {
const workspaceUri = this.workspaceUri
const params: TocModificationParams = { workspaceUri, event }
await this.context.client.sendRequest(
Expand Down Expand Up @@ -126,41 +128,43 @@ export class TocsEventHandler implements vscode.TreeDragAndDropController<BookOr
}
}
}
if (nodeType === TocNodeKind.Page) {
const event: CreatePageEvent = {
type: TocNodeKind.Page,
title,
bookIndex,
parentNodeToken
}
await this.fireEvent(event)
} else if (nodeType === TocNodeKind.Subbook) {
const event: CreateSubbookEvent = {
type: TocNodeKind.Subbook,
title,
slug,
bookIndex,
parentNodeToken
}
await this.fireEvent(event)
} else if (nodeType === TocNodeKind.Ancillary) {
const event: CreateAncillaryEvent = {
type: TocNodeKind.Ancillary,
title,
bookIndex,
parentNodeToken
}
await this.fireEvent(event)
let event: CreatePageEvent | CreateSubbookEvent | CreateAncillaryEvent
switch (nodeType) {
case TocNodeKind.Page:
event = {
type: TocNodeKind.Page,
title,
bookIndex,
parentNodeToken
}
break
case TocNodeKind.Subbook:
event = {
type: TocNodeKind.Subbook,
title,
slug,
bookIndex,
parentNodeToken
}
break
case TocNodeKind.Ancillary:
event = {
type: TocNodeKind.Ancillary,
title,
bookIndex,
parentNodeToken
}
break
}
await this.fireEvent(event)
}

async renameNode(node: BookOrTocNode) {
// TODO Implement the rename functionality using inline editing (wait for the API to be available)
// https://github.com/microsoft/vscode/issues/97190
// https://stackoverflow.com/questions/70594061/change-an-existing-label-name-in-tree-view-vscode-extension
let oldTitle: string | undefined = ''
if (node.type !== BookRootNode.Singleton && 'title' in node.value) {
/* istanbul ignore next */
if (isClientTocNode(node)) {
Copy link
Member

Choose a reason for hiding this comment

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

The naming seems a bit confusing because isClientTocNode and ClientOnlyTocKinds seem to mean opposite things. Maybe isServerTocNode? Or isClientOnlyTocNode and invert the logic?

Copy link
Contributor Author

@TylerZeroMaster TylerZeroMaster Jan 14, 2025

Choose a reason for hiding this comment

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

What about isTocLeafNode? The function is for checking if the node can be moved/renamed/deleted (leaf nodes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this was a little confusing. My goal was to illustrate that the orphan collection is something that is specific to the tree view. I did not want to entangle it with the ClientTocNode. I replaced the ClientOnlyTocKinds enum with an OrphanCollectionKind constant in hopes of making the difference more clear.

oldTitle = node.value.title
}
const newTitle = await this.askTitle(oldTitle)
Expand Down Expand Up @@ -204,10 +208,18 @@ export class TocsEventHandler implements vscode.TreeDragAndDropController<BookOr

async handleDrop(target: BookOrTocNode | undefined, dataTransfer: vscode.DataTransfer): Promise<void> {
const dragging: BookOrTocNode | undefined = dataTransfer.get(XFER_ITEM_ID)?.value
if (dragging?.type === undefined) throw new Error('BUG: Bad drag target')
if (target === undefined) throw new Error('BUG: Bad drop target')
if (target !== dragging && dragging.type !== BookRootNode.Singleton) {
await this.moveNode(dragging, target)
if (dragging?.type === undefined) { throw new Error('BUG: Bad drag target') }
if (target === undefined) { throw new Error('BUG: Bad drop target') }
if (
target !== dragging &&
dragging.type !== ClientOnlyTocKinds.OrphanCollection &&
dragging.type !== BookRootNode.Singleton
) {
if (target.type === ClientOnlyTocKinds.OrphanCollection) {
await this.removeNode(dragging)
Copy link
Member

Choose a reason for hiding this comment

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

So they never get put in the OrphanCollection, they just go under it somehow? How does the OrphanCollection actually gets elements, like in your screenshot?

Copy link
Contributor Author

@TylerZeroMaster TylerZeroMaster Jan 14, 2025

Choose a reason for hiding this comment

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

  • Orphans are sent by the language server here after the remove event is handled.
  • This updates the client's toc tree with books and orphans.
  • Orphans are added to the OrphanCollection here on update.
  • The OrphanCollection is added as the last top level tree item here and rendered here and then the OrphanCollection's children are eventually returned here.

So the language server is telling the client which modules are orphaned and the client renders them under the orphan collection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, here's a video instead of a screenshot.

Screen.Recording.2025-01-14.at.5.06.57.PM.mov

} else {
await this.moveNode(dragging, target)
}
}
}
}
6 changes: 6 additions & 0 deletions common/src/toc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ export interface ClientAncillaryish { token: Token, title: string | undefined, f
export interface ClientSubbookish { token: Token, title: string }
export type ClientTocNode = TocNode<ClientSubbookish, (ClientPageish | ClientAncillaryish)>

export const isClientTocNode = (n: { type: unknown }): n is ClientTocNode => (
n.type === TocNodeKind.Page ||
n.type === TocNodeKind.Subbook ||
n.type === TocNodeKind.Ancillary
)

export interface BookToc {
readonly type: BookRootNode.Singleton
readonly absPath: string
Expand Down
Loading