1103346 - Rename not available in webIDE

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[santosh]

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.

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - Rename in webIDE

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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!

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 4

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - Rename in webIDE

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff084cf79944