Saving a profile should also rename the currently selected one

RESOLVED FIXED in Firefox 43

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
P3
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: past, Assigned: anjaly, Mentored)

Tracking

unspecified
Firefox 43
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(4 attachments, 5 obsolete attachments)

When saving a profile, the name of the currently selected profile in the list is not updated to the new name. That means that creating multiple profiles and then saving them results in Profile 1, Profile 2, etc. without the descriptive names of their persisted versions. Moreover importing the stored profile will add a new entry to the list with the exact same profile.
Priority: -- → P3
Mentor: jsantell@mozilla.com
See Also: → bug 1161698
Whiteboard: [good first bug][lang=js]
(Assignee)

Comment 1

3 years ago
Hi 
I would like to take up on this fix. Could you give me pointer and assist me with the same?
Flags: needinfo?(jsantell)
Hey anjaly -- here's what I think the best way to do this is:

In the RecordingsView (browser/devtools/performance/views/recordings.js), listen to the event on the PerformanceController RECORDING_EXPORTED (bound where the other events for PerformanceController is) and call a function that takes a recording from the event, and updates the label element.

Currently, RECORDING_EXPORTED only emits the recording model, but we can also emit the file that the recording was just saved as. With that, we can get the file name. A handler could be something like (not tested, but the idea):

_onRecordingExported: function (_, recording, file) {
  // First we should check that the recording is not from the console, in which case,
  // it already has a label and we shouldn't update it
  if (recording.isConsole()) {
    return;
  }

  // Get the "recordingItem" for the recording, which is a representation of a list item in this widget
  let recordingItem = this.getItemForPredicate(e => e.attachment === recording);
  let titleNode = $(".recording-item-title", recordingItem.target);
  // Update the element with the file name, stripping out the extension
  titleNode.setAttribute("value", file.leafName.replace(/\..+$/, "");
}

Check out _onRecordingStateChange in the RecordingsView, that takes a recording model, gets the 'recordingItem', and updates the duration property, similar how the label property should be updated.

The test could probably be added to the general recording tests too. Let me know if you have any questions and thanks!
Assignee: nobody → anjalymehla51
Flags: needinfo?(jsantell)
(Assignee)

Comment 3

3 years ago
I am on it
(Assignee)

Comment 4

2 years ago
So where exactly this event in workflow should fit in? 
While saving , the file is exported with the default string name "profile" and emitted . The _onRecordingExported function should be called out before _onSaveButtonClick (before the save function emits the recording? And if thats the case then why not rename it beforehand?
Use needinfo? to be sure questions aren't lost.
Flags: needinfo?(jsantell)
(Assignee)

Comment 6

2 years ago
The file which RECORDING_EXPORTED event emits is null . The file is accessible after this :-
      this.emit(EVENTS.UI_EXPORT_RECORDING, recordingItem.attachment, fp.file);

However in here 
_onRecordingExported: function (_, recording, file){
	if (recording.isConsole()) {
      return;
  }
  let recordingItem = this.getItemForPredicate(e => e.attachment === recording);
  let titleNode = $(".recording-item-title", recordingItem.target);
  titleNode.setAttribute("value", file.leafName.replace(/\..+$/, ""));

  },
  
  "file" seems to be undefined . Is there something else to be added to explicitly ask the event to emit file as well?
(Assignee)

Comment 7

2 years ago
Ok nevermind . The RECORDING_EXPORTED event earlier emitted only the recording not file. I changed it in performance-controller.js so that file is emitted as well. The module is working fine. I will submit the patch in time :)
(Assignee)

Comment 8

2 years ago
Ok nevermind . The RECORDING_EXPORTED event earlier emitted only the recording not file. I changed it in performance-controller.js so that file is emitted as well. The module is working fine. I will submit the patch in time :)
(Assignee)

Comment 9

2 years ago
Created attachment 8651497 [details] [diff] [review]
saving_recording_name.patch
Attachment #8651497 - Flags: review?(jsantell)
(Assignee)

Comment 10

2 years ago
Created attachment 8651520 [details] [diff] [review]
display_name_after_import_export.patch

This patch contain code for displaying actual file name after an import.
Attachment #8651520 - Flags: review?(jsantell)
Attachment #8651497 - Flags: review?(jsantell)
Comment on attachment 8651520 [details] [diff] [review]
display_name_after_import_export.patch

