Closed Bug 1103346 Opened 5 years ago Closed 5 years ago

Rename not available in webIDE

Categories

(DevTools :: WebIDE, defect)

35 Branch
x86_64
Linux
defect
Not set

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)

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.
Component: Untriaged → Developer Tools: WebIDE
Status: UNCONFIRMED → NEW
Ever confirmed: true
Mentor: jryans
Whiteboard: [lang=js]
Whiteboard: [lang=js] → [good first bug][lang=js]
Attached patch rev 1 - Rename in webIDE (obsolete) — Splinter Review
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+
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+
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]
https://hg.mozilla.org/mozilla-central/rev/ff084cf79944
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 37
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.