Allow editing trace set labels via Opened Traces widget #451

thefinaljob · 2021-08-17T20:17:55Z

Contributes towards fixing #384 (changing the trace set label also changes the tab label for that trace).

Known Issues:

Default trace tab opened on launch of trace server does not have its tab renamed, issue is unrelated to code changes. @arfio is aware of this
Editing tab could use a bit more padding

Default Behaviour:

After consulting with @ebugden, both clicking outside the menu item and pressing enter will save the tab name and changes will be reflected in both the menu item and the tab name. @ssmagula
Max tab length of 50 characters
If 0 characters are inputed, tab name and menu item name are reverted to their state before they were being edited

Signed-off-by: Nikolai Peram [email protected]

mirsky-work · 2021-08-18T20:15:27Z

Thanks for contributing this useful feature!
Works nicely, but I do have a couple of comments:

An open tab's name doesn't change until it's closed and then reopened. After that it does update while typing. See video below.
Initially trace names are prefixed with "Trace:". On the first renaming it's preserved, but in following renaming it's discarded. I think it would be nice to preserve it, do differentiate trace tabs from source code tabs, for instance, or at least have a consistent behavior.
I think it would be visually cleaner to align the pen icon to the right, so it'll be at the same "column" for all traces.
In addition to the pen icon, I think we should also add a "Rename Trace Tab" entry to the Opened Traces context menu.
The text input box, when renaming, could perhaps be thinner by a few pixels, so it won't shadow part of the first line of the experiment's contents.

pr-451.mp4

ssmagula · 2021-08-18T21:32:34Z

Looking good! Below are some notes after screensharing with @thefinaljob :

This feature is valuable because experienced trace visualization users say things like: "It's important I know which is the 'before trace' and which is the 'after [I make a change] trace'..." and: "When I share screenshots with others I hate scrawling all the details on the screenshots like 'Before...' and 'After I did xyz' because it's so hard to write letters with a mouse."

Re: saving on enter or loss of focus: I've seen many users make edits in place then move on—never hitting enter—so saving when focus is lost is a good way to prevent errors. Nice nuance and thumbs up! The ability to revert is also good thinking, and I only wish we had a good visible affordance for "revert." Maybe the reload/revert icon?

I'd title this commit "Enable user to to edit _trace set labels from within the Trace Viewer list of traces"

Here's an edit-in-place sketch, showing various states in the process:

Everyone reading this is invited to join a whiteboard I set up in Figjam. There you can zoom in on this sketch, comment, vote, augment, correct, educate. It's free, and you can join via this link: https://www.figma.com/file/5nxXdb198DkTwT24ZyqLkv/TraceViz-Figjam-Edit-in-place?node-id=0%3A1

Key points, in priority order:
2. Indicate focus state with standard cyan border around text entry box
3. Pencil icon and darker bg appear on hover of label (rationale: imagine a list of many trace sets: do we need to clutter the screen with many pencils?)
3. Text entry box should have enough padding so it doesn't collide/overlap
4. Add the "Edit trace label" item to the contextual menu

ebugden · 2021-08-18T22:25:55Z

Requested changes:

Match styling to what is shown in ssmagula's mockup above
Change commit and PR title to something like "Allow editing trace set labels via Opened Traces widget" (as suggested by ssmagula)

Also look over the commit message guidelines in the new How to contribute code readme section. Some specific issues:

Write commit and PR title in imperative (ex. "Add..." not "Added...")
Link fixed issue should to the PR so the issue automatically closes when the PR is merged. Issue is linked automatically when you include "Fixes Allow renaming trace viewer tabs #384" in the commit message (preferred) or you can do this manually via Github UI.

Honestly changing the labels is so satisfying! Thanks for the PR 🍰

ssmagula: I'd title this commit "Enable user to to edit _trace set labels from within the Trace Viewer list of traces"

