Merge lp:~simon-kersey/bzr-explorer/add-tool-cmd-selected-support into lp:bzr-explorer
- add-tool-cmd-selected-support
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Merged at revision: | 534 | ||||
Proposed branch: | lp:~simon-kersey/bzr-explorer/add-tool-cmd-selected-support | ||||
Merge into: | lp:bzr-explorer | ||||
Diff against target: |
121 lines (+48/-3) 4 files modified
NEWS (+13/-0) lib/app_suite.py (+2/-0) lib/explorer.py (+32/-3) lib/extensions/tools.py (+1/-0) |
||||
To merge this branch: | bzr merge lp:~simon-kersey/bzr-explorer/add-tool-cmd-selected-support | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Alexander Belchenko | Approve | ||
Review via email: mp+70794@code.launchpad.net |
This proposal supersedes a proposal from 2010-04-19.
Commit message
Description of the change
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/
<tool action="ls %(wt_selected)s" icon="actions/
- 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.
- Repeat tests for different working tree style
NOTE: When using the Classic style of working tree the full path is provided
Alexander Belchenko (bialix) wrote : Posted in a previous version of this proposal | # |
Alexander Belchenko (bialix) wrote : Posted in a previous version of this proposal | # |
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.
Alexander Belchenko (bialix) wrote : Posted in a previous version of this proposal | # |
Please, update the news for post 1.1.1 and land the patch.
Simon Kersey (simon-kersey) wrote : Posted in a previous version of this proposal | # |
> 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) wrote : Posted in a previous version of this proposal | # |
Does it mean the patch is not ready yet?
Simon Kersey (simon-kersey) wrote : Posted in a previous version of this proposal | # |
> Does it mean the patch is not ready yet?
Unfortunately yes.
Alexander Belchenko (bialix) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
Updated patch to restore original location_context. This stops the log button behaviour identified above.
Alexander Belchenko (bialix) wrote : Posted in a previous version of this proposal | # |
> 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://
It will be nice if you add some documentation about your new feature there.
Thanks
Alexander Belchenko (bialix) wrote : Posted in a previous version of this proposal | # |
> 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) : Posted in a previous version of this proposal | # |
Simon Kersey (simon-kersey) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
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.
Alexander Belchenko (bialix) wrote : | # |
Simon,
I don't understand why do you put single quotes around multiple paths as in code:
+ self.location_
+ ("' '".join(paths))+"'"
Simon Kersey (simon-kersey) wrote : | # |
> Simon,
>
> I don't understand why do you put single quotes around multiple paths as in
> code:
>
> + self.location_
> + ("' '".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_
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/
<tool action="add --dry-run %(wt_selected)s" icon="actions/
(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) wrote : | # |
Simon Kersey пишет:
>> Simon,
>>
>> I don't understand why do you put single quotes around multiple paths as in
>> code:
>>
>> + self.location_
>> + ("' '".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_
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_
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) wrote : | # |
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_
Alexander Belchenko (bialix) wrote : | # |
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_
>
> escaped = [self._
> self.location_
That makes sense for me.
--
All the dude wanted was his rug back
- 476. By Simon Kersey
-
Clarify path quoting
- 477. By Simon Kersey
-
Fix missing line continuation
Simon Kersey (simon-kersey) wrote : | # |
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) wrote : | # |
Looks good for me. Land it.
Preview Diff
1 | === modified file 'NEWS' | |||
2 | --- NEWS 2011-08-04 04:07:53 +0000 | |||
3 | +++ NEWS 2011-08-11 09:18:31 +0000 | |||
4 | @@ -4,6 +4,19 @@ | |||
5 | 4 | 4 | ||
6 | 5 | .. contents:: | 5 | .. contents:: |
7 | 6 | 6 | ||
8 | 7 | 1.2.2 dd-mmm-2011 | ||
9 | 8 | ----------------- | ||
10 | 9 | |||
11 | 10 | New features: | ||
12 | 11 | |||
13 | 12 | Improvements: | ||
14 | 13 | |||
15 | 14 | * Added support to populate %(wt_selected)s tool action command placeholder from | ||
16 | 15 | working tree selected items. | ||
17 | 16 | |||
18 | 17 | Bug Fixes: | ||
19 | 18 | |||
20 | 19 | |||
21 | 7 | 1.2.1 04-Aug-2011 | 20 | 1.2.1 04-Aug-2011 |
22 | 8 | ----------------- | 21 | ----------------- |
23 | 9 | 22 | ||
24 | 10 | 23 | ||
25 | === modified file 'lib/app_suite.py' | |||
26 | --- lib/app_suite.py 2011-08-02 12:55:40 +0000 | |||
27 | +++ lib/app_suite.py 2011-08-11 09:18:31 +0000 | |||
28 | @@ -45,6 +45,7 @@ | |||
29 | 45 | "EDITOR", | 45 | "EDITOR", |
30 | 46 | "BZR_LOG", | 46 | "BZR_LOG", |
31 | 47 | "branch_root", | 47 | "branch_root", |
32 | 48 | "wt_selected", | ||
33 | 48 | ] | 49 | ] |
34 | 49 | 50 | ||
35 | 50 | 51 | ||
36 | @@ -64,6 +65,7 @@ | |||
37 | 64 | * EDITOR - preferred editor path and options | 65 | * EDITOR - preferred editor path and options |
38 | 65 | * BZR_LOG - path to .bzr.log file | 66 | * BZR_LOG - path to .bzr.log file |
39 | 66 | * branch_root - the root of the branch (for a checkout or branch) | 67 | * branch_root - the root of the branch (for a checkout or branch) |
40 | 68 | * wt_selected - space separated list of paths selected in working tree | ||
41 | 67 | 69 | ||
42 | 68 | Placeholder are defined in templates using the format %(name)s. | 70 | Placeholder are defined in templates using the format %(name)s. |
43 | 69 | 71 | ||
44 | 70 | 72 | ||
45 | === modified file 'lib/explorer.py' | |||
46 | --- lib/explorer.py 2011-08-02 12:55:40 +0000 | |||
47 | +++ lib/explorer.py 2011-08-11 09:18:31 +0000 | |||
48 | @@ -958,11 +958,38 @@ | |||
49 | 958 | mutter("skipping unknown action %s building tool-menu" % name) | 958 | mutter("skipping unknown action %s building tool-menu" % name) |
50 | 959 | 959 | ||
51 | 960 | def _do_open_tool(self, tool): | 960 | def _do_open_tool(self, tool): |
52 | 961 | old_loc_context = [] | ||
53 | 961 | if tool.type == 'link': | 962 | if tool.type == 'link': |
54 | 962 | link = mod_app_suite.command_to_expanded(tool.action, | 963 | link = mod_app_suite.command_to_expanded(tool.action, |
55 | 963 | self._escape_context(self.location_context)) | 964 | self._escape_context(self.location_context)) |
56 | 964 | self.open_web_browser(link) | 965 | self.open_web_browser(link) |
57 | 965 | else: | 966 | else: |
58 | 967 | if (tool.uses_selected): | ||
59 | 968 | # populate 'wt_selected' info for context from working tree | ||
60 | 969 | if isinstance(self._wt_browser._browser, | ||
61 | 970 | wt_browser._QBrowseStyleBrowser): | ||
62 | 971 | selected_items = \ | ||
63 | 972 | self._wt_browser._browser._tree_viewer.\ | ||
64 | 973 | get_selection_items() | ||
65 | 974 | if len(selected_items) > 0: | ||
66 | 975 | old_loc_context = self.location_context.copy() | ||
67 | 976 | paths = [item.path for item in selected_items] | ||
68 | 977 | # NOTE: | ||
69 | 978 | # Unless items are quoted then filenames with spaces | ||
70 | 979 | # in are not handled properly on Windows | ||
71 | 980 | escaped = [self._escape_context_item(p, "'")\ | ||
72 | 981 | for p in paths] | ||
73 | 982 | self.location_context['wt_selected'] = " ".join(escaped) | ||
74 | 983 | elif isinstance(self._wt_browser._browser, | ||
75 | 984 | wt_browser._ClassicBrowser): | ||
76 | 985 | if self._wt_browser._browser.\ | ||
77 | 986 | _action_panel._selected_fileinfo: | ||
78 | 987 | old_loc_context = self.location_context.copy() | ||
79 | 988 | selected_item = self._wt_browser._browser.\ | ||
80 | 989 | _action_panel._selected_fileinfo.filePath() | ||
81 | 990 | self.location_context['wt_selected'] = \ | ||
82 | 991 | self._escape_context_item(selected_item, "'") | ||
83 | 992 | # | ||
84 | 966 | args = mod_app_suite.command_to_args(tool.action, | 993 | args = mod_app_suite.command_to_args(tool.action, |
85 | 967 | self._escape_context(self.location_context)) | 994 | self._escape_context(self.location_context)) |
86 | 968 | if (tool.type == 'bzr') or (tool.type == 'bzr-exec'): | 995 | if (tool.type == 'bzr') or (tool.type == 'bzr-exec'): |
87 | @@ -976,6 +1003,8 @@ | |||
88 | 976 | else: | 1003 | else: |
89 | 977 | args = ['bzr', 'qrun', '--ui-mode', '--'] + args | 1004 | args = ['bzr', 'qrun', '--ui-mode', '--'] + args |
90 | 978 | self.run_app(args, use_shell=(tool.type=='shell')) | 1005 | self.run_app(args, use_shell=(tool.type=='shell')) |
91 | 1006 | if old_loc_context: | ||
92 | 1007 | self.location_context = old_loc_context.copy() | ||
93 | 979 | 1008 | ||
94 | 980 | ## Show message helpers ## | 1009 | ## Show message helpers ## |
95 | 981 | 1010 | ||
96 | @@ -1467,10 +1496,10 @@ | |||
97 | 1467 | result[key] = self._escape_context_item(value) | 1496 | result[key] = self._escape_context_item(value) |
98 | 1468 | return result | 1497 | return result |
99 | 1469 | 1498 | ||
101 | 1470 | def _escape_context_item(self, s): | 1499 | def _escape_context_item(self, s, q='"'): |
102 | 1471 | """If s contains spaces, surround it with quotes, otherwise return it.""" | 1500 | """If s contains spaces, surround it with quotes, otherwise return it.""" |
105 | 1472 | if s and s.find(' ') >= 0: | 1501 | if s and ' ' in s: |
106 | 1473 | return '"%s"' % (s,) | 1502 | return '%s%s%s' % (q,s,q) |
107 | 1474 | else: | 1503 | else: |
108 | 1475 | return s | 1504 | return s |
109 | 1476 | 1505 | ||
110 | 1477 | 1506 | ||
111 | === modified file 'lib/extensions/tools.py' | |||
112 | --- lib/extensions/tools.py 2011-08-02 12:55:40 +0000 | |||
113 | +++ lib/extensions/tools.py 2011-08-11 09:18:31 +0000 | |||
114 | @@ -57,6 +57,7 @@ | |||
115 | 57 | self.action = action | 57 | self.action = action |
116 | 58 | self.conditions = conditions | 58 | self.conditions = conditions |
117 | 59 | self.icon = icon | 59 | self.icon = icon |
118 | 60 | self.uses_selected = (action.find("%(wt_selected)s") != -1) | ||
119 | 60 | 61 | ||
120 | 61 | def __repr__(self): | 62 | def __repr__(self): |
121 | 62 | return "Tool(%s, %s, %s)" % (self.title, self.type, self.action) | 63 | return "Tool(%s, %s, %s)" % (self.title, self.type, self.action) |
Ping to myself.