Merge lp:~simon-kersey/bzr-explorer/add-tool-cmd-selected-support into lp:bzr-explorer

[Simon Kersey]

Adds support for population of tool action command placeholder %(wt_selected)s from the items selected in the working tree. It will work with both the classic and qbrowse styles of working tree.

Currently only tested on Windows 7.

To test it:
- Start Bzr Explorer
- Select Tools->Edit My Tools
- Add the following lines to tools.xml:
  <tool action="ls %(wt_selected)s" icon="actions/document-properties" title="List" type="bzr" />
  <tool action="ls %(wt_selected)s" icon="actions/edit-select-all" title="List Now" type="bzr-exec" />
- Save updated tools.xml
- Open a branch with a working tree that contains sub-directories
- Select a sub-directory in the working tree browser
- Click the 'List' tool in the Toolox (or select from the Tools menu)
- Run Bzr Command dialog should be displayed, 'options and arguments for command' should be populated with selected sub-directory
- Click the 'List Now' tool
- Run Bzr Command dialog should be displayed, automatically running ls command on selected sub-directory
- Change style of working tree (Welcome Tab->Setup and Personalise...->Options)
- Repeat tests for different working tree style
NOTE: When using the Classic style of working tree the full path is provided

[Alexander Belchenko (bialix)]

Ping to myself.

[Alexander Belchenko (bialix)]

Simon, I see there is 2 news items in one patch. This one seems unrelated:

"Fixed nested folders in toolbars.xml (Simon Kersey, #564811)"

Is it just leftover from another patch? Or is it added by mistake?

Anyway, if you still want to see this patch merged I'd like you to land it. Otherwise,... well, you know what to do ;-)

Sorry for not responding on it before.

review: Needs Information

[Alexander Belchenko (bialix)]

Please, update the news for post 1.1.1 and land the patch.

review: Needs Fixing

[Simon Kersey (simon-kersey)]

> Please, update the news for post 1.1.1 and land the patch.

I did some more testing when I updated the patch. There is a problem with using the %selected element to hold the selected file from the working tree browser as subsequently, when using the log button on the menu bar, the log of the working tree selected item is shown not that of the whole branch.

[Alexander Belchenko (bialix)]

Does it mean the patch is not ready yet?

[Simon Kersey (simon-kersey)]

> Does it mean the patch is not ready yet?

Unfortunately yes.

[Alexander Belchenko (bialix)]

Then it worth for patch submitter to change Status to "Work in progress". And change it back to "Needs Review" when new version available.

[Simon Kersey (simon-kersey)]

Updated patch to restore original location_context. This stops the log button behaviour identified above.

[Alexander Belchenko (bialix)]

> Adds support for population of tool action command placeholder %(selected)s from the items selected in the working tree. It will work with both the classic and qbrowse styles of working tree.
>
> Currently only tested on Windows XP.

Hi Simon,

It looks OK for me, but I think we need to add some documentation about
this feature, and maybe about other related to tools.xml.

We have branch lp:bzr-explorer-website which is used to generate docs
for http://doc.bazaar-vcs.org/explorer/en/index.html

It will be nice if you add some documentation about your new feature there.

Thanks

[Alexander Belchenko (bialix)]

> Updated patch to restore original location_context. This stops the log button
> behaviour identified above.

This has been stalled for a long time so I've lost the track.

Simon, can we land it now? Should we land it now?

What about adding some documentation on the config files for explorer?

[Alexander Belchenko (bialix)]

review: Needs Information

[Simon Kersey (simon-kersey)]

I have not had much time to progress this.
I have some reservations about landing it. Mainly due to reconsidering the last fix that I put in.
I think that I should go back to the original idea I had which was to introduce a new token that contained whatever was selected in the working tree (e.g. %wt_selected%). The problem was caused by %selected% being used by exisiting code to represent something else (in this case I think it was what was selected in a previous screen)so that when the log button was pressed you got a log for the selected branch. The other issue is knowing what %selected% actually means. As you make changes should %selected% represent the items modified etc. What I wanted to implement was a simple way to allow tools to use what was selected in the working tree so that plugins could provide a richer interface.
If you think that overloading the %selected% token (so that, when used in tool definitions, the token is replaced by what is selected in the working tree or, if nothing is selected in the working tree, left with the existing content) is OK then it should be OK to merge in and I will put up some documentation updates in separate patch.
I think my preference is to use a new token for the avoidance of doubt about behaviour.
Any thoughts?

[Alexander Belchenko (bialix)]

Simon Kersey пишет:
> I think my preference is to use a new token for the avoidance of doubt about behaviour.

I agree with that.

--
All the dude wanted was his rug back