I second this! Even though #384 was the motivation for these changes, the main action implemented in this PR is changing the trace set label (the tab changing names is a logical consequence of relabeling the trace set not the main action).

mirsky-work: I think we should also add a "Rename Trace Tab" entry to the Opened Traces context menu.

I think a context menu entry isn't critical at the moment since the action is already possible. However, if an entry is added it should be "Change trace label" or "Relabel trace" (for the same reason as above).

Ideally, renaming tabs would eventually also be possible through the tab's context menu, but @ssmagula made the good point that this should be a Theia contribution not a trace-viewer-extension only thing.

arfio

Thank you for the contribution! I think it is a much needed improvement in terms of usability because most traces do not have meaningful names.

An open tab's name doesn't change until it's closed and then reopened. After that it does update while typing. See video below.

This issue is related to #364 and #450. I don't think this pull request makes the issue worse.
Also, if you reopen the trace directly from the files it will focus on the tab that did not change and stay on it when you select the menu item.
I agree with all the changes requested before, and added some comments that are mainly coding style changes.

arfio · 2021-08-19T14:03:20Z

packages/base/src/signals/signal-manager.ts

@@ -43,6 +44,9 @@ export const Signals = {
 };

 export class SignalManager extends EventEmitter implements SignalManager {
+    fireTabChangedSignal(tabName: string, expirementUUID: string): void {
+        this.emit(Signals.TABNAME_CHANGED, {tabName, expirementUUID});


Should be { tabName, experimentUUID }); (typo + space separating the curly braces) and the typo should be corrected in the argument

arfio · 2021-08-19T14:04:51Z

packages/react-components/src/trace-explorer/menu-item-trace.tsx

+}
+interface MenuItemState {
+    editingTab: boolean;
+    oldexpirementName: string;


Incorrect lower camel case + typo oldExperimentName

arfio · 2021-08-19T14:07:05Z

packages/react-components/src/trace-explorer/menu-item-trace.tsx

+interface MenuItemState {
+    editingTab: boolean;
+    oldexpirementName: string;
+    expirementNameState: string;


All expirement should be changed in experiment

arfio · 2021-08-19T14:20:42Z

packages/react-components/src/trace-explorer/trace-explorer-opened-traces-widget.tsx

+        exp.name = tabEditingOpen;
+        const experimentOpenNew = this.state.openedExperiments;
+        experimentOpenNew[index] = exp;
+        this.setState({openedExperiments: experimentOpenNew});


Should have spaces { openedExperiments: experimentOpenNew }

arfio · 2021-08-19T14:29:04Z

packages/react-components/src/trace-explorer/trace-explorer-opened-traces-widget.tsx

        const selectedIndex = remoteExperiments.findIndex(experiment => this._selectedExperiment &&
            experiment.UUID === this._selectedExperiment.UUID);
        this.setState({ openedExperiments: remoteExperiments, selectedExperimentIndex: selectedIndex !== -1 ? selectedIndex : 0 });
        signalManager().fireOpenedTracesChangedSignal(new OpenedTracesUpdatedSignalPayload(remoteExperiments ? remoteExperiments.length : 0));
    }

+    protected handleTabNameUpdate(tabEditingOpen: string, index: number): void {


All functions or events related to experiment name change should not refer to the tab.
submitNewTab -> submitNewExperimentName
handleTabNameUpdate -> handleExperimentNameUpdate
fireTabChangedSignal -> fireExperimentChangedSignal
TABNAME_CHANGED -> EXPERIMENT_CHANGED
onTabNameChange -> onExperimentNameChange
doHandleTabeNameChange -> doHandleExperimentNameChange
tabEditingOpen -> newExperimentName

bhufmann

Thank you very much for the contribution. Will be a useful feature for the user.

I have some comments about the functionality.

Firstly, right now the change of the name is done purely on the front-end side. The back-end (trace-server) doesn't know about the changes. When disconnecting and re-connecting the front-end the old name is back.
To fix that, the name has to be propagated to the trace server so that the change is persisted for re-connection or connection using a different client. A fullstack update (front-end, TSP, and back-end) is needed to support that.

Secondly, we need to be clear that the renaming of experiment name doesn't change any file resources (i.e. the trace files)., and hence using this to keep a "before"-trace to be compared with an "after"-trace for trace comparison purposes won't work. This compare use case has to be addressed differently so that on the trace server each trace file of the experiment is kept. I think a deep copy of the experiment on the server would be needed instead of renaming the existing experiment.

ebugden · 2021-08-25T15:09:49Z

bhufmann: Firstly, right now the change of the name is done purely on the front-end side. The back-end (trace-server) doesn't know about the changes. When disconnecting and re-connecting the front-end the old name is back.
To fix that, the name has to be propagated to the trace server so that the change is persisted for re-connection or connection using a different client. A fullstack update (front-end, TSP, and back-end) is needed to support that.

True. I agree that the trace set label should be propagated to the server. Do these additional changes need to be prepared before this PR is merged? I'm divided, since I agree that merging just the front end changes is incomplete, but I think it still improves most use cases (re-connection or connection using a different client are less common).

bhufmann: Secondly, we need to be clear that the renaming of experiment name doesn't change any file resources (i.e. the trace files)., ...

I agree that it's not clear that changing the trace set label doesn't affect the filesystem and that this is confusing. But I don't think resolving this is within the scope of this PR. At the moment, trace set generation is all implicit and behind the scenes so the first step would be to to introduce the concept of "trace sets" that have no connection to the filesystem. If necessary, the changes proposed below could be made before merging this PR.

In the short term (new issues?):

Better default trace set name: The difference between the filesystem and the trace set would be more clear if the trace set label wasn't exactly the folder name (when automatically generating a trace set when a folder containing CTF traces), but this can be addressed in a different issue/PR. For example: "Trace Set 0 - Generated from trace-folder-name" instead of just "trace-folder-name".
Explicitly show file "Refresh" button next to trace set (or toast notification) when traces files are overwritten. This clearly communicates that a copy of the trace files was not made.

... and hence using this to keep a "before"-trace to be compared with an "after"-trace for trace comparison purposes won't work. This compare use case has to be addressed differently so that on the trace server each trace file of the experiment is kept. I think a deep copy of the experiment on the server would be needed instead of renaming the existing experiment.

Good point that these changes don't make it easier to manage trace files when doing a before/after comparison (as in, users would still have to manually move/rename the "before" traces before generating the "after" traces). I don't think comparison is within the scope of this issue and I don't think renaming the trace set is the only indicator of "I want to keep this version of these trace files", but more thought definitely needs to go into comparison and whether/when to make copies of the actual trace files (especially since it's likely that a portion of users will trace using scripts that output trace files that always have the same name).

Thank you for this relevant feedback!

thefinaljob · 2021-08-27T06:20:52Z

Hello!

Just wanted to update everyone following this on the status of the updated PR. Here's where we stand now.

Styling has almost fully changed to fit @ssmagula's specifications (including the trace button being aligned on the left). I need to figure out the css colour code of the textbox in the mockup and then we should be ok.
Clicks were not working when pressing another tab's edit button after typing some text into an open submission box. This bug was discovered after attempting to make the edit button hide after the submission of a new tab name. This has been fixed, however the color of a selected tab does not change from grey to blue when clicking on it, until there's a resize. I will discuss with @arfio in the morning
"Trace: " stays in the tab name at all times
Tab editing box has been made smaller by a handful of pixels
Variable names still not have been renamed. I'll do this once I iron out the issue mentioned above with @arfio .
PR description will be changed upon pushing the code that resolves the issues mentioned above

Extending the response mentioned by @ebugden , I do believe that @bhufmann's feedback could be implemented through opening new issues for tracecompass.

bhufmann · 2021-08-30T13:32:15Z

bhufmann: Firstly, right now the change of the name is done purely on the front-end side. The back-end (trace-server) doesn't know about the changes. When disconnecting and re-connecting the front-end the old name is back.
To fix that, the name has to be propagated to the trace server so that the change is persisted for re-connection or connection using a different client. A fullstack update (front-end, TSP, and back-end) is needed to support that.

True. I agree that the trace set label should be propagated to the server. Do these additional changes need to be prepared before this PR is merged? I'm divided, since I agree that merging just the front end changes is incomplete, but I think it still improves most use cases (re-connection or connection using a different client are less common).

I think that persistence is important. Closing the trace viewer while keeping a trace on the server is quite common. So, when opening the trace viewer again I personally would expect, that the renamed trace name is shown.

I think the back-end support can be implemented outside of the scope of this PR. However, it needs to be done shortly after.

bhufmann: Secondly, we need to be clear that the renaming of experiment name doesn't change any file resources (i.e. the trace files)., ...

I agree that it's not clear that changing the trace set label doesn't affect the filesystem and that this is confusing. But I don't think resolving this is within the scope of this PR. At the moment, trace set generation is all implicit and behind the scenes so the first step would be to to introduce the concept of "trace sets" that have no connection to the filesystem. If necessary, the changes proposed below could be made before merging this PR.

In the short term (new issues?):
* **Better default trace set name:** The difference between the filesystem and the trace set would be more clear if the trace set label wasn't exactly the folder name (when automatically generating a trace set when a folder containing CTF traces), but this can be addressed in a different issue/PR. For example: "Trace Set 0 - _Generated from trace-folder-name_" instead of just "trace-folder-name".

This could be the description and shown as tooltip. Using "Trace Set 0", "Trace Set 1" is not necessary better than using the directory name. The directory name right now works well, for example, when opening a trace set from the LTTng session root directory and the trace set name becomes the session name. Another use case is, when the root directory is a test case name and opening the trace set name become the unit test name. Changing the name to "Trace Set 0" as default, would break these use cases.

I'm wondering if a dialog box should open with text box after selecting "Open with Trace Viewer". It opens with the default name. If the use wants to keep it, then he only needs to press ok. Otherwise the use can type a new name and then press enter.

* **Explicitly show file "Refresh"** button next to trace set (or toast notification) when traces files are overwritten. This clearly communicates that a copy of the trace files was not made.

Some indication is needed and a refresh needs to be done. Right now with the local trace server implementation, the trace server only has a symbolic link to the original trace file in the file system. If the original trace file is overridden, then the trace index, state systems etc (all trace analysis files) are out-dated as well and need to be re-built.

... and hence using this to keep a "before"-trace to be compared with an "after"-trace for trace comparison purposes won't work. This compare use case has to be addressed differently so that on the trace server each trace file of the experiment is kept. I think a deep copy of the experiment on the server would be needed instead of renaming the existing experiment.

Good point that these changes don't make it easier to manage trace files when doing a before/after comparison (as in, users would still have to manually move/rename the "before" traces before generating the "after" traces). I don't think comparison is within the scope of this issue and I don't think renaming the trace set is the only indicator of "I want to keep this version of these trace files", but more thought definitely needs to go into comparison and whether/when to make copies of the actual trace files (especially since it's likely that a portion of users will trace using scripts that output trace files that always have the same name).

