Closed
Bug 1103346
Opened 8 years ago
Closed 8 years ago
Rename not available in webIDE
Categories
(DevTools Graveyard :: WebIDE, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 37
People
(Reporter: viswanathamsantosh, Assigned: abdelrahman, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 1 obsolete file)
11.85 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0 Build ID: 20141013200257 Steps to reproduce: I tried to rename a File from the Tree Structure on Left side. Actual results: There is no option of Renaming the file name on it. Expected results: Option to rename a file would be very helpful.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Mentor: jryans
Whiteboard: [lang=js]
Whiteboard: [lang=js] → [good first bug][lang=js]
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8539814 -
Flags: review?(jryans)
This is quite cool, thanks for working on this! :D I won't be able to review this until I get back from holidays on Dec. 29. Sorry for the delay!
Assignee: nobody → a.ahmed1026
Status: NEW → ASSIGNED
Comment on attachment 8539814 [details] [diff] [review] rev 1 - Rename in webIDE Review of attachment 8539814 [details] [diff] [review]: ----------------------------------------------------------------- Overall, looks good! I've made a few comments about what to address. When you attach an updated patch, be sure to mark the previous one as obsolete. Add r=jryans to the end of the commit message. Please add a test for this feature. See browser/devtools/projecteditor/test/browser_projecteditor_delete_file.js as an example. ::: browser/devtools/projecteditor/lib/plugins/rename/rename.js @@ +22,5 @@ > + }); > + }, > + > + onContextMenuOpen: function(resource) { > + // Hide rename from menu of directories. Why is it not allowed for directories? Seems like that would be useful to have. Is it harder to support for some reason? @@ +39,5 @@ > + > + if (resource.isDir) { > + return; > + } > + else Move the else branch onto one line. See the style guide[1]. [1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style @@ +45,5 @@ > + parent = resource.parent; > + } > + > + tree.promptEdit(oldName, resource).then(name => { > + Nit: Remove this blank line @@ +49,5 @@ > + > + if (name === oldName) { > + return resource; > + } > + if (!this.isAvailable(parent, name)) { Checking if |hasChild| is true should be sufficient. @@ +54,5 @@ > + let matches = name.match(/([^\d.]*)(\d*)([^.]*)(.*)/); > + let template = matches[1] + "{1}" + matches[3] + matches[4]; > + name = this.suggestName(parent, template, parseInt(matches[2]) || 2); > + } > + Nit: Remove this blank line @@ +74,5 @@ > + > + return name; > + }, > + > + hasChild: function(resource, name) { Move this function onto the |Resource| object. @@ +83,5 @@ > + } > + return false; > + }, > + > + isAvailable: function(resource, name) { This function is confusing, since it mixes "check if a name exists already" with "increment a counter to look for a new name". Also, the counter is thrown away and the work repeated in |suggestName|. Remove this function, and change it's call site as mentioned above. ::: browser/devtools/projecteditor/lib/stores/resource.js @@ +303,5 @@ > }); > }, > > /** > + * Rename the file from the filesystem Nit: Align * with first * of line above ::: browser/devtools/projecteditor/lib/tree.js @@ +276,5 @@ > return deferred.promise; > }, > > /** > + * Prompt the user to rename file in the tree. Nit: Align * with first * of line above
Attachment #8539814 -
Flags: review?(jryans) → feedback+
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8539814 -
Attachment is obsolete: true
Attachment #8543403 -
Flags: review?(jryans)
Comment on attachment 8543403 [details] [diff] [review] rev 2 - Rename in webIDE Review of attachment 8543403 [details] [diff] [review]: ----------------------------------------------------------------- Great, this looks good to me! I've pushed this to our Try server (testing environment). Once it looks green, we can mark this for check in. Please look around for more WebIDE bugs that interest you! :) Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e58e28fd3d22
Attachment #8543403 -
Flags: review?(jryans) → review+
Keywords: checkin-needed
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ff084cf79944 Thanks for the patch, Abdelrhman! One thing of note - I tweaked your commit message to better describe what the patch was doing. Take a look at the link below for some more general guidelines for how to craft a checkin commit message. Thanks again! https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Comment 7•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ff084cf79944
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 37
Blocks: 1075432
Updated•5 years ago
|
Product: Firefox → DevTools
Updated•3 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•