[Alexander Belchenko (bialix)]

Simon, I'm marking this MP as Work-In-Progress. Once you finish the change as you want to, please change it back to Needs Review.

review: Needs Fixing

[Alexander Belchenko (bialix)]

Simon,

I don't understand why do you put single quotes around multiple paths as in code:

+ self.location_context['wt_selected'] = "'"+ \
+ ("' '".join(paths))+"'"

[Simon Kersey (simon-kersey)]

> Simon,
>
> I don't understand why do you put single quotes around multiple paths as in
> code:
>
> + self.location_context['wt_selected'] = "'"+ \
> + ("' '".join(paths))+"'"

The reason they are quoted is to cope with paths that contain spaces.

The reason they are quoted with ' rather than " is that "s get removed when populating the qrun options and arguments list.

I'm not sure whether it is the call to self._escape_context or mod_app_suite.command_to_args (line 992) or the call to self.run_app (line 1004) or indeed qrun itself that does it. Without the quotes, any path with spaces is parsed as separate arguments by qrun giving the wrong result.

If you add the following tools to tools.xml (Tools->Edit My Tools) then you can test the behavior with multiple file selections:
  <tool action="add --dry-run %(wt_selected)s" icon="actions/document-properties" title="Add" type="bzr" />
  <tool action="add --dry-run %(wt_selected)s" icon="actions/edit-select-all" title="Add Now" type="bzr-exec" />

(The example tools I listed in the testing section above are based on bzr ls which only takes a single path as an argument and so it will give an error if multiple paths are selected.)

[Alexander Belchenko (bialix)]

Simon Kersey пишет:
>> Simon,
>>
>> I don't understand why do you put single quotes around multiple paths as in
>> code:
>>
>> + self.location_context['wt_selected'] = "'"+ \
>> + ("' '".join(paths))+"'"
>
> The reason they are quoted is to cope with paths that contain spaces.
>
> The reason they are quoted with ' rather than " is that "s get removed when populating the qrun options and arguments list.

OK, I see now. But your code is hard to read and understand. Why not
quote each argument with spaces and then join them? E.g.

self.location_context['wt_selected'] = ' '.join(["'%u'"%p for p in paths])

I hope that code better explains what it wants to do. What do you think?

> I'm not sure whether it is the call to self._escape_context or mod_app_suite.command_to_args (line 992) or the call to self.run_app (line 1004) or indeed qrun itself that does it. Without the quotes, any path with spaces is parsed as separate arguments by qrun giving the wrong result.

It will be better if we can pass list of arguments to extend the command
line instead of manual join & quote them. But this is not here right
now, so your current approach is OK for me.

--
All the dude wanted was his rug back

[Simon Kersey (simon-kersey)]

I have done some experimenting and if you run the following from a Windows command prompt:

bzr qrun add -- --dry-run "path with spaces\1 2.txt" "path with spaces\3 4.txt"

Then when the qrun dialog is displayed there are no quotes.

As for the change above, I notice that you have used %u (which seems to be an obsolete form of %d) is this for a particular reason?

I will update the code tonight. My current thoughts are to update _escape_context_items to take an additional optional parameter q which defaults to " and then to use the following as per your suggestion above:

                        escaped = [self._escape_context_item(s,"'") for s in paths]
                        self.location_context['wt_selected'] = " ".join(escaped)

[Alexander Belchenko (bialix)]

Simon Kersey пишет:
> I have done some experimenting and if you run the following from a Windows command prompt:
>
> bzr qrun add -- --dry-run "path with spaces\1 2.txt" "path with spaces\3 4.txt"

That's because bzr command-line processor split the arguments into list
(without quotes of course) and then we put those arguments into qrun
dialog without noticing that some arguments may require quoting. It's a
definitely bug in qrun. Care to file a bug?

> Then when the qrun dialog is displayed there are no quotes.
>
> As for the change above, I notice that you have used %u (which seems to be an obsolete form of %d) is this for a particular reason?

Sorry, my bad. I was thinking about forcing unicode, actually it should
be u"'%s'" % p instead of %u.

>
> I will update the code tonight. My current thoughts are to update _escape_context_items to take an additional optional parameter q which defaults to " and then to use the following as per your suggestion above:
>
> escaped = [self._escape_context_item(s,"'") for s in paths]
> self.location_context['wt_selected'] = " ".join(escaped)

That makes sense for me.

--
All the dude wanted was his rug back

[lp:~simon-kersey/bzr-explorer/add-tool-cmd-selected-support]

476. By Simon Kersey on 2011-08-11

Clarify path quoting

477. By Simon Kersey on 2011-08-11