Yes, Trace comparison is not scope of this PR. But it's somehow related when it comes to managing trace files and I think it was important to mention it here.

Thank you for this relevant feedback!

No problem!

MatthewKhouzam

Reminder, commit messages need to be in the present tense.

thefinaljob · 2021-09-04T01:19:11Z

@arfio has been working on getting the re-render issue for the menu completed. If we can't get it fixed shortly, we might have to live with this and submit another issue stating that the menu needs to be re-tooled so that it works with react components as menu items.

ebugden · 2021-09-08T14:37:13Z

Exceptionally, I'm going to make edits to the title and description of this PR to correct some style issues.

ebugden · 2021-09-08T21:39:34Z

Storing label names in the trace server

bhufmann: I think the back-end support can be implemented outside of the scope of this PR. However, it needs to be done shortly after.

I created an issue #474 so the required additional changes can be defined there. However, it's unlikely that they would be made soon. At the moment @arfio is working on finishing this PR when they have free time, but they don't have much free time these days.

Clarify that re-labeling the trace doesn't change the trace files

bhufmann: Secondly, we need to be clear that the renaming of experiment name doesn't change any file resources (i.e. the trace files).,

I agree it's best clarify this before merging this PR. I moved this discussion into its own issue #475

…clipse-cdt-cloud#384. Made each menu item into its own react component. Signed-off-by: Nikolai Peram <[email protected]>

