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

[Notebook] Browse Bar holding onto stale model, reverts changes #7944

Merged
merged 8 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from 7 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
16 changes: 16 additions & 0 deletions e2e/appActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,21 @@ async function linkParameterToObject(page, parameterName, objectName) {
await page.getByLabel('Save').click();
}

/**
* Rename the currently viewed `domainObject` from the browse bar.
*
* @param {import('@playwright/test').Page} page
* @param {string} newName
*/
async function renameCurrentObjectFromBrowseBar(page, newName) {
const nameInput = page.getByLabel('Browse bar object name');
await nameInput.click();
await nameInput.fill('');
await nameInput.fill(newName);
// Click the browse bar container to save changes
await page.getByLabel('Browse bar', { exact: true }).click();
}

export {
createDomainObjectWithDefaults,
createExampleTelemetryObject,
Expand All @@ -693,6 +708,7 @@ export {
linkParameterToObject,
navigateToObjectWithFixedTimeBounds,
navigateToObjectWithRealTime,
renameCurrentObjectFromBrowseBar,
setEndOffset,
setFixedIndependentTimeConductorBounds,
setFixedTimeMode,
Expand Down
62 changes: 61 additions & 1 deletion e2e/tests/functional/plugins/notebook/notebook.e2e.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ This test suite is dedicated to tests which verify the basic operations surround

import { fileURLToPath } from 'url';

import { createDomainObjectWithDefaults } from '../../../../appActions.js';
import {
createDomainObjectWithDefaults,
renameCurrentObjectFromBrowseBar
} from '../../../../appActions.js';
import { copy, paste, selectAll } from '../../../../helper/hotkeys/hotkeys.js';
import * as nbUtils from '../../../../helper/notebookUtils.js';
import { expect, streamToString, test } from '../../../../pluginFixtures.js';
Expand Down Expand Up @@ -596,4 +599,61 @@ test.describe('Notebook entry tests', () => {
await expect(await page.locator(`text="${TEST_TEXT.repeat(1)}"`).count()).toEqual(1);
await expect(await page.locator(`text="${TEST_TEXT.repeat(2)}"`).count()).toEqual(0);
});

test('When changing the name of a notebook in the browse bar, new notebook changes are not lost', async ({
page
}) => {
const TEST_TEXT = 'Do not lose me!';
const FIRST_NEW_NAME = 'New Name';
const SECOND_NEW_NAME = 'Second New Name';

await page.goto(notebookObject.url);

await page.getByLabel('Expand My Items folder').click();

await renameCurrentObjectFromBrowseBar(page, FIRST_NEW_NAME);

// verify the name change in tree and browse bar
await verifyNameChange(page, FIRST_NEW_NAME);

// enter one entry
await enterAndCommitTextEntry(page, TEST_TEXT);

// verify the entry is present
await expect(await page.locator(`text="${TEST_TEXT}"`).count()).toEqual(1);

// change the name
await renameCurrentObjectFromBrowseBar(page, SECOND_NEW_NAME);

// verify the name change in tree and browse bar
await verifyNameChange(page, SECOND_NEW_NAME);

// verify the entry is still present
await expect(await page.locator(`text="${TEST_TEXT}"`).count()).toEqual(1);
});
});

/**
* Enter text into the last notebook entry and commit it.
*
* @param {import('@playwright/test').Page} page
* @param {string} text
*/
async function enterAndCommitTextEntry(page, text) {
await nbUtils.addNotebookEntry(page);
await nbUtils.enterTextInLastEntry(page, text);
await nbUtils.commitEntry(page);
}

/**
* Verify the name change in the tree and browse bar.
*
* @param {import('@playwright/test').Page} page
* @param {string} newName
*/
async function verifyNameChange(page, newName) {
await expect(
page.getByRole('treeitem').locator('.is-navigated-object .c-tree__item__name')
).toHaveText(newName);
await expect(page.getByLabel('Browse bar object name')).toHaveText(newName);
}
3 changes: 2 additions & 1 deletion src/ui/layout/BrowseBar.vue
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
at runtime from the About dialog for additional information.
-->
<template>
<div class="l-browse-bar">
<div class="l-browse-bar" aria-label="Browse bar">
<div class="l-browse-bar__start">
<button
v-if="hasParent"
Expand All @@ -35,6 +35,7 @@
</div>
<span
ref="objectName"
aria-label="Browse bar object name"
class="l-browse-bar__object-name c-object-label__name"
:class="{ 'c-input-inline': isPersistable }"
:contenteditable="isNameEditable"
Expand Down
16 changes: 7 additions & 9 deletions src/ui/router/Browse.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,11 @@ class Browse {
this.#openmct.layout.$refs.browseBar.viewKey = viewProvider.key;
}

#updateDocumentTitleOnNameMutation(newName) {
if (typeof newName === 'string' && newName !== document.title) {
document.title = newName;
this.#openmct.layout.$refs.browseBar.domainObject = {
...this.#openmct.layout.$refs.browseBar.domainObject,
name: newName
};
#handelBrowseObjectUpdate(newObject) {
jvigliotta marked this conversation as resolved.
Show resolved Hide resolved
this.#openmct.layout.$refs.browseBar.domainObject = newObject;

if (typeof newObject.name === 'string' && newObject.name !== document.title) {
document.title = newObject.name;
}
}

Expand Down Expand Up @@ -120,8 +118,8 @@ class Browse {
document.title = this.#browseObject.name; //change document title to current object in main view
this.#unobserve = this.#openmct.objects.observe(
this.#browseObject,
'name',
this.#updateDocumentTitleOnNameMutation.bind(this)
'*',
this.#handelBrowseObjectUpdate.bind(this)
);
const currentProvider = this.#openmct.objectViews.getByProviderKey(currentViewKey);
if (currentProvider && currentProvider.canView(this.#browseObject, this.#openmct.router.path)) {
Expand Down
Loading