Merge lp:~dorins/qbzr/qdiff-changes into lp:qbzr
| Status: | Merged | ||||
|---|---|---|---|---|---|
| Merged at revision: | 1368 | ||||
| Proposed branch: | lp:~dorins/qbzr/qdiff-changes | ||||
| Merge into: | lp:qbzr | ||||
| Diff against target: |
1172 lines (+463/-206) 6 files modified
data/qbzr.qrc (+3/-1) lib/commands.py (+50/-45) lib/diff_arg.py (+40/-31) lib/diffwindow.py (+210/-100) lib/resources.py (+144/-28) lib/util.py (+16/-1) |
||||
| To merge this branch: | bzr merge lp:~dorins/qbzr/qdiff-changes | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Alexander Belchenko | Approve on 2011-04-27 | ||
| Gary van der Merwe | 2010-09-06 | Pending | |
|
Review via email:
|
|||
Description of the Change
Implements the following changes on the qdiff window:
- Move actions from the bottom of the window on a toolbar, similar to the toolbar from the qannotate window
- Add a Find actions which toggles a find toolbar. Find action acts on the active panel - the panel that had focus last.
- Add shortcut keys for "Refresh", "Find" and "Toggle view mode" actions and show shortcut key hints on tooltips for these actions and also for "Find next", "Find previous" actions.
- Add an "Ignore whitespace changes" option.
| Alexander Belchenko (bialix) wrote : | # |
| Alexander Belchenko (bialix) wrote : | # |
Shortcut for switching between Side-by-Side and Unidiff is Crtl+| which means pressing Ctrl+Shift+\ on my keyboard. Is using Shift intended here? Maybe it worth to use something simler instead?
| Alexander Belchenko (bialix) wrote : | # |
I see you have added Ignore whitespace changes option. Does it related to this merge proposal: https:/
| Alexander Belchenko (bialix) wrote : | # |
Alexander Belchenko пишет:
> Shortcut for switching between Side-by-Side and Unidiff is Crtl+| which means pressing Ctrl+Shift+\ on my keyboard. Is using Shift intended here? Maybe it worth to use something simler instead?
I suggest using Ctrl+U.
| Dorin Scutarașu (dorins) wrote : | # |
> I see you have added Ignore whitespace changes option. Does it related to this
> merge proposal: https:/
> whitespace/
Yes. Looks like the other branch has similar changes except that the UI is different: I added a menu item to toggle whitespace changes, the other branch uses a command line parameter.
- 1317. By Dorin Scutarașu on 2010-10-06
-
* Replaced "Side by side" toolbar button with "Unidiff" button to reflect default view mode.
* Changed shortcut key for toggling view mode to "Ctrl-U".
| Dorin Scutarașu (dorins) wrote : | # |
> Although it's not very intuitive that clicking lowered Side-by-side button
> will give me unidiff view.
I replaced the "Side by side" button with a "Unidiff" button which makes more sense since side by side is the default. Also, I changed the shortcut key to "Ctrl-U". Thanks for the feedback.
| Alexander Belchenko (bialix) wrote : | # |
I like new variant. I have some doubts about using 3-panel icon for Unidiff button now, but I have no good suggestions.
Also I found that External Diff button does not work. Have you tested it?
| Alexander Belchenko (bialix) wrote : | # |
Dorin Scutarașu пишет:
>> I see you have added Ignore whitespace changes option. Does it related to this
>> merge proposal: https:/
>> whitespace/
>
> Yes. Looks like the other branch has similar changes except that the UI is different: I added a menu item to toggle whitespace changes, the other branch uses a command line parameter.
@Dorin, can you merge the changes from the other branch, so I can land
both branches soon? Or I can land the other branch first and you will
update your patch later?
@Glen, can you help Dorin to merge your patches, or do you prefer to
land your patch first?
Maybe you guys can collaborate on this feature?
| Glen Mailer (glenjamin) wrote : | # |
I think the best approach will be to merge my command line changes stuff into this branch - I can have a go at that this weekend.
Interestingly my first approach to whitespace removal was the regular expression substitution, but I replaced it with string.whitespace and string.translate after seeing Dorin's approach - meanwhile Dorin appears to have started with string.whitespace and moved to regular expression substitution.
Any ideas which is preferrable?
| Dorin Scutarașu (dorins) wrote : | # |
On 7 octombrie 2010, 13:43, Glen Mailer <email address hidden> wrote:
> I think the best approach will be to merge my command line changes stuff into this branch - I can have a go at that this weekend.
>
Sounds like a plan.
> Interestingly my first approach to whitespace removal was the regular expression substitution, but I replaced it with string.whitespace and string.translate after seeing Dorin's approach - meanwhile Dorin appears to have started with string.whitespace and moved to regular expression substitution.
>
> Any ideas which is preferrable?
> --
> https:/
> You are the owner of lp:~dorins/qbzr/qdiff-changes.
>
I made some tests and it looks like string.translate is fastest. I
switched to using regular expresion substitution because I had some
problems with string.translate:
* python >= 2.6 accepts None as the first parameter, but older python
versions don't.
* string.translate delegates to either str.translate(s, table,
delchars=None) or unicode.
strings when passing delchars argument.
I switched to regular expressions at the time thinking that I'll get back
to it and figure out how to use 'translate' safely, but I didn't manage to
do that so far.
- 1318. By Dorin Scutarașu on 2010-10-14
-
Fixed "External Diff" toolbar button.
- 1319. By Dorin Scutarașu on 2010-10-14
-
Add shortcut keys: Alt+V pops up the view menu, Alt+E pops up the "External Diff" menu.
The shortcut keys are appended to the respective tool tips.
Also, "Complete" action is now accessible using "Alt+V,C" and "Ignore Whitespace changes" is accessible using "Alt+V,I". - 1320. By Dorin Scutarașu on 2010-10-15
-
Merge with lp:~glenjamin/qbzr/qdiff-ignore-whitespace.
Adds --ignore-whitespace option in qdiff. - 1321. By Dorin Scutarașu on 2010-10-15
-
Merge trunk.
| Dorin Scutarașu (dorins) wrote : | # |
> Also I found that External Diff button does not work. Have you tested it?
I missed that somehow. Anyway, I fixed it today.
> Dorin Scutarașu пишет:
> > > I see you have added Ignore whitespace changes option. Does it related to this
> > > merge proposal: https:/
> > > whitespace/
> >
> > Yes. Looks like the other branch has similar changes except that the
> > UI is different: I added a menu item to toggle whitespace changes,
> > the other branch uses a command line parameter.
> @Dorin, can you merge the changes from the other branch, so I can land
> both branches soon? Or I can land the other branch first and you will
> update your patch later?
I just merged the changes from this merge proposal:
https:/
, which exposes an --ignore-whitespace option to qdiff.
- 1322. By Dorin Scutarașu on 2010-10-16
-
Cleanup.
| Glen Mailer (glenjamin) wrote : | # |
> > Also I found that External Diff button does not work. Have you tested it?
>
> I missed that somehow. Anyway, I fixed it today.
>
> > Dorin Scutarașu пишет:
> > > > I see you have added Ignore whitespace changes option. Does it related
> to this
> > > > merge proposal: https:/
> > > > whitespace/
> > >
> > > Yes. Looks like the other branch has similar changes except that the
> > > UI is different: I added a menu item to toggle whitespace changes,
> > > the other branch uses a command line parameter.
>
> > @Dorin, can you merge the changes from the other branch, so I can land
> > both branches soon? Or I can land the other branch first and you will
> > update your patch later?
>
> I just merged the changes from this merge proposal:
> https:/
> whitespace/
> , which exposes an --ignore-whitespace option to qdiff.
Excellent! I was just about to take a look at this :)
| Alexander Belchenko (bialix) wrote : | # |
Glen Mailer пишет:
>>> Also I found that External Diff button does not work. Have you tested it?
>> I missed that somehow. Anyway, I fixed it today.
>>
>>> Dorin Scutarașu пишет:
>>>>> I see you have added Ignore whitespace changes option. Does it related
>> to this
>>>>> merge proposal: https:/
>>>>> whitespace/
>>>> Yes. Looks like the other branch has similar changes except that the
>>>> UI is different: I added a menu item to toggle whitespace changes,
>>>> the other branch uses a command line parameter.
>>> @Dorin, can you merge the changes from the other branch, so I can land
>>> both branches soon? Or I can land the other branch first and you will
>>> update your patch later?
>> I just merged the changes from this merge proposal:
>> https:/
>> whitespace/
>> , which exposes an --ignore-whitespace option to qdiff.
>
> Excellent! I was just about to take a look at this :)
Thank you, guys! I will review it ASAP and I hope to land it soon.
--
All the dude wanted was his rug back
| Dorin Scutarașu (dorins) wrote : | # |
Could someone review this please? Is there anything I can do to speed things up?
| Alexander Belchenko (bialix) wrote : | # |
Dorin Scutarașu пишет:
> Could someone review this please? Is there anything I can do to speed things up?
I'm very very very sorry, but it seems your patch misses our deadline
for 0.20 final to be in sync with bzr 2.3.
We will merge it as soon as 0.20 will be ready.
Sorry for the delay :-/
--
All the dude wanted was his rug back
| Dorin Scutarașu (dorins) wrote : | # |
> Sorry for the delay :-/
No problem. I was just curious as to what's holding this up. I was thinking about adding line numbers and "go to line" function, and wanted to work on top of this. I guess I need to figure out how the pipeline plug-in works.
| Alexander Belchenko (bialix) wrote : | # |
I was very busy last months, sorry. I'm going to merge it now. I think there is nothing we need to change or improve, and just need merge it, right?
Gary, are you have any comments on this patch?
| Dorin Scutarașu (dorins) wrote : | # |
> I was very busy last months, sorry. I'm going to merge it now. I think there
> is nothing we need to change or improve, and just need merge it, right?
Yes, as far as I know.
| Alexander Belchenko (bialix) wrote : | # |
Dorin, can you merge the trunk into your branch and resolve conflicts, please? There are some conflicted changes that look trivial, but I'm not quite sure about them. That will help me to do final review.
| Alexander Belchenko (bialix) wrote : | # |
> Dorin, can you merge the trunk into your branch and resolve conflicts, please?
> There are some conflicted changes that look trivial, but I'm not quite sure
> about them. That will help me to do final review.
I did it.
| Alexander Belchenko (bialix) wrote : | # |
Find is not intuitive because it search only on the left pane, I have to click on right pane to force it search there. I'd rather expect it searches both panes
| Alexander Belchenko (bialix) wrote : | # |
I've tested new qdiff but I haven't inspected every single line of your changes.
In the future please avoid unnecessary whitespace-only changes in contribution to QBzr. It breaks annotations and makes the review (without qdiff -w) much harder.
Also I wonder if we want to save state of "Ignore whitespace" knob in some config file (maybe in qbzr.conf or branch.conf) but I'm not sure yet. If you have any opinion on this we can discuss it in our google group ML.
Resume: many thanks for great work to you and Glenjamin. I'm going to land it to trunk and will release 0.21 beta1 soon so many people will have a chance to test it in the action.

Looks nice.
Although it's not very intuitive that clicking lowered Side-by-side button will give me unidiff view.