thefinaljob · 2021-09-18T00:44:31Z

Given that reworking the entire menu is taking a while I suggest we live with the current rendering issue present (currently the user needs to resize the menu in order to reflect the most recently selected trace). Other than that, all of the requested changes have been implemented

Commit has been modified per the contribution guidelines
Typos have been fixed as per @arfio's feedback
You can easily click between menu items and their changes are reflected
"Trace: " now always appears
@ssmagula's and @mirsky-work's feedback regarding the styling has been implemented

I also made a modification to the selected item colour. Having it as blue resulted in a very intense contrast with the black bar when it's selected.

bhufmann

Storing label names in the trace server

bhufmann: I think the back-end support can be implemented outside of the scope of this PR. However, it needs to be done shortly after.

I created an issue #474 so the required additional changes can be defined there. However, it's unlikely that they would be made soon. At the moment @arfio is working on finishing this PR when they have free time, but they don't have much free time these days.

As I said above the back-end should be available soon after this PR is ready. Merging without having a proper back-end support I'd like to avoid.

BTW, this PR needs to be rebased to master. As well as it' seems to have some styling issue in light theme:

bhufmann · 2021-09-30T11:25:38Z

packages/react-components/src/trace-explorer/menu-item-trace.tsx

@@ -0,0 +1,148 @@
+import * as React from 'react';
+import { Trace } from 'tsp-typescript-client';
+import { signalManager, Signals } from '@trace-viewer/base/lib/signals/signal-manager';


