Need support for custom context menu menus

[GoogleCodeExporter]

Hey there,

My name is Adam Bender and I am working with Flint OBrien on some enhancements 
to svg-edit. I was told that the best thing to do was to file issue with patch 
file attached so that is what I am going to do. 

We need the ability to add a custom 'properties' option to the context menu 
that appear when right clicking on a selected svg object. Attached is a patch 
which adds addContextMenuItem to the Editor object.

As this is my first patch and we are using git, please let me know if this 
patch is in a useful form or not.

Thanks,
Adam

Original issue reported on code.google.com by adamben...@gmail.com on 23 Feb 2012 at 6:04

Attachments:

• 0001-added-support-for-custom-context-menus.patch

[GoogleCodeExporter]

1) Style-wise, can you correct your patch to have

  if (code like this) {

2) Did you make a change in the HTML to the "Bring to Front" and "Send to Back" 
lines other than whitespace?  Please revert if not.

3) var valid = !(typeof menuItem ==='undefined' || (menuItem == null)  ...

can't this be replaced by

  var valid = menuItem && ...

4) We haven't used console.error() in the code that I'm aware of.  Is it 
standard and supported everywhere?

5) In a future revision, it would be really great if the context menu stuff was 
pulled out into a separate module (JS file) so that it's all in one place and 
can be unit tested easier

6) Do you have a simple demo that shows this working?

Original comment by codedr...@gmail.com on 23 Feb 2012 at 8:21

[GoogleCodeExporter]

In order:

1) Style-wise, can you correct your patch to have...

I assume you are talking about white space here, and yes I can definitely get 
proper formatting setup. Do you have a formatting guide or some other document 
that describes your conventions?

2)Did you make a change in the HTML to the "Bring to Front" and "Send to Back" 
lines other than whitespace?  Please revert if not...

I can revert, was definitely just white space

3)var valid = !(typeof menuItem ==='undefined' || (menuItem == null)  ...

I think you are correct, just getting overzealous with my insurance policy.

4) We haven't used console.error() in the code that I'm aware of.  Is it 
standard and supported everywhere?

I use error a lot in other JS apps, and especially in GWT apps so that it is 
clear what kind of issues are being encountered. My background is much more 
Java so I am used to being explicit about logging. As for standard support I 
can vouch for webkit and firefox under firebug, beyond that Im not 100% sure. I 
can find out for you if you'd like.

5)  In a future revision, it would be really great if the context menu stuff 
was pulled out into a separate module (JS file) so that ...

Absolutely, we are doing most of our work in separate extensions, and I am a 
big believer in TDD. The reason I put this code here is that it seemed like 
most of the initialization stuff and declaring of app wide resources was going 
on in svg-editor.js. In this case though we aren't dependent on anything in 
that scope so I will definitely move it out into a separate file. 

6) Do you have a simple demo that shows this working?

I do, but I don't want to post the address in public so I will email you with a 
url. 

Finally, 
This was the first of about 4 patches that I had for features I have been 
working on. I will go back and correct them in accordance with your suggestions 
and start submitting them.

Original comment by adamben...@gmail.com on 23 Feb 2012 at 9:16

[GoogleCodeExporter]

I have been trying to address the issue of making this context menu stuff a 
separate module and I have a question regarding how to hook it into svg-edit. I 
assumed that the correct way to do it was to make it an extension. If this is 
correct I will have to add a line to the svg-editor.html to include the 
extension.  Is this the way you'd me to proceed?

Adam 

Original comment by adamben...@gmail.com on 23 Feb 2012 at 11:01

[GoogleCodeExporter]

Attached are two patches which corrects the issues requested and also refactors 
the contextmenu behavior into a separate extension.

Original comment by adamben...@gmail.com on 23 Feb 2012 at 11:33

Attachments:

• 0007-cleaning-up-based-on-feedback-for-svg-edit-issue-907.patch
• 0010-refactored-addContextMenu-behavior-into-extension.patch

