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.

Sign up for GitHub

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

fix: add an internal debounce to setOutline + move code to OutlineView #118

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

fix: add an internal debounce to setOutline + move code to OutlineView #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
16d1df0
fix: add an internal debounce to setOutline + move code [skip release]
aminya Apr 18, 2021
09ffa03
chore: add private/public to the OutlineView methods
aminya Apr 18, 2021
b1022b7
fix: make setOutline async
aminya Apr 18, 2021
20e95a4
test: fix the tests
aminya Apr 18, 2021
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
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@
"test.format": "prettier . --check",
"lint": "eslint . --fix",
"test.lint": "eslint .",
"test": "atom --test spec",
"test": "npm run clean && cross-env NODE_ENV=test rollup -c && atom --test spec",
"clean": "shx rm -rf dist",
"dev": "npm run clean && cross-env NODE_ENV=development rollup -c -w",
"build": "npm run clean && cross-env NODE_ENV=production rollup -c ",
"build": "npm run clean && cross-env NODE_ENV=production rollup -c",
"build-commit": "build-commit -o dist",
"bump": "ncu -u",
"prepare": "npm run build"
Expand Down
44 changes: 31 additions & 13 deletions spec/outline-view-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"use babel"

const { TextEditor } = require("atom")
import { TextEditor, Dock } from "atom"
import * as OutlinePackage from "../dist/main"
import { statuses } from "../dist/main"

Expand All @@ -11,14 +11,36 @@ import outlineMock from "./outlineMock.json"
// To run a specific `it` or `describe` block add an `f` to the front (e.g. `fit`
// or `fdescribe`). Remove the `f` to unfocus the block.