the package was renamed: use traceviewer-base instead of @trace-viewer/base

ebugden · 2021-09-30T18:51:51Z

@thefinaljob Thank you for the update!

I agree with @bhufmann on not merging as it is now. I'm uncomfortable with merging a feature feels unfinished when the timeline of when it will be completed is unclear since currently no one is actively working on finalizing this feature.

At this stage in the project we're working on making the tool feel more stable and polished and adding this feature as it is now would be a step backwards for polish. Allowing users to label traces is convenient, but doesn't add enough user value to justify making the tool feel less polished.

thefinaljob force-pushed the editingtabs branch 3 times, most recently from ab23a85 to 33d7ea8 Compare August 18, 2021 01:32

arfio self-requested a review August 18, 2021 01:39

arfio suggested changes Aug 19, 2021

View reviewed changes

bhufmann requested changes Aug 24, 2021

View reviewed changes

ebugden linked an issue Aug 25, 2021 that may be closed by this pull request

Allow renaming trace viewer tabs #384

Open

MatthewKhouzam reviewed Aug 30, 2021

View reviewed changes

ebugden changed the title ~~Added ability for users to change tab names from menu items in trace explorer widget~~ Allow editing trace set labels via Opened Traces widget Sep 8, 2021

This was referenced Sep 8, 2021

Make re-labelled trace names persistent #474

Open

Clarify that re-labeling traces doesn't change trace file names #475

Open

thefinaljob force-pushed the editingtabs branch 2 times, most recently from 685bdec to 01eb485 Compare September 18, 2021 00:22