Review of attachment 8651520 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, looks great! Some big things though, we should remove any of the importing handlers and things like that, this is only for exporting (another bug handles that), and some formatting. Can you combine these two into the same patch? It's a bit hard to review them separately.

Thanks!

::: browser/devtools/performance/performance-controller.js
@@ +337,5 @@
>     *        The file to stream the data into.
>     */
>    exportRecording: Task.async(function*(_, recording, file) {
>      yield recording.exportRecording(file);
> +    this.emit(EVENTS.RECORDING_EXPORTED, recording,file);

Space between the comma and "file": `recording, file`

@@ +370,5 @@
>     */
>    importRecording: Task.async(function*(_, file) {
>      let recording = yield gFront.importRecording(file);
>      this._addNewRecording(recording);
> +    this.emit(EVENTS.RECORDING_IMPORTED, recording,file);

There's another bug (which is done I believe) which does this for importing profiles and renaming, so the importing work here should be taken out

@@ +375,5 @@
>  
>      // Only emit in tests for legacy purposes for shorthand --
>      // other things in UI should handle the generic NEW_RECORDING
>      // event to handle lazy recordings.
> +    //if (DevToolsUtils.testing) {

This should not be commented out

::: browser/devtools/performance/views/recordings.js
@@ +18,5 @@
>      this._onNewRecording = this._onNewRecording.bind(this);
>      this._onSaveButtonClick = this._onSaveButtonClick.bind(this);
>      this._onRecordingsCleared = this._onRecordingsCleared.bind(this);
> +	this._onRecordingExported=this._onRecordingExported.bind(this);
> +	this._onRecordingImported=this._onRecordingImported.bind(this);

These should be lined up with the rest of the function bindings, and spaces around the " = "

@@ +25,5 @@
>      PerformanceController.on(EVENTS.RECORDING_STATE_CHANGE, this._onRecordingStateChange);
>      PerformanceController.on(EVENTS.NEW_RECORDING, this._onNewRecording);
>      PerformanceController.on(EVENTS.RECORDINGS_CLEARED, this._onRecordingsCleared);
> +    PerformanceController.on(EVENTS.RECORDING_EXPORTED, this._onRecordingExported);
> +    PerformanceController.on(EVENTS.RECORDING_IMPORTED, this._onRecordingImported);

Remove the importing listeners

@@ +202,5 @@
>        this.emit(EVENTS.UI_EXPORT_RECORDING, recordingItem.attachment, fp.file);
>      }});
>    },
>  
> +  _onRecordingExported: function (_, recording,file){

space before "file", and the formatting in this function should be two spaces of indentation

@@ +209,5 @@
> +	}
> +	let recordingItem = this.getItemForPredicate(e => e.attachment === recording);
> +	let titleNode = $(".recording-item-title", recordingItem.target);
> +	titleNode.setAttribute("value", file.leafName.replace(/\..+$/, ""));
> + 

extra white space

