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

update window title when the document name changes #1624

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
32 changes: 25 additions & 7 deletions v3/cypress/e2e/cfm.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,30 @@ import { CfmElements as cfm } from "../support/elements/cfm"
import { ComponentElements as c } from "../support/elements/component-elements"
import { TableTileElements as table } from "../support/elements/table-tile"

// TODO: update this with a test that checks the window title
context("CloudFileManager", () => {
beforeEach(function () {
function visitEmptyCodap() {
const queryParams = "?mouseSensor"
const url = `${Cypress.config("index")}${queryParams}`
cy.visit(url)
})
it("Opens Mammals example document via url parameter", () => {
const mammalsUrl = "https://codap-resources.s3.amazonaws.com/example-documents/documents/mammals.codap"
}

it("Opens Mammals document using different methods", () => {
cy.log("Opening via a url parameter")
const mammalsUrl = "https://codap-resources.concord.org/example-documents/documents/mammals.codap"
cy.visit(`${Cypress.config("index")}?url=${mammalsUrl}`)
cy.get(".codap-component.codap-case-table").contains(".title-bar", "Mammals").should("exist")
})
it("Opens Mammals example document via CFM Open dialog", () => {

// The title is not set to mammals because the CFM is not reading the name from the document
// nor is it looking at the filename in the URL. This behaves the same in CODAPv2.
// If this ever gets fixed it should be:
// cy.title().should("equal", "mammals - CODAP")
cy.title().should("equal", "Untitled Document - CODAP")

cy.log("Opening via the CFM Open dialog")
visitEmptyCodap()
cy.title().should("equal", "Untitled Document - CODAP")

// hamburger menu is hidden initially
cfm.getHamburgerMenuButton().should("exist")
cfm.getHamburgerMenu().should("not.exist")
Expand All @@ -31,16 +43,18 @@ context("CloudFileManager", () => {
// Selecting Mammals document should load the mammals example document
cfm.getModalDialog().contains(".filelist div.selectable", "Mammals").click()
cfm.getModalDialog().contains(".buttons button", "Open").click()
cy.wait(1000)

// once loaded, Open dialog should be hidden and document content should be shown
cfm.getModalDialog().should("not.exist")
cy.get(".codap-component.codap-case-table").contains(".title-bar", "Mammals").should("exist")
cy.title().should("equal", "Mammals - CODAP")
})
it("Opens a local document using different methods", () => {
const fileName = "../v3/cypress/fixtures/mammals.codap"
const CSVFileName = "../v3/cypress/fixtures/map-data.csv"

cy.log("Opens a CODAP document from a local file using CFM dialog")
visitEmptyCodap()

// Open the document from Hamburger menu
// Select file from dialog
Expand All @@ -55,6 +69,8 @@ context("CloudFileManager", () => {
.click({force:true})
cy.get('input[type=file]').selectFile(fileName)

cy.title().should("equal", "mammals - CODAP")

// Verify table in Mammals exists
table.getAttribute("Order").should("have.text", "Order")
table.getGridCell(2, 2).should("contain", "African Elephant")
Expand Down Expand Up @@ -95,6 +111,8 @@ context("CloudFileManager", () => {
c.checkComponentDoesNotExist("table")
})
it("verify language menu is present", () => {
visitEmptyCodap()

cfm.getLanguageMenuButton().should("exist")
cfm.getLanguageMenu().should("not.exist")
cfm.getLanguageMenuButton().click()
Expand Down
18 changes: 12 additions & 6 deletions v3/src/components/tool-shelf/tool-shelf.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { ToolShelf } from "./tool-shelf"
import { render, screen } from "@testing-library/react"
import { createCodapDocument } from "../../models/codap/create-codap-document"
import { IDocumentModel } from "../../models/document/document"
import { ITestTileContent, TestTileContent } from "../../test/test-tile-content"
import { TileModel } from "../../models/tiles/tile-model"

// way to get a writable reference to libDebug
const libDebug = require("../../lib/debug")
Expand Down Expand Up @@ -56,11 +58,13 @@ describe("Tool shelf", () => {
renderToolShelf(document)
expect(screen.getByTestId("tool-shelf")).toBeDefined()

document.setTitle("New Title")
const tile = TileModel.create({ id: "tile-1", content: TestTileContent.create() })
document.addTile(tile)
expect(document.content?.tileMap.size).toBe(1)
await user.click(screen.getByTestId("tool-shelf-button-undo"))
expect(document.title).toBe("New Title")
expect(document.content?.tileMap.size).toBe(1)
await user.click(screen.getByTestId("tool-shelf-button-redo"))
expect(document.title).toBe("New Title")
expect(document.content?.tileMap.size).toBe(1)
})

it("undo/redo buttons work as expected when enabled", async () => {
Expand All @@ -70,11 +74,13 @@ describe("Tool shelf", () => {
expect(screen.getByTestId("tool-shelf")).toBeDefined()

document.treeMonitor?.enableMonitoring()
document.setTitle("New Title")
const tile = TileModel.create({ id: "tile-1", content: TestTileContent.create() })
document.addTile(tile)
expect(document.content?.tileMap.size).toBe(1)
await user.click(screen.getByTestId("tool-shelf-button-undo"))
expect(document.title).toBeUndefined()
expect(document.content?.tileMap.size).toBe(0)
await user.click(screen.getByTestId("tool-shelf-button-redo"))
expect(document.title).toBe("New Title")
expect(document.content?.tileMap.size).toBe(1)
document.treeMonitor?.disableMonitoring()
})
})
17 changes: 15 additions & 2 deletions v3/src/lib/handle-cfm-event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@
if (!appState.isCurrentRevision(content.revisionId)) {
cfmClient.dirty(true)
}

// The filename might have changed when the document was saved
const filename = event.state?.metadata?.filename
if (filename) {
appState.document.setTitleFromFilename(filename)

Check warning on line 65 in v3/src/lib/handle-cfm-event.ts

View check run for this annotation

Codecov / codecov/patch

v3/src/lib/handle-cfm-event.ts#L65

Added line #L65 was not covered by tests
}
break
}
// case "sharedFile":
Expand All @@ -66,8 +72,15 @@
// break
// case "importedData":
// break
// case "renamedFile":
// break
case "renamedFile": {

Check warning on line 75 in v3/src/lib/handle-cfm-event.ts

View check run for this annotation

Codecov / codecov/patch

v3/src/lib/handle-cfm-event.ts#L75

Added line #L75 was not covered by tests
// Possible Improvement: Add a custom undo entry when the file is renamed.
// It would need to send the appropriate commands to the CFM to update the filename.
const filename = event.state?.metadata?.filename
if (filename) {
appState.document.setTitleFromFilename(filename)

Check warning on line 80 in v3/src/lib/handle-cfm-event.ts

View check run for this annotation

Codecov / codecov/patch

v3/src/lib/handle-cfm-event.ts#L80

Added line #L80 was not covered by tests
}
break

Check warning on line 82 in v3/src/lib/handle-cfm-event.ts

View check run for this annotation

Codecov / codecov/patch

v3/src/lib/handle-cfm-event.ts#L82

Added line #L82 was not covered by tests
}
// case "log":
// break
}
Expand Down
2 changes: 1 addition & 1 deletion v3/src/lib/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export class Logger {
if (!this._instance) return

const time = Date.now() // eventually we will want server skew (or to add this via FB directly)
const documentTitle = this._instance.document.title || "Untitled Document"
const documentTitle = this._instance.document.title
if (this._instance) {
this._instance.formatAndSend(time, event, documentTitle, category, args)
} else {
Expand Down
42 changes: 36 additions & 6 deletions v3/src/models/app-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
generally be MobX-observable.
*/
import { cloneDeep } from "lodash"
import { action, computed, makeObservable, observable, reaction, flow } from "mobx"
import { action, autorun, computed, makeObservable, observable, reaction, flow } from "mobx"
import { getSnapshot } from "mobx-state-tree"
import { CloudFileManager } from "@concord-consortium/cloud-file-manager"
import { createCodapDocument } from "./codap/create-codap-document"
Expand All @@ -24,6 +24,8 @@
import { CodapV2Document } from "../v2/codap-v2-document"
import { importV2Document } from "../v2/import-v2-document"

const kAppName = "CODAP"

type AppMode = "normal" | "performance"

type ISerializedDocumentModel = IDocumentModelSnapshot & {revisionId?: string}
Expand All @@ -42,6 +44,7 @@
private version = ""
private cfm: CloudFileManager | undefined
private dirtyMonitorDisposer: (() => void) | undefined
titleMonitorDisposer: any
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
titleMonitorDisposer: any
private titleMonitorDisposer: (() => void) | undefined


constructor() {
this.currentDocument = createCodapDocument()
Expand Down Expand Up @@ -120,13 +123,20 @@
if (metadata) {
const metadataEntries = Object.entries(metadata)
metadataEntries.forEach(([key, value]) => {
if (value != null) {
if (value == null) return

if (key === "filename") {
// We don't save the filename because it is redundant with the filename in the actual
// filesystem.
// However we need to the extension-less name for the window title
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// However we need to the extension-less name for the window title
// However we need the extension-less name for the window title.

or something like that.

// The CFM also expects the document to have a name field when the document
// is loaded from a filesystem that doesn't use filenames.
this.currentDocument.setTitleFromFilename(value)
} else {

Check warning on line 135 in v3/src/models/app-state.ts

View check run for this annotation

Codecov / codecov/patch

v3/src/models/app-state.ts#L135

Added line #L135 was not covered by tests
this.currentDocument.setProperty(key, value)
}
})
}
const docTitle = this.currentDocument.getDocumentTitle()
this.currentDocument.setTitle(docTitle || t("DG.Document.defaultDocumentName"))
if (content.revisionId && this.treeManager) {
// Restore the revisionId from the stored document
// This will allow us to consistently compare the local document
Expand All @@ -153,22 +163,42 @@

@action
enableDocumentMonitoring() {
this.currentDocument?.treeMonitor?.enableMonitoring()
if (this.currentDocument && !this.dirtyMonitorDisposer) {
if (!this.currentDocument) return

this.currentDocument.treeMonitor?.enableMonitoring()
if (!this.dirtyMonitorDisposer) {
this.dirtyMonitorDisposer = reaction(
() => this.treeManager?.revisionId,
() => {
this.cfm?.client.dirty(true)
}
)
}

// TODO: look for tests of opening documents so we can update them to check
// the title
if (!this.titleMonitorDisposer) {
this.titleMonitorDisposer = autorun(() => {
const { title } = this.currentDocument

// TODO: handle componentMode and embeddedMode
// if ((DG.get('componentMode') === 'yes') || (DG.get('embeddedMode') === 'yes')) {
// return;
// }

const titleString = t("DG.main.page.title", {vars: [title, kAppName]})
window.document.title = titleString
})
}
}

@action
disableDocumentMonitoring() {
this.currentDocument?.treeMonitor?.disableMonitoring()
this.dirtyMonitorDisposer?.()
this.dirtyMonitorDisposer = undefined
this.titleMonitorDisposer?.()
this.titleMonitorDisposer = undefined
}

@action
Expand Down
1 change: 1 addition & 0 deletions v3/src/models/codap/create-codap-document.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ interface IOptions {
}
export function createCodapDocument(snapshot?: ICodapDocumentModelSnapshot, options?: IOptions): IDocumentModel {
const { layout = "free", noGlobals = false } = options || {}
// Note: The version and build will not be updated after the document is first created
Copy link
Member

Choose a reason for hiding this comment

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

Can/should we do something about that? Seems like the expectation would be that it would be the version that most recently saved it.

const document = createDocumentModel({ type: "CODAP", version, build: `${buildNumber}`, ...snapshot })
// create the content if there isn't any
if (!document.content) {
Expand Down
8 changes: 4 additions & 4 deletions v3/src/models/document/document.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ describe("document model", () => {
expect(document.createdAt).toBe(10)
})

it("can set title", () => {
expect(document.title).toBeUndefined()
document.setTitle("FooTitle")
expect(document.title).toBe("FooTitle")
it("has a default title", () => {
// This is the default document name defined by the localization key:
// DG.Document.defaultDocumentName
expect(document.title).toBe("Untitled Document")
})

it("can set properties", () => {
Expand Down
15 changes: 11 additions & 4 deletions v3/src/models/document/document.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { DocumentContentModel, IDocumentContentSnapshotIn } from "./document-con
import { IDocumentProperties } from "./document-types"
import { ISharedModelDocumentManager } from "./shared-model-document-manager"
import { typedId } from "../../utilities/js-utils"
import { t } from "../../utilities/translation/translate"

export const DocumentModel = Tree.named("Document")
.props({
Expand All @@ -18,12 +19,17 @@ export const DocumentModel = Tree.named("Document")
version: types.maybe(types.string),
build: types.maybe(types.string),
createdAt: 0, // remote documents fill this in when content is loaded
title: types.maybe(types.string),
properties: types.map(types.string),
content: types.maybe(DocumentContentModel),
})
.volatile(self => ({
treeMonitor: undefined as TreeMonitor | undefined
treeMonitor: undefined as TreeMonitor | undefined,
// This is in volatile because the CFM is the source of truth for the title.
// Do not change this property directly, rename the file in the CFM instead.
// The default value should match the MENUBAR.UNTITLED_DOCUMENT in the CFM. When a new
// document is saved, the CFM will use its default title not this one. However we need a
// default value to show in the browser window.
title: t("DG.Document.defaultDocumentName")
}))
.views(self => ({
// This is needed for the tree monitor and manager
Expand Down Expand Up @@ -91,8 +97,9 @@ export const DocumentModel = Tree.named("Document")
self.createdAt = createdAt
},

setTitle(title: string) {
self.title = title
setTitleFromFilename(filename: string) {
// This will be called when the file is loaded, saved, or renamed by the CFM
self.title = filename.split(".")[0]
},

setProperty(key: string, value?: string) {
Expand Down
4 changes: 3 additions & 1 deletion v3/src/v2/import-v2-document.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ export function importV2Document(v2Document: CodapV2Document) {
})
}

v3Document.setTitle(v2Document.getDocumentTitle())
// Note: it is not necessary to set the name when importing a v2 document
// The CFM will get the name itself and then we'll set the name when
// the v3 document is initialized

// add shared models (data sets and case metadata)
v2Document.dataSets.forEach((data, key) => {
Expand Down
Loading