[GoogleCodeExporter]

Hi Adam - the move to a separate JS file doesn't have to be done with this 
change, but if you feel comfortable with it, great.  Changes are along the 
lines of what I did for refactoring in general:

  * create a new JS file, like I did with browser.js or svgtransformlist.js, making a new, unique namespace in the svgedit namespace
  * add it to the <!--{if svg_edit_release}> block in the svg-editor.html
  * add it to the Makefile so that it gets compiled in teh release version
  * add a JS unit test :)

Original comment by codedr...@gmail.com on 24 Feb 2012 at 12:21

[GoogleCodeExporter]

Oh, and I wont' have time to look at this until much later tonight or (more 
likely) some time tomorrow evening :-/

Original comment by codedr...@gmail.com on 24 Feb 2012 at 12:21

[GoogleCodeExporter]

Got it, I will re-re factor out of extension and into module as described. 
Thanks for your guidance.

Adam

Original comment by adamben...@gmail.com on 24 Feb 2012 at 12:35

[GoogleCodeExporter]

Oh, as for code style guide, we have none - but it can be picked up from 
working with the code.  Looking back, I think choosing tabs over a consistent 
number of spaces was a bad idea so a whitespace-only refactor at some point in 
the future would be cool if anyone is up for it.

Original comment by codedr...@gmail.com on 24 Feb 2012 at 4:52

[GoogleCodeExporter]

Looking at namespace options I can create a single svgEdit.contextMenu 
namespace to collection all context menu relation functionality, or I can make 
one just for this method svgEdit.addContextMenu... Do you have a preference?

Original comment by adamben...@gmail.com on 24 Feb 2012 at 4:29

[GoogleCodeExporter]

i would say svgedit.contextmenu sounds good to me.  You don't have to move 
everything in there initially.  I'm all for iterating and not large-scale 
refactors.

One nitpick is that all existing modules use 'svgedit' (all lower-case), so 
please follow that convention:

var svgedit = svgedit || {};

(function() {

if (!svgedit.contextmenu) {
        svgedit.contextmenu = {};
}

... your stuff here ...

})();

Original comment by codedr...@gmail.com on 24 Feb 2012 at 5:24

• Changed state: Accepted

[GoogleCodeExporter]

Excellent, will have a patch for you shortly.

Original comment by adamben...@gmail.com on 24 Feb 2012 at 5:31

• Removed labels: Priority-Medium, Type-Enhancement

[GoogleCodeExporter]

Alright, I think I have what you are looking for. Attached is a patch that 
factors context menu behavior into something like svgtransformlist.js. And 
tests are included :)

Original comment by adamben...@gmail.com on 24 Feb 2012 at 7:02

Attachments:

• 0001-factored-contextmenu-into-module-instead-of-extensio.patch

[GoogleCodeExporter]

Thanks Adam.  I've applied your patch in r2055.  Once you've verified the 
behavior, please close this bug.

One question: you say the dependencies are jQuery, but I just noticed there is 
a jquery.contextMenu.js plugin in the source tree.  Does your contextmenu.js 
depend on that?

Original comment by codedr...@gmail.com on 25 Feb 2012 at 2:15

[GoogleCodeExporter]

Original comment by codedr...@gmail.com on 25 Feb 2012 at 2:19

• Changed state: Accepted

[GoogleCodeExporter]

We depend directly on jQuery for the dom insertion of a new context menu, and 
indirectly we expect contextMenu is around to trigger the rest of the context 
menu behavior, however if it isn't around out changes wont fail to work you 
just wouldnt see any context menus at all.

Original comment by adamben...@gmail.com on 25 Feb 2012 at 9:33

[GoogleCodeExporter]

Thanks for fighting through the muddled patch.

Original comment by adamben...@gmail.com on 25 Feb 2012 at 9:45

• Changed state: Fixed