Fix missing line continuation

[Simon Kersey (simon-kersey)]

Alexander,

I have finished the updates and raised the qrun bug so I think this update is ready to go.

Cheers,

Simon

[Alexander Belchenko (bialix)]

Looks good for me. Land it.

review: Approve

=== modified file 'NEWS'
--- NEWS	2011-08-04 04:07:53 +0000
+++ NEWS	2011-08-11 09:18:31 +0000
@@ -4,6 +4,19 @@
 
 .. contents::
 
+1.2.2 dd-mmm-2011
+-----------------
+
+New features:
+
+Improvements:
+
+* Added support to populate %(wt_selected)s tool action command placeholder from
+  working tree selected items.
+
+Bug Fixes:
+
+
 1.2.1 04-Aug-2011
 -----------------
 

=== modified file 'lib/app_suite.py'
--- lib/app_suite.py	2011-08-02 12:55:40 +0000
+++ lib/app_suite.py	2011-08-11 09:18:31 +0000
@@ -45,6 +45,7 @@
     "EDITOR",
     "BZR_LOG",
     "branch_root",
+    "wt_selected",
     ]
 
 
@@ -64,6 +65,7 @@
     * EDITOR - preferred editor path and options
     * BZR_LOG - path to .bzr.log file
     * branch_root - the root of the branch (for a checkout or branch)
+    * wt_selected - space separated list of paths selected in working tree    
 
     Placeholder are defined in templates using the format %(name)s.
 

=== modified file 'lib/explorer.py'
--- lib/explorer.py	2011-08-02 12:55:40 +0000
+++ lib/explorer.py	2011-08-11 09:18:31 +0000
@@ -958,11 +958,38 @@
                 mutter("skipping unknown action %s building tool-menu" % name)
 
     def _do_open_tool(self, tool):
+        old_loc_context = []
         if tool.type == 'link':
             link = mod_app_suite.command_to_expanded(tool.action,
                 self._escape_context(self.location_context))
             self.open_web_browser(link)
         else:
+            if  (tool.uses_selected):
+                # populate 'wt_selected' info for context from working tree
+                if isinstance(self._wt_browser._browser,
+                              wt_browser._QBrowseStyleBrowser):
+                    selected_items = \
+                        self._wt_browser._browser._tree_viewer.\
+                        get_selection_items()
+                    if len(selected_items) > 0:
+                        old_loc_context = self.location_context.copy()
+                        paths = [item.path for item in selected_items]
+                        # NOTE:
+                        # Unless items are quoted then filenames with spaces
+                        # in are not handled properly on Windows
+                        escaped = [self._escape_context_item(p, "'")\
+                            for p in paths]
+                        self.location_context['wt_selected'] = " ".join(escaped)
+                elif isinstance(self._wt_browser._browser,
+                                wt_browser._ClassicBrowser):
+                    if self._wt_browser._browser.\
+                        _action_panel._selected_fileinfo:
+                        old_loc_context = self.location_context.copy()
+                        selected_item = self._wt_browser._browser.\
+                            _action_panel._selected_fileinfo.filePath()
+                        self.location_context['wt_selected'] = \
+                            self._escape_context_item(selected_item, "'")
+            #
             args = mod_app_suite.command_to_args(tool.action,
                 self._escape_context(self.location_context))
             if (tool.type == 'bzr') or (tool.type == 'bzr-exec'):
@@ -976,6 +1003,8 @@
                 else:
                     args = ['bzr', 'qrun', '--ui-mode', '--'] + args
             self.run_app(args, use_shell=(tool.type=='shell'))
+            if old_loc_context:
+                self.location_context = old_loc_context.copy()
 
     ## Show message helpers ##
 
@@ -1467,10 +1496,10 @@
             result[key] = self._escape_context_item(value)
         return result
 
-    def _escape_context_item(self, s):
+    def _escape_context_item(self, s, q='"'):
         """If s contains spaces, surround it with quotes, otherwise return it."""
-        if s and s.find(' ') >= 0:
-            return '"%s"' % (s,)
+        if s and ' ' in s:
+            return '%s%s%s' % (q,s,q)
         else:
             return s
 

=== modified file 'lib/extensions/tools.py'
--- lib/extensions/tools.py	2011-08-02 12:55:40 +0000
+++ lib/extensions/tools.py	2011-08-11 09:18:31 +0000
@@ -57,6 +57,7 @@
         self.action = action
         self.conditions = conditions
         self.icon = icon
+        self.uses_selected = (action.find("%(wt_selected)s") != -1) 
 
     def __repr__(self):
         return "Tool(%s, %s, %s)" % (self.title, self.type, self.action)