@@ +212,5 @@
> +	titleNode.setAttribute("value", file.leafName.replace(/\..+$/, ""));
> + 
> +   },
> +
> +  _onRecordingImported:function (_,recording,file){

Remove things related to importing
Attachment #8651520 - Flags: review?(jsantell)
Flags: needinfo?(jsantell)
(Assignee)

Comment 12

2 years ago
Created attachment 8651527 [details] [diff] [review]
final_export_display_name.patch

Final patch !
The patch earlier added were kind of draft thats why indentation lacked. Sorry for the trouble
Attachment #8651497 - Attachment is obsolete: true
Attachment #8651520 - Attachment is obsolete: true
Attachment #8651527 - Flags: review?(jsantell)
Comment on attachment 8651527 [details] [diff] [review]
final_export_display_name.patch

Review of attachment 8651527 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like only one part, can you combine all the changes into one patch?
Attachment #8651527 - Flags: review?(jsantell)
(Assignee)

Comment 14

2 years ago
Created attachment 8651531 [details] [diff] [review]
export_name_display.patch

This contain all the changes in performance-controller.js and recording.js 
(I hope this should work )
Attachment #8651527 - Attachment is obsolete: true
Attachment #8651531 - Flags: review?(jsantell)
Comment on attachment 8651531 [details] [diff] [review]
export_name_display.patch

Review of attachment 8651531 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like you're diffing the patch between what you have currently, and previous patches, so it cannot be applied -- notice the left side of the diff[0] still containing your changes. See if you can squash down all of your commits into one -- I think this might work[1]

[0] https://bugzilla.mozilla.org/attachment.cgi?id=8651531&action=diff
[1] http://stackoverflow.com/a/1725638
Attachment #8651531 - Flags: review?(jsantell)
(Assignee)

Comment 16

2 years ago
Created attachment 8651800 [details] [diff] [review]
export_name_display.patch
Attachment #8651800 - Flags: review?(jsantell)
Comment on attachment 8651800 [details] [diff] [review]
export_name_display.patch

Review of attachment 8651800 [details] [diff] [review]:
-----------------------------------------------------------------

Some small comments on formatting, but looks great! I can write tests or help you write tests, just let me know!

::: browser/devtools/performance/views/recordings.js
@@ +200,5 @@
>      }});
>    },
>  
> +  _onRecordingExported: function (_, recording, file){
> +    if(recording.isConsole()) {

nit: space after `if`

@@ +201,5 @@
>    },
>  
> +  _onRecordingExported: function (_, recording, file){
> +    if(recording.isConsole()) {
> +	  return;

nit: indentation, only two spaces

@@ +207,5 @@
> +    let recordingItem = this.getItemForPredicate(e => e.attachment === recording);
> +    let titleNode = $(".recording-item-title", recordingItem.target);
> +    titleNode.setAttribute("value", file.leafName.replace(/\..+$/, ""));
> +  },
> +	  

nit: remove extra whitespace here
Attachment #8651800 - Flags: review?(jsantell)
(Assignee)

Comment 18

2 years ago
Created attachment 8651915 [details] [diff] [review]
bug_963005.patch

All indentation take cared of! 

Yeah sure I can try if you could give me some pointers about tests.
Attachment #8651531 - Attachment is obsolete: true
Attachment #8651800 - Attachment is obsolete: true
Attachment #8651915 - Flags: review?(jsantell)
Great! For a test, you should be able to add a line in browser_perf_recordings-io-04.js, right after exporting, check to see if the label is updated, something like this should work:

is($(".recording-item-title").getAttribute("value"), EXPECTED_LABEL, "Exported recording is displayed with the file name as the label");
Attachment #8651915 - Flags: review?(jsantell) → review+
(Assignee)

Comment 20

2 years ago
For tests , using getAttribute gives the file name of exported file , but to verify it there has to be some definition for EXPECTED_LABEL, where is it coming from? I cannot figure out where to take the parameter from in browser_perf_recordings-io-05.js file once exporting is done.!
I think browser_perf_recordings-io-01.js is a better test to add this to.

The file is saved as 'tmpprofile.json'[0], so you'd expect the rendered display to be "tmpprofile", right?

[0] https://github.com/mozilla/gecko-dev/blob/2be1d3e641685fff871e1570fbb01bdfdf161ad1/browser/devtools/performance/test/browser_perf_recordings-io-01.js#L39
(Assignee)

Comment 22

2 years ago
Created attachment 8654414 [details]
screenshot_test.PNG

The test fails and this is the output
Looks like creating that temporary file, the `createUnique` function[0] will add on various endings so it doesn't have any save conflicts -- we can check here instead that "tmpprofile" is in the displayed name and that "json" is not. Also a comment why this is necessary would be useful

let displayedName = $(".recording-item-title").getAttribute("value");
ok(/^tmpprofile/.test(displayedName), "has expected display name after import");
ok(!/\.json$/.test(displayedName), "display name does not have .json in it");

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFile#createUnique%28%29
(Assignee)

Comment 24

2 years ago
Created attachment 8654532 [details] [diff] [review]
test.patch
Attachment #8654532 - Flags: review?(jsantell)
Attachment #8654532 - Flags: review?(jsantell) → review+
Created attachment 8654542 [details] [diff] [review]
963005-saving-rec.patch

Gonna push this to our try servers once they're up (hg.mozilla.org seems down at the moment for me?), but I think everything looks good :)
Attachment #8654542 - Flags: review+
Status: NEW → ASSIGNED
Ok test run looks great, now pushed and landed in fx-team -- should hit nightly in a day! Thanks so much for the contribution and patience, anjaly!
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4ceab182385b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.