Closed Bug 963005 Opened 10 years ago Closed 9 years ago

Saving a profile should also rename the currently selected one

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect, P3)

defect

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: past, Assigned: anjalymehla51, Mentored)

References

Details

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

Attachments

(4 files, 5 obsolete files)

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
See Also: → 1161698
Whiteboard: [good first bug][lang=js]
Hi 
I would like to take up on this fix. Could you give me pointer and assist me with the same?
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)
I am on it
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)
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?
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 :)
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 :)
Attached patch saving_recording_name.patch (obsolete) — Splinter Review
Attachment #8651497 - Flags: review?(jsantell)
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)
Attached patch final_export_display_name.patch (obsolete) — Splinter Review
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)
Attached patch export_name_display.patch (obsolete) — Splinter Review
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)
Attached patch export_name_display.patch (obsolete) — Splinter Review
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)
Attached patch bug_963005.patchSplinter Review
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+
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
Attached image 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
Attached patch test.patchSplinter Review
Attachment #8654532 - Flags: review?(jsantell)
Attachment #8654532 - Flags: review?(jsantell) → review+
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
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 43
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.