Bug 337867

Comment 1 Susan McCourt 2011-02-22 12:55:48 EST

Great!
(See also bug 337766)

My advice for now is to not leak too much knowledge about the command framework into the file extension.  Obviously information overlaps (icon, tooltip, name, id, etc.) but we can just make these attributes of the file extension point.

I also think we should avoid exposing terminology like "visibleWhen" or "enabledWhen" in the extension.  We need the same concept (look at the items and decide if you are valid), but I think using a generic term like "validWhen" can make it clear that there aren't UI decisions being made here.  From the command framework point of view, we'll be deciding when menu and toolbar items should simply disappear (in a hovering/transient toolset, like the actions column), or when they should just disable.  I want to talk to Paul W. about the whole visible/enabled distinction because I think we changed it going from 3.x to 4.0.

specifics:
- I agree info should have a tooltip
- we need a namespace qualified extension id (which will be used as command id)
- validWhen(items) ... I've been defining "items" as being either a single item or array of items.  That has to be handled, you can see examples of this in the visibleWhen expressions for the commands in fileCommands.js
- for now, I would model what you call the "scheduleCallback" along the lines of the editorActions extension.  In other words, you just call "service.run" with some defined set of parameters that are specified in the extension.  
- once all this is in place, the the command generation code in fileCommands.js would walk the extensions and generate commands.  You can look at how the editor commands are generated in editorFeatures.js
- then...once we get everything showing up, we can use command groupings to determine whether these things appear in the item action columns, or in the nav toolbar, or both, and how they are grouped (by separator or named menus).  I can help with this if the command groupings aren't obvious...
- another thing is that we currently have no command that operates on bulk selections (the checkmarks) in the navigator table.  I'm not sure I've hooked up the selection service yet to make this work.  For git commands, you could start by making them available at "object" scope (per item) and then I can make sure the navigator applies this scope for bulk commands using the selection service.  (I'm not sure how all this will work yet).

Comment 2 Susan McCourt 2011-02-22 13:01:27 EST

(In reply to comment #1)

> - another thing is that we currently have no command that operates on bulk
> selections (the checkmarks) in the navigator table.  I'm not sure I've hooked
> up the selection service yet to make this work.  For git commands, you could
> start by making them available at "object" scope (per item) and then I can make
> sure the navigator applies this scope for bulk commands using the selection
> service.  (I'm not sure how all this will work yet).

The tree nav handles the selections properly, so if you have commands that operate on bulk items, you can go ahead and implement them (for example, deleteFile is implemented this way).  And they will work in the tree nav.  I just need to add the plumbing that connects the nav-table checkmarks in the same way, and we need a good UI for distinguishing those actions on the toolbar that apply to your current location (such as new folder) and those that apply to the checked items (such as delete, push, etc.)

Comment 3 Boris Bokowski 2011-02-22 14:24:48 EST

(In reply to comment #0)
> We need some extra commands for GIT support (add, commit, pull, push etc) and
> since on Bug 336188 John is working on moving file service to Orion plugin we
> were planning to do the same with GIT service.
> Since GIT support would be optional (not every Orion will be required to
> support GIT) it's natural that we could create an Orion plugin to extend the
> commands for navigation toolbar and navigation table.

I would rather see us do a separate web page for the task of staging and committing changes to Git. So instead of adding commands that support Git to the navigator, I would like to have a separate page that shows, in effect, the information returned by the "git status" command.

Comment 4 Szymon Brandys 2011-02-22 14:33:19 EST

(In reply to comment #3)
> I would rather see us do a separate web page for the task of staging and
> committing changes to Git. So instead of adding commands that support Git to the
> navigator, I would like to have a separate page that shows, in effect, the
> information returned by the "git status" command.

I'm not sure why we can't have both? And IMO having actions in the navigator will make it easier to use. BTW git diff is already implemented and git status is in progress. You should be able to call GET /git/diff/[pathToFile] and it should return what you need.

Comment 5 Susan McCourt 2011-02-22 15:45:21 EST

Boris and I were on the phone when he typed this brief comment and we continued to discuss it a little.  I think there's a place for both.

I think there are different tasks and thus different workflows.  Here is how I think of it:

There are some things I do with git that have to do with "a file I'm working on."  I'm hacking away and I'm not even close to staging anything, but I realize I've screwed up something and want to get some code back.  In that case, I could imagine wanting to do a "show history" or even a "revert" from the navigator for a particular file.  In fact, I might even want to trigger it as a command from the editor.

Now, I agree with Boris that when it comes to staging, it feels like "another place" to me.  I want to go look at everything I've changed, decide what to stage, etc.  The way I work (I'm in the minority I realize) is that I work with the command line for most things, but when it comes to staging a bunch of changes, I switch to Git Gui.  Here I can see what's changed, do some diffing in-page, decide what to stage, and possibly commit.   

Now, if a merge has to occur, i might even want to go to a different page completely and do a side-by-side diff and merge.

Since this bug is called "extension point for file commands," I suggest that we stick to the file extension in this bug, and debate the larger UI issues in bug 336116.  The file extension point is needed for at least some git commands as well as other stuff, and we can evolve the UI as we go.

Comment 6 Boris Bokowski 2011-02-22 16:05:08 EST

(In reply to comment #4)
> I'm not sure why we can't have both? And IMO having actions in the navigator
> will make it easier to use. 

I am just trying to get everybody in the same mindset of "page == resource + task", and that we should err on the side of putting functionality on separate web pages instead of adding to existing ones. The default for new UI functionality should always be to think about how we can add a new separate page as opposed to how we can add to an existing page. I agree that we may want to combine functionality later to improve the flow. The key point is that we need to be able to defer decisions about this until later. If you develop it as features that live on a single page, it is going to be really hard to separate the code later. On the other hand, it is usually easy to combine functionality from independently developed components on one page.

Comment 7 Szymon Brandys 2011-02-23 06:08:43 EST

Git client will contribute actions to the navigator or toolbar. We should have at least an action that opens Git View for staging and committing. That is why fixing this bug is still important.

I imagine that the user may click the Open Git View action in the navigator what would open Git View showing the repository for the selected file. Then the user may start doing Git things.

Comment 8 Malgorzata Janczarska 2011-02-24 13:04:02 EST

I pushed first version, I had to make some changes to explorer tree & treetable to refresh the commands on every plugin response (that is asynchronous), so I appreciate any constructive criticism.
One thing I yet did not implement is visibleWhen. I will probably have to change the main idea because scheduling a function to plugin each time we want to determine validWhen may be long running for remote plugins and is asynchronous, so it would require changing the visibleWhen dynamics a little.
I think a good idea might be describing a pattern (or even better a list of patterns) that we might match with an item. For instance if we want to add a command to each \*.jpg file, the plugin would return a pattern {"name": "\*.jpg"}. If we want our command to apply to all directories that begin from "D", the pattern would be {"name": "D\*", "isDirectory": true}.

Comment 9 Malgorzata Janczarska 2011-02-24 13:05:35 EST

Created attachment 189725 [details]
sample plugin

This is a sample plugin I've been using to test the extention point.

Comment 10 Susan McCourt 2011-02-24 18:48:15 EST

(In reply to comment #8)
> I pushed first version, I had to make some changes to explorer tree & treetable
> to refresh the commands on every plugin response (that is asynchronous), so I
> appreciate any constructive criticism.

Overall, this looks quite nice.  The refresh changes make sense, I had the same issue with the editor commands.  I went ahead and pushed the sample plug-in so that others could easily try it or copy it.  

The main thing I think that needs work is deciding how much description of placement and grouping belongs in a plug-in extension point.  What you have feels very "Eclipse-ish" to me, except that it combines the notion of a command (the semantics) with the contribution (menu/toolbar, etc.)  Plug-ins can be quite specific about where their contributions can appear, define groups, locations, etc. but this leads to issues like "everyone thinks they should go first" and deeply nested menus and groupings.  In the end, we have to have code that decides who goes first when all else is equal.  So I prefer for now that the extension has no say in the matter, and that the page code makes these decisions based on semantic info provided by the command.  I say this because hopefully we have many less navigator commands than one would have in Eclipse since we just want to direct people to task-oriented pages.  The way we did editorActions (as a start) was to have *no* contribution info provided by the plug-in.  The editor page gets to decide how the commands appear, and I arbitrarily grouped the text ones in a "More" menu and the icon ones in the order I found them.  Separated by the "core" commands.  This is very anti-Eclipse but I think our world is different and I'd like to avoid rebuilding the contribution extensions if we can help it.

I think that "commandDescription.type" is not really "menu" vs "toolbar" but it's grappling with whether it is a bulk command or applies only to one thing.  We could then determine the contribution based on that info.  

> One thing I yet did not implement is visibleWhen. I will probably have to
> change the main idea because scheduling a function to plugin each time we want
> to determine validWhen may be long running for remote plugins and is
> asynchronous, so it would require changing the visibleWhen dynamics a little.
> I think a good idea might be describing a pattern (or even better a list of
> patterns) that we might match with an item. For instance if we want to add a
> command to each \*.jpg file, the plugin would return a pattern {"name":
> "\*.jpg"}. If we want our command to apply to all directories that begin from
> "D", the pattern would be {"name": "D\*", "isDirectory": true}.

Yes, it makes sense that a generic validWhen is not a good idea.  I think we'll run up against this with the editor extension, too. (For example, some commands aren't applicable if nothing is selected, and we could know that up front.)  

What you describe sounds like a reasonable start. I think we need to include the notion of whether it is intended to operate on one target (such as new folder) or could be used with multiples (such as delete).  This gets tricky because conceptually, even "new folder" could apply to many folder (create "foo" in every selected folder) but it's really not useful in that way.  

For Eclipse Desktop commands, we ended up with the powerful expression syntax that ended up replacing hard-coded concepts like how many items it applied to, file types, etc.  We could look at such a syntax so that the expression was supplied by the plug-in, but runs on the client.  But best to keep it simple and then we can evolve to this kind of thing. 

For now, I suggest we scrap the prefix and groupings, and just have each contribution be a single command.  (A plugin could provide more than one.)  Something liek this (pseudocode:)

something like this:  (pseudocode)

provider.registerServiceProvider("fileCommands", {
   info : function() { return {
      image: "/profile/images/create_user.gif",
      name: "Sample1Command",
      id: sample.commands.sample1,
      tooltip: "Sample1 tooltip",
      singleItemOnly: true,
      validationProperties: {"name": "D\*", "isDirectory": true}
   }}, ...		
		

I think that a common case might be even simpler.  The plug-in doesn't run the action, but just provides an href based on a selected file name.  This would work for many cases, and then a link could be built without ever having to contact the plugin again.  

provider.registerServiceProvider("fileCommands", {
   info : function() { return {
      image: "/profile/images/runUnitTest.gif",
      name: "Run tests,
      id: sample.commands.runtest,
      tooltip: "Run tests",
      href: "unitTestPage.html",
      validationProperties: {"name": "D\*", "isDirectory": true}
   }}, ...

Here we could hash the file name onto the page provided by the command.

Comment 11 Malgorzata Janczarska 2011-02-28 12:42:54 EST

I committed a functionality that parses and verifies validationProperties, so plugin can now restrict commands to based on item content.

As for commandDescription.type I agree that this is not the best pattern. Especially  type names are not suitable. I am however not sure if distinction between single and multiple item action is right. I think plenty of toolbar actions will apply to no items (for instance clone GIT repository). I am actually still a little confused what to do with action for multiple items. Currently actions in toolbar are invoked on the current item, not items selected in the tree.

I don't fully understand the "href" proposition. Do you mean the type of action that would redirect user to a given page? Like with editor? I think it would be a good idea to add this as an alternative to classic action, but a standard function invocation should be available as well, for actions like "add to git index", or already existing "delete" and "mark favorite".

For the groupings I think that you are right that adding a separate group doesn't make sense when a plugin contributes only one action, but we could probably use a action group if there are 3 actions contributed from one plugin I think it would be much more clear if they were grouped somehow.

Comment 12 Susan McCourt 2011-02-28 20:25:46 EST

(In reply to comment #11)
> As for commandDescription.type I agree that this is not the best pattern.
> Especially  type names are not suitable. I am however not sure if distinction
> between single and multiple item action is right. I think plenty of toolbar
> actions will apply to no items (for instance clone GIT repository). I am
> actually still a little confused what to do with action for multiple items.
> Currently actions in toolbar are invoked on the current item, not items
> selected in the tree.

I think discussion of "what's in the nav toolbar" is clouding this discussion.  This extension point is not about how to stick stuff in the nav toolbar.  It's about how to describe something you want to do to folder/files.  

If git clone doesn't apply to any file at all (because it's going to prompt you for what to clone), then I would argue it is not described by this extension point at all.  It's like open resource.  It is simply a global command and we can decide which pages it appears on and where it renders.  We could decide to put it in the same toolbar as nav actions, but that is a rendering decision (the same way we render global and local editor commands side by side even though they are defined differently).  Similarly, the "change view" between tree and table is shown on the nav toolbar but is not contributed by extension.

So in my mind, if a plug-in wants to operate on some files in the nav, telling us what kinds of files and whether it's one or more is enough.  Then we decide how to show that in the nav.

1) if it applies to multiples, it has to be represented as a bulk action playing off the selections.
2) if it applies to one only, we have some visual decisions to make:
   2a) we can put it as its own icon in the actions column 
   2b) we can put it in a "more" menu of actions column (grouped in tree menu)
   2c) we can put it in the nav toolbar if we want to apply it to the breadcrumb location.

I would argue as a starting point that 2a) and 2c) are reserved for our commands, and we don't expose this at all in the extension point.

To me the main question is whether we distinguish between single/multiple, thus choosing to render it as (1) or (2b).  Or whether we simply always do (1) and the action isn't shown if more than one thing is selected.  

I'm hand-waving right now about bulk actions because I don't have them represented yet in the nav toolbar or the checkmarks hooked in to the selection service.  I'm working on that right now for move/copy so this will be easier to discuss in the next day or two.

> 
> I don't fully understand the "href" proposition. Do you mean the type of action
> that would redirect user to a given page? Like with editor? I think it would be
> a good idea to add this as an alternative to classic action, but a standard
> function invocation should be available as well, for actions like "add to git
> index", or already existing "delete" and "mark favorite".

Again, let's distiguish between "file command extension" and the framework.
The function is needed for our delete, mark favorite, etc.  The command framework supports both already (hrefCallback and action callback).

But the question is...what do we expose in the extension?  I'm starting to like keeping it very loosely coupled and seeing how far we can go with just the href style.  The two examples for this extension that I've heard of are:
- open git status page on the repo that the file is contained in
- open the test results page on a test suite file and run tests

and they would be suited to this style.  

I realize you have tons of git commands that are more complex (push, pull, add to git index, etc.).  But my point is that these are just commands you'll define in your git page, and you can define whatever groupings you want.  You don't need to use this extension point to do so.

So I'm starting to like the idea of having this extension point be really dumb and just provide an href.  Then when we build the local actions, we could have a "go to" menu that contains things like "Git status", "Unit test" and it opens the href with the file as hash so that the page can do the right thing.

> For the groupings I think that you are right that adding a separate group
> doesn't make sense when a plugin contributes only one action, but we could
> probably use a action group if there are 3 actions contributed from one plugin
> I think it would be much more clear if they were grouped somehow.

Nothing will stop you from defining nice, named groupings for git commands in your git client code and putting them in the dom scope on your git page.  I'm just saying that we haven't yet proven that we need to expose all of that in the file commands extension point.  

If this still doesn't make sense, feel free to ping me and we can discuss.  I think we are just operating from different perspectives.  You have a bunch of git commands you know you need to implement.  But I think you'll end up doing most of those outside of this extension point.  I'd like this extension point to remain as simple as possible until we know we need more.

Comment 13 Boris Bokowski 2011-02-28 21:01:15 EST

(In reply to comment #12)
> But the question is...what do we expose in the extension?  I'm starting to like
> keeping it very loosely coupled and seeing how far we can go with just the href
> style.

+1 for loosely coupled for as long as possible!

> So I'm starting to like the idea of having this extension point be really dumb
> and just provide an href.  Then when we build the local actions, we could have
> a "go to" menu that contains things like "Git status", "Unit test" and it opens
> the href with the file as hash so that the page can do the right thing.

I like the "go to" idea - this might even generalize to more than just file commands. Something to keep in mind...

If we do come across examples where the simple/dumb extension point is not enough, how about we file a new bug to collect them until we have enough evidence to revisit our decision?

Comment 14 Szymon Brandys 2011-03-01 10:14:45 EST

I have the following cases to address:
- I have 'Profile' and 'Sign In' actions in the top-right corner of the navigator window. 'Profile' opens a separate page, while 'Sign In' just kills the user session.
- I need to add 'Git Clone' action to the navigator toolbar. Git clone may create a folder in the navigator, so it seems like it applies to files.
- I want 'Git Status' action next to each file. Using it should open a separate Git Status page.
- I want 'Git Reset' action next to each file in the navigator. I can imagine having it in the editor toolbar too in the future. It does not open a separate page. It just resets the content of selected resource and its children.

The question is. Which cases can be addressed using the command extension mechanism during M6? So far these things are hard coded in the navigator code.

One more case I have is:
- I would like to choose whether clicking 'Git Status' in the object toolbar will open a separate window or will just replace the current window content. We can decide how to open editors for files, since file names are just links. I wonder if we can do the same for object toolbar actions.

Comment 15 Malgorzata Janczarska 2011-03-01 10:56:34 EST

After pulling latest version today I noticed that Boris implemented hrefCallback for the plug-in functions.
This is the API he proposed:
1. in functions info he added a boolean "href" that only indicated if this command should be a link or a functions
2. if "href" was set to "true" the function "run" was expected to return a link build based on the item information

I think it's a good idea to let service build a link based on his own logic. If we decide that links always have the same format (<provided url>#<file url>) we force all plugins to consume this url format. This wouldn't be valid for the first potential command I have in mind, git status, that would probably be <git status page>#/git/status/<file id>, not <git status page>#/file/<file id>. I can also think of plenty examples where commands would like to redirect to a page that requires different file format, for instance w3 validator.

I made some changes to Boris implementation that I think are reasonable:
1. I moved "href" attribute from plugin info down to command description, so that it's defined separately for each command
2. I use getHref instead of run function to get the link, just because "run" does not reflect function use
3. Boris modified command.js to handle a promise from hrefCallback if returned, for menu commands. I did the same for image and link commands.

Comment 16 Boris Bokowski 2011-03-01 11:43:52 EST

(In reply to comment #15)
> After pulling latest version today I noticed that Boris implemented
> hrefCallback for the plug-in functions.
> This is the API he proposed:
> 1. in functions info he added a boolean "href" that only indicated if this
> command should be a link or a functions
> 2. if "href" was set to "true" the function "run" was expected to return a link
> build based on the item information

I just wanted to make the "run as unit test" functionality available. If you (Gosia, Susan) think that the API should be changed, simplified, or made more consistent then please go ahead and make changes (ideally, while keeping the "run as unit test" functionality in a working state). The code changes I made were just the quickest way to get something to work, I wanted to be able to demo it today. With the power outage today, the presentation has been postponed until next week.

Comment 17 Susan McCourt 2011-03-01 16:46:48 EST

(In reply to comment #15)

> I think it's a good idea to let service build a link based on his own logic. If
> we decide that links always have the same format (<provided url>#<file url>) we
> force all plugins to consume this url format. This wouldn't be valid for the
> first potential command I have in mind, git status, that would probably be <git
> status page>#/git/status/<file id>, not <git status page>#/file/<file id>. I
> can also think of plenty examples where commands would like to redirect to a
> page that requires different file format, for instance w3 validator.

Agree.  Discussed some further points with Gosia off-line, but to summarize here.

> 
> I made some changes to Boris implementation that I think are reasonable:
> 1. I moved "href" attribute from plugin info down to command description, so
> that it's defined separately for each command

In my mind, the extension = command, in the same way that one editor action = one command.  So I'm hoping that command description will go away, and instead, everything will be in info.  A plug-in could still provide the extension more than one time.  This gets rid of the naming/grouping stuff.  I believe that if we end up wanting plug-ins to group things and say where they go in the UI, that is a different extension altogether.  This is just "I have a command, here's what it operates on, here's what it does."  I don't know if I'll get to these changes today or not...that's what I'm working toward.

> 2. I use getHref instead of run function to get the link, just because "run"
> does not reflect function use

Seems reasonable.

> 3. Boris modified command.js to handle a promise from hrefCallback if returned,
> for menu commands. I did the same for image and link commands.

Thanks.

Comment 18 Susan McCourt 2011-03-01 23:37:57 EST

okay, I just committed changes that I'm happy with, based on our discussion from earlier today.

- got rid of the nested commandDescription and instead the sample plugin contributes three implementations of "fileCommands".  So all the commandDescription stuff is now collapsed into "info" and the groupings/namings/prefixes are gone.

- introduced "info.forceSingleItem".  If true, the contribution is placed in the object contribution.  For now, I also assume that info.href implies forceSingleItem, because we currently don't have logic to handle this, since we are replacing the window location.  I think this is reasonable.  

- if there is no info.forceSingleItem and no info.href, the command gets contributed to the "more actions" menu in the nav toolbar and the selection service is used to feed items to the command.  

- One could argue that just because something is a bulk action, it might also be good to appear in the object-level menu for single items.  We do this with some commands like delete, and may do so for copy/move.  But for now I'm trying to reduce clutter, so I'm not automatically putting plug-in bulk actions anywhere else.  (Not to mention we don't have a real example of this yet).

- since we have the info.href flag, it didn't make sense to me to also have different methods for run and getHref.  I would either expect to have two methods and no flag...
    if (service.getHref) {service.getHref(item)}
or else a flag and then the same method
    if (service.href) {service.run(item)}
If we have both the flag and separate methods, it's just more to get wrong.  (for example gitPlugin was using href=true with a run method.)

- updated the gitPlugin, unittestPlugin, and sampleCommandsPlugin.  note that gitPlugin and unittestPlugin both use the href, so only the sample shows how to handle bulk selections or functions.

- fixed up typos in the git plugin.  While I was doing so, Libing committed the test page so it actually launches the page now.

One thing I don't like is that because we render these href commands in a menu, we don't get a real link, and hence the user can't decide whether to open a new browser tab or replace the document.  I opened bug 338618 for this.

From my point of view we can close this bug and open new bugs as we find them (such as wanting more control over how plugin commands are contributed to various UI).  But this was Gosia's bug so I'll leave it to her to decide if we are done for M6.

Comment 19 Malgorzata Janczarska 2011-03-02 08:58:41 EST

Thanks Susan.

I added only some validation of validateProperties for bulk commands. If you add validateProperties to any bulk command now it will be displayed only if all selected items match the properties mask.
I think for now this bug is done.

Comment 20 Susan McCourt 2011-03-02 11:46:40 EST

(In reply to comment #19)
> I added only some validation of validateProperties for bulk commands. If you
> add validateProperties to any bulk command now it will be displayed only if all
> selected items match the properties mask.
> I think for now this bug is done.

Thanks!  Forgot about that.