function createMockProvider() {
export function createMockProvider() {
return {
getOutline: () => outlineMock,
}
}

function getOutlineItem() {
const rightDock: Dock = atom.workspace.getRightDock()
let dockItems = rightDock.getPaneItems()
if (dockItems.length === 0) {
throw new Error("Outline is not added to the dock")
}
dockItems = rightDock.getPaneItems()
return dockItems.filter((item) => item.getTitle() === "Outline")[0]
}

describe("Outline view", () => {
let workspaceElement
let editor
let outlineContent
let outlineView

async function createMockOutline() {
atom.config.set("outline.sortEntries", false)
editor = new TextEditor()
await OutlinePackage.getOutline(editor)
outlineContent = workspaceElement.querySelector(".outline-content")
outlineView = getOutlineItem()
return { editor, outlineContent, outlineView }
}

beforeEach(async () => {
workspaceElement = atom.views.getView(atom.workspace)
Expand All @@ -31,19 +53,11 @@ describe("Outline view", () => {
await atom.packages.activatePackage("atom-ide-outline")

expect(atom.packages.isPackageLoaded("atom-ide-outline")).toBeTruthy()

spyOn(OutlinePackage.outlineProviderRegistry, "getProviderForEditor").and.returnValue(createMockProvider())
})

describe("getOutline", () => {
let editor
let outlineContent

beforeEach(async () => {
atom.config.set("outline.sortEntries", false)
editor = new TextEditor()
await OutlinePackage.getOutline(editor)
outlineContent = workspaceElement.querySelector(".outline-content")
await createMockOutline()
})

it("renders outline into HTML", () => {
Expand Down Expand Up @@ -78,14 +92,18 @@ describe("Outline view", () => {
})

describe("setStatus", () => {
beforeEach(async () => {
await createMockOutline()
})

it("presents status message correctly", () => {
const mockStatus = {
title: "Error message",
description: "Something went wrong",
}
statuses.mockStatus = mockStatus

OutlinePackage.setStatus("mockStatus")
outlineView.setStatus("mockStatus")

const statusHolder = workspaceElement.querySelector(".outline-content .status")
const title = statusHolder.querySelector("h1")
Expand All @@ -101,7 +119,7 @@ describe("Outline view", () => {
}
statuses.mockStatus = mockStatus

OutlinePackage.setStatus("mockStatus")
outlineView.setStatus("mockStatus")

const statusHolder = workspaceElement.querySelector(".outline-content .status")
const title = statusHolder.querySelector("h1")
Expand Down
41 changes: 10 additions & 31 deletions src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,25 @@ import { notifyError, largeness as editorLargeness } from "atom-ide-base/commons
import { isItemVisible } from "atom-ide-base/commons-ui"

export { statuses } from "./statuses" // for spec
import { statuses } from "./statuses"
import debounce from "lodash/debounce"

const subscriptions = new CompositeDisposable()

let view: OutlineView | undefined
export const outlineProviderRegistry = new ProviderRegistry<OutlineProvider>()

// let busySignalProvider: BusySignalProvider | undefined // service might be consumed late
// export let busySignalProvider: BusySignalProvider | undefined // service might be consumed late

export function activate() {
addCommands()
addObservers()
if (atom.config.get("atom-ide-outline.initialDisplay")) {
// initially show outline pane
toggleOutlineView().catch((e) => {
try {
toggleOutlineView()
} catch (e) {
notifyError(e)
})
}
}
}

Expand Down Expand Up @@ -98,7 +99,7 @@ async function editorChanged(editor?: TextEditor) {

// clean up if the editor editor is closed
editor.onDidDestroy(() => {
setStatus("noEditor")
view?.setStatus("noEditor")
})
)
}
Expand All @@ -115,7 +116,7 @@ export function revealCursor() {
}
}

export async function toggleOutlineView() {
export function toggleOutlineView() {
if (view === undefined) {
view = new OutlineView() // create outline pane
}
Expand All @@ -135,7 +136,7 @@ export async function toggleOutlineView() {

// Trigger an editor change whenever an outline is toggeled.
try {
await editorChanged(atom.workspace.getActiveTextEditor())
editorChanged(atom.workspace.getActiveTextEditor())
} catch (e) {
notifyError(e)
}
Expand All @@ -149,33 +150,11 @@ function getOutlintIfVisible(editor = atom.workspace.getActiveTextEditor()) {
return getOutline(editor)
}

export async function getOutline(editor = atom.workspace.getActiveTextEditor()) {
export function getOutline(editor = atom.workspace.getActiveTextEditor()) {
if (view === undefined) {
view = new OutlineView() // create outline pane
}
// editor
if (editor === undefined) {
return setStatus("noEditor")
}

// provider
const provider = outlineProviderRegistry.getProviderForEditor(editor)

if (!provider) {
return setStatus("noProvider")
}

// const busySignalID = `Outline: ${editor.getPath()}`
// busySignalProvider?.add(busySignalID)

const outline = await provider.getOutline(editor)
view.setOutline(outline?.outlineTrees ?? [], editor, Boolean(editorLargeness(editor as TextEditor)))

// busySignalProvider?.remove(busySignalID)
}

export function setStatus(id: "noEditor" | "noProvider") {
view?.presentStatus(statuses[id])
return view.setOutline(editor)
}

export { default as config } from "./config.json"
80 changes: 71 additions & 9 deletions src/outlineView.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,20 @@
import { TextEditor, Point } from "atom"
import { OutlineTree } from "atom-ide-base"
import { isItemVisible, scrollIntoViewIfNeeded } from "atom-ide-base/commons-ui"
import { statuses } from "./statuses"
import { outlineProviderRegistry } from "./main"
import { largeness as editorLargeness } from "atom-ide-base/commons-atom"

// exported for spec
export function getProviderForEditor(editor: TextEditor) {
if (process.env.NODE_ENV !== "test") {
return outlineProviderRegistry.getProviderForEditor(editor)
} else {
// create a mock outline
// eslint-disable-next-line
return require("../spec/outline-view-spec").createMockProvider()
}
}

export class OutlineView {
public element: HTMLDivElement
Expand All @@ -17,7 +31,10 @@ export class OutlineView {
/** cache of last rendered list used to avoid rerendering */
lastEntries: OutlineTree[] | undefined

constructor() {
private setOutlineTimeout: NodeJS.Timeout | undefined
private isRendering: boolean = false

public constructor() {
this.element = document.createElement("div")
this.element.classList.add("atom-ide-outline")

Expand All @@ -29,23 +46,63 @@ export class OutlineView {
this.outlineContent.classList.add("outline-content")
}

destroy() {
public destroy() {
this.element.remove()
}

getElement() {
public getElement() {
return this.element
}

getTitle() {
public getTitle() {
return "Outline"
}

getIconName() {
public getIconName() {
return "list-unordered"
}

setOutline(outlineTree: OutlineTree[], editor: TextEditor, isLarge: boolean) {
public setOutline(editor: TextEditor | undefined) {
return new Promise<void>((resolve) => {
Copy link
Member Author

@aminya aminya Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Another simpler approach is to not care about when this function is resolved, and use a sleep function in the tests. I actually started with that. But jasmine wasn't awaiting the sleep function either.

function sleep(ms = 1000) {
	return new Promise((resolve) => {
		setTimeout(resolve, ms)
	})
}

Copy link
Member Author

Choose a reason for hiding this comment

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

One other thing is to make a timer using a while loop. I haven't tried that though

  function sleep(delay) {
    const start = new Date().getTime();
    while (new Date().getTime() < start + delay) {
    	// do nothing
    };
  }

if (this.isRendering) {
// return if setOutline is already called and not finished yet.
resolve()
return
Copy link
Member

Choose a reason for hiding this comment

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

If secondary calls return here the timeout never gets cleared and reset below.

Copy link
Member Author

Choose a reason for hiding this comment

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

But this.isRendering becomes false inside the setTimeout callback. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yes so the workflow goes like this:

  1. Call setOutline; this.isRendering is set to true; timer is created.
  2. Call setoutline; this.isRendering is true so returns early; timer is not changed.

The second call should reset the timer before returning. Otherwise the clearTimeout(this.setOutlineTimeout) is redundant because it will only be called after the timeout has already completed and this.isRendering is set to false.

}
this.isRendering = true
// const busySignalID = `Outline: ${editor.getPath()}`
// busySignalProvider?.add(busySignalID)

// editor
if (editor === undefined) {
this.setStatus("noEditor")
resolve()
return
}

// provider
const provider = getProviderForEditor(editor)

if (!provider) {
this.setStatus("noProvider")
resolve()
return
}

if (this.setOutlineTimeout) {
clearTimeout(this.setOutlineTimeout)
}
this.setOutlineTimeout = setTimeout(async () => {
const outline = await provider.getOutline(editor)
this._setOutline(outline?.outlineTrees ?? [], editor, Boolean(editorLargeness(editor as TextEditor)))
// busySignalProvider?.remove(busySignalID)
this.isRendering = false
resolve()
}, 50) // 50ms internal debounce to prevent multiple calls
})
}

private _setOutline(outlineTree: OutlineTree[], editor: TextEditor, isLarge: boolean) {
// skip rendering if it is the same
// TIME 0.2-1.2ms // the check itself takes ~0.2-0.5ms, so it is better than rerendering
if (this.lastEntries !== undefined && hasEqualContent(outlineTree, this.lastEntries)) {
Expand All @@ -72,15 +129,20 @@ export class OutlineView {
this.outlineContent.appendChild(this.outlineList)
}

clearContent() {
private clearContent() {
this.outlineContent.innerHTML = ""
if (this.outlineList !== undefined) {
this.outlineList.dataset.editorRootScope = ""
}
this.lastEntries = undefined
}

presentStatus(status: { title: string; description: string }) {
public setStatus(id: "noEditor" | "noProvider") {
this.presentStatus(statuses[id])
this.isRendering = false
}

private presentStatus(status: { title: string; description: string }) {
this.clearContent()

const statusElement = status && generateStatusElement(status)
Expand All @@ -91,7 +153,7 @@ export class OutlineView {
}

// callback for scrolling and highlighting the element that the cursor is on
selectAtCursorLine(editor: TextEditor) {
public selectAtCursorLine(editor: TextEditor) {
const cursor = editor.getLastCursor()

// no cursor
Expand Down