thefinaljob closed this Sep 18, 2021

thefinaljob force-pushed the editingtabs branch from 01eb485 to 820176f Compare September 18, 2021 00:33

Allow editing trace set labels via Opened Traces widget. Fixes issue e…

14af176

…clipse-cdt-cloud#384. Made each menu item into its own react component. Signed-off-by: Nikolai Peram <[email protected]>

thefinaljob reopened this Sep 18, 2021

bhufmann requested changes Sep 30, 2021

View reviewed changes

ebugden mentioned this pull request Nov 12, 2021

Introduce the concept of a "trace set" to the UI (formerly "experiment") #561

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow editing trace set labels via Opened Traces widget #451

Allow editing trace set labels via Opened Traces widget #451

thefinaljob commented Aug 17, 2021 •

edited by ebugden

Loading

mirsky-work commented Aug 18, 2021

ssmagula commented Aug 18, 2021 •

edited

Loading

ebugden commented Aug 18, 2021 •

edited

Loading

arfio left a comment •

edited

Loading

arfio Aug 19, 2021

arfio Aug 19, 2021

arfio Aug 19, 2021

arfio Aug 19, 2021

arfio Aug 19, 2021

bhufmann left a comment

ebugden commented Aug 25, 2021 •

edited

Loading

thefinaljob commented Aug 27, 2021

bhufmann commented Aug 30, 2021 •

edited

Loading

MatthewKhouzam left a comment

thefinaljob commented Sep 4, 2021

ebugden commented Sep 8, 2021 •

edited

Loading

ebugden commented Sep 8, 2021

thefinaljob commented Sep 18, 2021 •

edited

Loading

bhufmann left a comment

bhufmann Sep 30, 2021

ebugden commented Sep 30, 2021

Allow editing trace set labels via Opened Traces widget #451

Are you sure you want to change the base?

Allow editing trace set labels via Opened Traces widget #451

Conversation

thefinaljob commented Aug 17, 2021 • edited by ebugden Loading

mirsky-work commented Aug 18, 2021

ssmagula commented Aug 18, 2021 • edited Loading

ebugden commented Aug 18, 2021 • edited Loading

arfio left a comment • edited Loading

Choose a reason for hiding this comment

arfio Aug 19, 2021

Choose a reason for hiding this comment

arfio Aug 19, 2021

Choose a reason for hiding this comment

arfio Aug 19, 2021

Choose a reason for hiding this comment

arfio Aug 19, 2021

Choose a reason for hiding this comment

arfio Aug 19, 2021

Choose a reason for hiding this comment

bhufmann left a comment

Choose a reason for hiding this comment

ebugden commented Aug 25, 2021 • edited Loading

thefinaljob commented Aug 27, 2021

bhufmann commented Aug 30, 2021 • edited Loading

MatthewKhouzam left a comment

Choose a reason for hiding this comment

thefinaljob commented Sep 4, 2021

ebugden commented Sep 8, 2021 • edited Loading

ebugden commented Sep 8, 2021

Storing label names in the trace server

Clarify that re-labeling the trace doesn't change the trace files

thefinaljob commented Sep 18, 2021 • edited Loading

bhufmann left a comment

Choose a reason for hiding this comment

Storing label names in the trace server

bhufmann Sep 30, 2021

Choose a reason for hiding this comment

ebugden commented Sep 30, 2021

thefinaljob commented Aug 17, 2021 •

edited by ebugden

Loading

ssmagula commented Aug 18, 2021 •

edited

Loading

ebugden commented Aug 18, 2021 •

edited

Loading

arfio left a comment •

edited

Loading

ebugden commented Aug 25, 2021 •

edited

Loading

bhufmann commented Aug 30, 2021 •

edited

Loading

ebugden commented Sep 8, 2021 •

edited

Loading

thefinaljob commented Sep 18, 2021 •

edited

Loading